> On 28 Mar 2018, at 17:20, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote: > > 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 ;) That’s correct. > > Christophe > _______________________________________________ > Spice-devel mailing list > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/spice-devel _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel