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

Error: move << operator from .hh to .cc file #623

Merged
merged 2 commits into from
Jul 13, 2021

Conversation

scpeters
Copy link
Member

🦟 Bug fix

Fixes seg-fault of test in ign-physics when printing sdf::Error messages.

Summary

An ign-physics test was observed to seg-fault in gazebosim/gz-physics#261 when printing sdf::Error messages (see gazebosim/gz-physics#261 (comment)). From that comment:

I can now reproduce it with sdformat build from source. The trick is to build it with g++ 7.5 as it's done by the nightly debbuilder. My new hypothesis is that sdf::operator<<(std::ostream, const std::Error&) is inlined in ign-physics code because it's defined in sdf/Error.hh. And since ign-physics is compiled with g++ 8, there might be some weird ABI issues between symbols accessed by the inlined code and the ones available in libsdformat.so.

I see two options:

  1. Move sdf::operator<<(std::ostream, const std::Error&) to Error.cc so it won't get inlined.
  2. Use g++8 in the sdformat nightly debbuilder job.

This takes the second approach. I'm interested to see if the ABI checker is ok with this.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

@scpeters scpeters force-pushed the error_stream_operator_cc_11 branch from bb03dfd to 7af69a0 Compare July 13, 2021 07:53
@scpeters
Copy link
Member Author

CI is not happy; maybe @azeey has a better idea how to implement this?

Signed-off-by: Addisu Z. Taddese <[email protected]>
@azeey
Copy link
Collaborator

azeey commented Jul 13, 2021

Fixed compiler error and visibility issues in 1432e97

@scpeters
Copy link
Member Author

Fixed compiler error and visibility issues in 1432e97

thanks!

@scpeters
Copy link
Member Author

@azeey I'll let you merge if you approve

@mjcarroll mjcarroll merged commit cb6cd03 into gazebosim:sdf11 Jul 13, 2021
scpeters added a commit to scpeters/sdformat that referenced this pull request Jul 14, 2021
* Error: move << operator from .hh to .cc file

Signed-off-by: Steven Peters <[email protected]>

* Fix visibility

Signed-off-by: Addisu Z. Taddese <[email protected]>

Co-authored-by: Addisu Z. Taddese <[email protected]>
@scpeters scpeters deleted the error_stream_operator_cc_11 branch July 14, 2021 04:18
@scpeters
Copy link
Member Author

backport to sdf9 in #625

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

Successfully merging this pull request may close these issues.

4 participants