Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[image_transport] ABI incompatibility? #34

Closed
marc-hanheide opened this issue May 26, 2015 · 7 comments
Closed

[image_transport] ABI incompatibility? #34

marc-hanheide opened this issue May 26, 2015 · 7 comments

Comments

@marc-hanheide
Copy link

The recent changes in image_transport between releases 1.11.4 and 1.11.5 seems to have caused an ABI incompatibility, so that code that was compiled against 1.11.4 segfaults after a system has been updated to use image_transport 1.11.5. I believe this is due to ABI changes introduced by adding virtual methods in the header files publisher_plugin.h and raw_publisher.h.
Since updating to 1.11.5 we have observed random segmentation faults in a number of components that use image_transport, including just running roslaunch openni_launch openni.launch with a Kinect connected, but also others (e.g. our own code). The backtraces always pointed to image_transport:: PublisherPlugin::advertise or in other cases to image_transport:: PublisherPlugin::publish.

I feel that it was wrong to bump just the "patch" component of the version number in this one. Could this be the reason to the problems we are observing? I have had the experience in the past that these kinds of changes lead to segmentation faults when depending projects / packages have not been recompiled.

@jack-oquin
Copy link
Member

I don't think we were aware of any ABI problem with release 1.11.5. We would not have released it to Hydro or Indigo, had we known of one. I agree that changing ABI merits a minor number bump (assuming the source API is compatible).

Which ROS distro are you on?

You have image_common 1.11.15, but older versions of other components depending on it?

@marc-hanheide
Copy link
Author

We are on indigo, and we are using image_transport quite extensively in our project. We updated our whole system last week, when the change from 1.11.4 to 1.11.5 got deployed to our robots. Since this date we are experiencing many segmentation faults in components that publish images via image_transport.

I'm by no means an expert in C++ ABI, but reading KDE's Binary Compatibility Issues With C++ - The Do's and Don'ts and looking at the change set form 1.11.4 -> 1.11.5 it contains some changes that are normally not allowed to maintain ABI compatibility e.g. making a function inline, or adding a virtual function to a base class). As I said, I'm not an expert, but I have seen strange behaviour like symbols not being found or segfaults due to such cases before. And if only the patch number is increased, then AFAIK, neither Debian packages nor ld can figure this out.

Actually, re-compiling our packages solved the issue (in most cases, there is some other odd cases, that need further investigation), but obviously this does not work for packages released. We just did a full apt-get upgrade on our indigo installation (without our own packages installed) and tested roslaunch openni_launch openni.launch which was working perfectly find before and is not working since the update.

@jack-oquin
Copy link
Member

Sounds like we screwed up.

Not sure what to do about it at this point, I'll ask for advice on the ros-releases mailing list.

@vrabaud
Copy link
Contributor

vrabaud commented May 30, 2015

Yes, that was my mistake, I indeed broke the ABI by adding a member function (to provide an API to speed up certain cases). We cannot do anything at this point as it's gone public.
Thx for reporting though, for next time: you should be on shadow-fixed and whenever there is an issue, just mention it and we will block the sync to public. http://wiki.ros.org/ShadowRepository

@tfoote
Copy link
Contributor

tfoote commented May 30, 2015

@marc-hanheide Released packages should be fine. They all get rebuilt whenever a dependency is rebuilt.

@marc-hanheide
Copy link
Author

@tfoote. Yes, agreed. Problem is we have our own build farm and that obviously missed that change. Maybe that could be better configured. I now triggered a rebuild of all our own packages. As i say, still seemed to gave a few problems but can't say for sure yet where they come from. But that could then solely be my own problem. I just wanted confirmation that the bugs we are seeing are indeed caused by the image_transport ABI change. Thanks.

@jack-oquin
Copy link
Member

@marc-hanheide: we thank you for reporting this problem, and apologize for the inconvenience it caused you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants