-
Notifications
You must be signed in to change notification settings - Fork 290
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
3 ➡️ 4 #469
Merged
Merged
3 ➡️ 4 #469
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
632d774
Helper function to set component data (#436)
chapulina f02c5e4
Remove unneeded if statement (#432)
08b6c42
Fixes flaky RecordAndPlayback test in INTEGRATION_log_system (#463)
azeey 2f76de8
Make PeerTracker test more robust (#452)
chapulina 74c6dd4
3 ➡️ 4
chapulina 2a25fc7
3 ➡️ 4: fixes for #463 (#469)
chapulina File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ABI checker gave what I assumed was a false positive, see gazebo-tooling/release-tools#336
I inspected the symbols locally just in case, and was surprised to see that this symbol was actually gone:
_ZNK8ignition6gazebo2v410components13BaseComponent9SerializeERSo
I narrowed it down to line 955. When it's removed, the symbol disappears with it 😨 🤷♀️
You can see all symbols before and after removing the line here: https://gist.github.com/chapulina/7f53a3d43cb031715f005650e582fcb7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like that symbol is not used by anything in
libignition-gazebo.so
so it must be getting optimized away. If you build in debug mode, it doesn't get removed.Since the function is defined in a header, I think this is somewhat of a false positive. Any translation unit that includes the
Component.hh
header will have access to symbol. For example,UNIT_Component_TEST
still has the symbol even though this it is missing fromlibignition-gazebo.so
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum interesting and mysterious how the compiler decides to remove the symbol. Your reasoning about the ABI not being broken makes sense to me. I guess all functions defined in headers may be subject to the same rule.
Makes me wonder about why the symbol would be added in the first place. When the compiler does add the symbol, I guess it can be used directly instead of regenerated when the header is included?
This messes up the poor ABI checker 😕