On Wed, Mar 28, 2018 at 11:10:36AM +0200, Christophe de Dinechin wrote:
>
>
> > On 26 Mar 2018, at 19:06, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote:
> >
> > On Fri, Mar 23, 2018 at 01:05:23PM +0100, Christophe de Dinechin wrote:
> >> 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
> >> Agent::PluginVersionIsCompatible 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.
> >>
> >> The current code therefore relies on implementation dependent knowlege
> >> of how virtual functions are laid out in the vtable, and puts
> >> unnecessary constraints on the changes allowed in the classes
> >> (e.g. it's not allowed to put anything before some member functions)
> >
> > Just to be sure I understand everything properly, as I could not figure
> > it out fully from the commit log... The problem at the moment is that we
> > expect the plugins to call Agent::PluginVersionIsCompatible(), but if
> > the agent ABI drastically changed, we won't be able to do that as the
> > layout of the Agent when the plugin was built and the new layout of the
> > Agent won't match. Given that Agent::PluginVersionIsCompatible() is
> > supposed to be used to detect incompatible ABIs, this could be an issue.
>
> What you are describing is a practical case where we would crash or
> call the wrong function or whatever.
>
> However, in reality, calling PluginVersionIsCompatible is “undefined
> behavior” as soon as the interface of the class changed in any way
> relative to what the plugin was compiled with. That’s what “One
> Definition Rule” means.
>
> Now, to be clear, I’m not advocating that we should mark any change in
> the plugin interface as binary incompatible. Adding a method at the
> end of the agent interface, for example, should be safe with g++, so
> we could say that the ABI version with the added method is “binary
> compatible” with the previous one.
Yup, sounds reasonable.
> What I’m advocating is that we should not rely on undefined behavior to perform that binary compatibility test.
Yes, I agree, I was just making sure I understood your point properly.
> > However, my understanding is that once the version check has succeeded, we
> > assume that ODR violations are not going to cause issues, because we
> > have validated through that version check that the ABI is compatible.
> > In other word, without any version check, we could have issues with
> > calling Agent::Register() from a plugin, but thanks to the version
> > check, we assume that this ODR violation is not going to cause actual
> > issues?
>
> We do not assume. We test it first, and based on that test, we decide
> whether we mark the version binary compatible or not.
>
> Relying on the ODR means that the language does not specify what
> happens. We can, however, make a judgment call based for example on
> the generated code and on our testing, and *after* that judgement call
> decide to call the agent methods.
>
> Does that make sense?
I feel more comfortable with a statement like "as long as we don't break
ABI, we are going to assume that the plugin and agent are going to work
fine together, and that we won't be getting ODR problems". Testing can
only be done with a limited set of compiler and flags, so I won't feel
so confident saying "it worked fine on my machine, it's going to work
fine everywhere".
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!).
The paragraph
"The current code therefore relies on implementation dependent knowlege
of how virtual functions are laid out in the vtable, and puts
unnecessary constraints on the changes allowed in the classes
(e.g. it's not allowed to put anything before some member
functions)"
is probably already plenty enough to describe why we want it, I would
just be explicit that "the changes allowed in the classes" are "the
changes allowed in the classes *when breaking ABI*" (or at least I
understand it this way ;)
Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel