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.

Sign up for GitHub

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

Jump to bottom

Fix symbol checking test when compiled with debug symbols #1474

Merged
merged 2 commits into from
Aug 27, 2024
Merged

Fix symbol checking test when compiled with debug symbols #1474

merged 2 commits into from
Aug 27, 2024

Conversation

j-rivero
Copy link
Contributor

🦟 Bug fix

Summary

The test that checks for prefixed binary symbols was broken when compiled with DebWithRelInfo since it was checking debugging symbols that broke the heuristics used.

The commit fixes it doing a couple of actions:

  • Include the length of the namespace sdf according to the mangling rules: 3sdf
  • Check only dynamic symbols being exported

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • 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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@j-rivero
Fix symbol checking test
a26d156 
The test that checks for prefixed binary symbols was broken when
compiled with DebWithRelInfo since it was checking debugging symbols
that broke the heuristics used.

The commit fixes it doing a couple of actions:
 - Include the length of the namespace sdf: 3sdf
 - Check only dynamic symbols being exported

Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero
Copy link
Contributor Author

Tested (pending): Build Status

@scpeters
Copy link
Member

Tested (pending): Build Status

trying again with different branch parameters

@j-rivero @scpeters
Update test/integration/all_symbols_have_version.bash.in
755cf20 
Co-authored-by: Steve Peters <[email protected]>
Signed-off-by: Jose Luis Rivero <[email protected]>
@azeey
Copy link
Collaborator

azeey commented Aug 27, 2024

I was also working on this and found that adding the --extern-only flag to nm fixes the issue. From what I can understand, the issue is that debbuilders enable LTO which causes some debug symbols to be added to the shared library. These symbols contain the file names, one of which is :

0000000000f9c8ce N sdformat15_get_install_prefix_impl.cc.b80149f8

This causes a false negative on the versioned symbol test.

I tested by adding set(CMAKE_INTERPROCEDURAL_OPTIMIZATION TRUE) in the root CMakeLists.txt file and was able to reproduce the test failure locally.

In general, it would be good to enable LTO on our libraries so these types of issues are caught at the PR stage. I think this is the third time this LTO stuff has bit us.

@scpeters
Copy link
Member

In general, it would be good to enable LTO on our libraries so these types of issues are caught at the PR stage. I think this is the third time this LTO stuff has bit us.

should we do this in the action-gz-ci workflow?

scpeters
scpeters approved these changes Aug 27, 2024
View reviewed changes
Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve since this fixes it, but defer to @azeey if he suggests a different approach

@azeey
Copy link
Collaborator

azeey commented Aug 27, 2024

In general, it would be good to enable LTO on our libraries so these types of issues are caught at the PR stage. I think this is the third time this LTO stuff has bit us.

should we do this in the action-gz-ci workflow?

I was thinking of enabling it via CMake, maybe in gz-cmake or on a per-project basis. That way local development will also use the same flags.

azeey
azeey approved these changes Aug 27, 2024
View reviewed changes
Copy link
Collaborator

@azeey azeey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested by adding a function outside of the inline namespace and verified that it failed.

@azeey azeey merged commit 904706c into main Aug 27, 2024
11 checks passed
@azeey azeey deleted the jrivero/fix_symbol_checking branch August 27, 2024 19:32
@scpeters
Copy link
Member

@Mergifyio backport sdf15 sdf14

@mergify Mergify
Copy link
Contributor

mergify bot commented Aug 27, 2024 •
edited
Loading

backport sdf15 sdf14

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Aug 27, 2024
@j-rivero @mergify
Fix symbol checking test when compiled with debug symbols (#1474)
e26c523 
* Fix symbol checking test

The test that checks for prefixed binary symbols was broken when
compiled with DebWithRelInfo since it was checking debugging symbols
that broke the heuristics used.

The commit fixes it doing a couple of actions:
 - Include the length of the namespace sdf: 3sdf
 - Check only dynamic symbols being exported

Signed-off-by: Jose Luis Rivero <[email protected]>

* Update test/integration/all_symbols_have_version.bash.in

Co-authored-by: Steve Peters <[email protected]>
Signed-off-by: Jose Luis Rivero <[email protected]>

---------

Signed-off-by: Jose Luis Rivero <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
(cherry picked from commit 904706c)
mergify bot pushed a commit that referenced this pull request Aug 27, 2024
@j-rivero @mergify
Fix symbol checking test when compiled with debug symbols (#1474)
ae32d3a 
* Fix symbol checking test

The test that checks for prefixed binary symbols was broken when
compiled with DebWithRelInfo since it was checking debugging symbols
that broke the heuristics used.

The commit fixes it doing a couple of actions:
 - Include the length of the namespace sdf: 3sdf
 - Check only dynamic symbols being exported

Signed-off-by: Jose Luis Rivero <[email protected]>

* Update test/integration/all_symbols_have_version.bash.in

Co-authored-by: Steve Peters <[email protected]>
Signed-off-by: Jose Luis Rivero <[email protected]>

---------

Signed-off-by: Jose Luis Rivero <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
(cherry picked from commit 904706c)
This was referenced Aug 27, 2024
Merged
Merged
scpeters pushed a commit that referenced this pull request Aug 27, 2024
@j-rivero @scpeters
Fix symbol checking test when compiled with debug symbols (#1474)
c306c57 
* Fix symbol checking test

The test that checks for prefixed binary symbols was broken when
compiled with DebWithRelInfo since it was checking debugging symbols
that broke the heuristics used.

The commit fixes it doing a couple of actions:
 - Include the length of the namespace sdf: 3sdf
 - Check only dynamic symbols being exported

Signed-off-by: Jose Luis Rivero <[email protected]>

* Update test/integration/all_symbols_have_version.bash.in

Co-authored-by: Steve Peters <[email protected]>
Signed-off-by: Jose Luis Rivero <[email protected]>

---------

Signed-off-by: Jose Luis Rivero <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
(cherry picked from commit 904706c)
scpeters pushed a commit that referenced this pull request Aug 27, 2024
@j-rivero @scpeters
Fix symbol checking test when compiled with debug symbols (#1474)
68159a7 
* Fix symbol checking test

The test that checks for prefixed binary symbols was broken when
compiled with DebWithRelInfo since it was checking debugging symbols
that broke the heuristics used.

The commit fixes it doing a couple of actions:
 - Include the length of the namespace sdf: 3sdf
 - Check only dynamic symbols being exported

Signed-off-by: Jose Luis Rivero <[email protected]>

* Update test/integration/all_symbols_have_version.bash.in

Co-authored-by: Steve Peters <[email protected]>
Signed-off-by: Jose Luis Rivero <[email protected]>

---------

Signed-off-by: Jose Luis Rivero <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
(cherry picked from commit 904706c)
This was referenced Aug 27, 2024
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scpeters scpeters scpeters approved these changes

@azeey azeey azeey approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Core development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@j-rivero @scpeters @azeey