> 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. What I’m advocating is that we should not rely on undefined behavior to perform that binary compatibility test. > > After your change, the plugin compatibility detection is changed to be > done from the agent, and it only needs to get an int from the plugin, > which we should be able to not break from an ABI point of view. Well, we can, you demonstrated it in your “split patches”. But the way we do that (e.g. the name of the variable and the values in it) is precisely under our control, unlike the layout of C++ objects. Of course, there are ways a plugin could break it voluntarily, e.g. setting the value of the variable to some arbitrarily high value. But the chances of breaking it by accident are very low. > > 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? Christophe > > 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