On Thu, Mar 29, 2018 at 10:36:06AM +0200, Christophe de Dinechin wrote:
>
>
> > On 29 Mar 2018, at 10:05, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote:
> >
> > On Wed, Mar 28, 2018 at 05:54:21PM +0200, Christophe de Dinechin wrote:
> >>> With that said, I find the current "ODR" portion of the commit log
> >>> misleading, it's easy to get the impression that after this change, we
> >>> won't have any ODR problem anymore (this is what I initially thought!).
> >>
> >> Why do you find it misleading?
> >>
> >> The precise wording is "The current plugin version check violates the
> >> C++ ODR”. It specifically talks about “current plugin version check”.
> >> And there is indeed no ODR violation in the version check after that
> >> change.
> >>
> >> Whether there are ODR violations elsewhere, and whether we accept such
> >> violations purposefully on a case by case basis for other calls is a
> >> judgement call. The mechanism makes it entirely possible to avoid ODR
> >> violations completely (at the cost of unnecessarily restricting binary
> >> compatibility).
> >>
> >> So I don’t see it as misleading or implying that it magically gets rid
> >> of other ODR violations.
> >
> > As long as we don't change the ABI, the version check is as much
> > an ODR violation as any other part of the code.
>
> In the present code, we violate the ODR *before* being able to assess if that’s an ODR violation we are OK with.
Yes, this is also what I was saying ;) Though we could alternatively
mandate that the first few fields of the class must never change, in
which case the version check would still be an ODR violation, but one we
are fine with.
> In any case, could you suggest a wording that you think would be less “misleading”?
I would use something like this:
2. ODR-related problems
The C++ One Definition Rule (ODR) states that all translation units
must see the same definitions. In the current code, when we call
any Agent method from the plugin from the plugin, it is an ODR
violation as soon as we have made any change in the Agent class
compared to what the plugin was compiled against. As long as the
agent/plugin ABI does not change, this is deemed acceptable and
should not cause actual runtime issues.
However, the current version check (which is done through
Agent::PluginVersionIsCompatible) relies on the layout of the Agent
class not changing, which puts unnecessary constraints on the changes
allowed in the classes when breaking ABI (e.g. it's not allowed to
put anything before some member functions). Given that this version
check is used to detect ABI incompatibilities, it's better to make
it rely as little as possible on the ABI.
Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel