Skip to content

backwards-cpp support for gz-sim-main and gz-sim-gui #2919

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

j-rivero
Copy link
Contributor

🎉 New feature

Closes #2906

Summary

The PR adds support for backwards-cpp in gz-sim-main and gz-sim-gui. To do so:

  • Bump backwards vendor to 1.6 5913812
  • Relocate the backwards vendor directory outside of the root to be used by CLI 965d463
  • Build binaries with backwards support fd0edc9

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • 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 added 3 commits May 22, 2025 12:23
Signed-off-by: Jose Luis Rivero <[email protected]>
Signed-off-by: Jose Luis Rivero <[email protected]>
@j-rivero j-rivero requested a review from arjo129 May 22, 2025 10:26
@j-rivero j-rivero requested a review from mjcarroll as a code owner May 22, 2025 10:26
@github-actions github-actions bot added the 🪵 jetty Gazebo Jetty label May 22, 2025
Copy link
Contributor

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

I tried this branch and introduced a segfault, however the stack traces are not being printed. I'll go through and check more thoroughly.

@github-project-automation github-project-automation bot moved this from Inbox to In review in Core development May 22, 2025
@arjo129
Copy link
Contributor

arjo129 commented May 22, 2025

So after reverting the version of backward to the current one, I managed to link it to backward_object and it works correctly in this branch: https://github.com/gazebosim/gz-sim/tree/arjo/properly_segfaults_w_stacktrace

I don't think the above branch has the ideal solution however. TBH the CMake-fu going on in the library exceeds my domain of knowledge. One option is to get rid of all the cmake stuff and only include the header. we can manually instantiate BackwardHandle in the C++ binary.

I think whats happening is that Backward::Backward is getting optimized out since we dont use it in our code.

LMK what you think @j-rivero

@j-rivero
Copy link
Contributor Author

I tried this branch and introduced a segfault, however the stack traces are not being printed.

Makes sense. The build log does not display any modifcation of code being done by the linker. re-reading the backwards-cpp lib instructions we can use either the header file or the .cpp file. I think that you are using the later when linking against backwards_object target. I would prefer to keep the installation more standard by using the header and the Backwards::Backwards CMake target.

Said that, I've brought the backwards-cpp code into the main/gui in 42383a5 by including the headers and the signal handler. Segfaulting on propose triggers clear stacktraces on my system after using RelWithDebInfo.

@arjo129 arjo129 linked an issue May 23, 2025 that may be closed by this pull request
Copy link
Contributor

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

This works great for the binaries but introduces a regression on the tests where segfaulting tests no longer print stack traces.

CMakeLists.txt Outdated
if (UNIX AND NOT APPLE)
set (EXTRA_TEST_LIB_DEPS stdc++fs backward_object)
set (EXTRA_TEST_LIB_DEPS stdc++fs Backward::Backward)
Copy link
Contributor

@arjo129 arjo129 May 23, 2025

Choose a reason for hiding this comment

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

Sadly, this won't work for the tests, perhaps Backward::Object? Or linking tests with whole-archive options enabled.

@j-rivero
Copy link
Contributor Author

j-rivero commented May 23, 2025

This works great for the binaries but introduces a regression on the tests where segfaulting tests no longer print stack traces.

You are right. I've reverted the usage of the header-only files, I think that it is more intrusive for the code 255dd43.

For using the linking approach and the "object" CMake target we need to stop using the find_package and use the add_subdirectory as detailed in the backwards-cpp README. Changing that and using Backwards::Object was good enough for the binaries.

For the special case of EXTRA_TEST_LIB_DEPS I think that CMake is not resolving properly that CMake Backwards::Object so I used the backward_object instead.

Seems good enough for me after tesing tests and binaries 0109512

@j-rivero j-rivero requested a review from arjo129 May 23, 2025 11:46
Signed-off-by: Arjo Chakravarty <[email protected]>
Copy link
Contributor

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM! There was a minor style issue with regards to spacing, I've pushed a commit that fixes it.

@arjo129 arjo129 enabled auto-merge (squash) May 29, 2025 04:56
@j-rivero
Copy link
Contributor Author

Ubuntu servers are having a bad day. To merge the PR I wanted to give a look into how much overload in time we are adding with this PR.

Comparing a couple of build from the same day, 28 May:

Let's see if we can get data from today builds when servers are back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪵 jetty Gazebo Jetty
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

Restore Stack Traces in jetty
2 participants