search

gazebosim / gazebo-classic

Gazebo classic. For the latest version, see https://github.com/gazebosim/gz-sim
http://classic.gazebosim.org/
Other
1.2k stars 481 forks source link

Risk of mixing ign-math2 and ign-math3 ABIs in gazebo8 #2141

Closed osrf-migration closed 6 years ago

osrf-migration commented 7 years ago

Original report (archived issue) by Jose Luis Rivero (Bitbucket: Jose Luis Rivero, GitHub: j-rivero).

This issue follows up the statement made by @traversaro in a gazebo PR:

"If there is an ABI breakage between ign-math2 and ign-math3, ign-msgs functions would use ign-math2's ABI, while Gazebo would use ign-math3's ABI, with runtime errors when Gazebo is calling ign-msgs functions that use ignition::math objects."

Given that we are exposing the same symbols in ign-math2 and ign-math3, there is a risk of ABI breakage if those are being exposed in the public interfaces of the different ignition robotics libraries.

Short-term action: In order to avoid the potential risk in gazebo8, I proposed to migrate all the gazebo dependencies to ign-math3, release them and make gazebo8 to depend strictly on those versions using ign-math3 or newer.

Proper solution: To avoid the binary symbol conflict we can expore the option of using inline namespaces in the different ignition libraries. The method is described for libstdc++ or in the mongdb project

osrf-migration commented 7 years ago

Original comment by Jose Luis Rivero (Bitbucket: Jose Luis Rivero, GitHub: j-rivero).

I've implemented the inline namespace for the Box class of ign-math as a test and it works as expected. Note that I did not need to modify the Box_TEST.cc file, which means that third party software is ok with using ignition/math generic namespaces.

jrivero@nium ~ $ nm -D
/usr/lib/x86_64-linux-gnu/libignition-math3.so.3.0.0  | grep Box | head -n 1
000000000000fba0 T _ZN8ignition4math3Box3MaxEv

jrivero@nium ign-math $ (inline_namespaces_tests *) nm -D
build/src/libignition-math3.so.3.0.0  | grep Box | head -n 1
000000000000fc10 T _ZN8ignition4math2v33Box3MaxEv
osrf-migration commented 7 years ago

Original comment by Jose Luis Rivero (Bitbucket: Jose Luis Rivero, GitHub: j-rivero).

I'm pretty sure that we could have worked around the ABI problems by using the inline namespaces, however there are other problems coming from mixing ignition libraries (like having a software which requires different dependencies that in configuring time end up calling two different cmake modules of the same ignition library). There are solution for these problems too, but they are not trivial and discouraged at this time of the gazebo8 release.

We finally decided to go with the following plan for releasing gazebo8:

osrf-migration commented 7 years ago

Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).

osrf-migration commented 7 years ago

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

Is there more to be done here? Should we retitle this issue to mention inline namespaces?

osrf-migration commented 7 years ago

Original comment by Silvio Traversaro (Bitbucket: traversaro).

My 2 cents: in the specific case of gazebo and ignition-math, using inline namespaces will make the problem of mixing ABI more explicit (a compilation error rather then a run-time enigmatic crash) but will not enable mixing different major versions of ignition-math and its dependencies.

inline namespaces work fine in preventing errors in "private" dependencies, such as the two (incompatible) copies of ipopt in this bug: https://osrf-migration.github.io/gazebo-gh-pages/#!/osrf/gazebo/issues/2005/segfault-when-calling-ipopt-virtual-method (#2005) . In Gazebo and its dependencies case, however, the ignition-math classes are used in all the public interfaces, so it is common practice that Gazebo calls a method in a dependent library that returns an ignition-math object, and assigns it to a member of a Gazebo class, that itself is a ignition-math object . Using inline namespaces all this assignements will result in compile errors, if two different versions of ignition-math would be used across gazebo and its dependencies.

osrf-migration commented 6 years ago

Original comment by Jose Luis Rivero (Bitbucket: Jose Luis Rivero, GitHub: j-rivero).

fixed some time ago