Add option to use RTLD_NODELETE when loading a library #102

azeey · 2022-08-15T21:08:44Z

🎉 New feature

Summary

Some of our segfaults in gz-sim runs and tests (flaky tests) is caused by the access of memory (data or code) that resides in a plugin/shared library after the library has been unloaded and deleted. With RTLD_NODELETE, dlclose will unload the shared library and run it's fini function, but doesn't actually delete it. Thus, access to code (eg. destructors) after the library has been "dlclosed" will not cause a segfault.

Test it

TODO

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.

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

codecov · 2022-08-15T21:13:41Z

Codecov Report

Merging #102 (591243e) into gz-plugin2 (daa9a65) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 591243e differs from pull request most recent head 4f988f0. Consider uploading reports for the commit 4f988f0 to get more accurate results

@@             Coverage Diff             @@
##           gz-plugin2     #102   +/-   ##
===========================================
  Coverage       98.25%   98.26%           
===========================================
  Files              23       23           
  Lines             745      748    +3     
===========================================
+ Hits              732      735    +3     
  Misses             13       13

Impacted Files	Coverage Δ
loader/src/Loader.cc	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

chapulina · 2022-08-15T23:16:23Z

This could target ign-plugin1. It's backwards-compatible and also a bug to be fixed in earlier versions.

ahcorde

is there any way to add some test for this new method?

loader/include/gz/plugin/Loader.hh

scpeters · 2022-08-18T08:25:57Z

loader/src/Loader.cc

@@ -431,7 +437,8 @@ namespace gz

      // NOTE: We open using RTLD_LOCAL instead of RTLD_GLOBAL to prevent the
      // symbols of different libraries from writing over each other.
-      void *dlHandle = dlopen(_full_path.c_str(), RTLD_LAZY | RTLD_LOCAL);
+      int dlopenMode = RTLD_LAZY | RTLD_LOCAL | (_noDelete ? RTLD_NODELETE : 0);


we might need an ifdef for RTLD_NODELETE on windows

Done in 644f613

Apparently, the behavior of dlopen is different between Linux and macOS. On Linux, if you dlopen (with RTLD_NODELETE) , dlclose, and dlopen again, since the plugin is not deleted, dlopen will reuse the existing plugin. On macOS, however, even though the plugin is not deleted, dlopen will treat the second dlopen as though a new library is being loaded and end up loading it into a new memory location. Thus, the type of test I was using won't work. Since we're running out of time, I've decided to enable the test only on Linux. If there are other ways to test this, I'm open to ideas.

Should we add __APPLE__ to the ifdef?

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

azeey · 2022-08-23T23:09:25Z

is there any way to add some test for this new method?

Added a test in 739ae12.

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

azeey · 2022-08-26T21:50:00Z

@ahcorde Apparently, the behavior of dlopen is different between Linux and macOS. On Linux, if you dlopen (with RTLD_NODELETE) , dlclose, and dlopen again, since the plugin is not deleted, dlopen will reuse the existing plugin. On macOS, however, even though the plugin is not deleted, dlopen will treat the second dlopen as though a new library is being loaded and end up loading it into a new memory location. Thus, the type of test I was using won't work. Since we're running out of time, I've decided to enable the test only on Linux. If there are other ways to test this, I'm open to ideas.

jennuine

This LGTM, left a minor comment

jennuine · 2022-08-30T18:42:32Z

loader/src/Loader.cc

@@ -431,7 +437,8 @@ namespace gz

      // NOTE: We open using RTLD_LOCAL instead of RTLD_GLOBAL to prevent the
      // symbols of different libraries from writing over each other.
-      void *dlHandle = dlopen(_full_path.c_str(), RTLD_LAZY | RTLD_LOCAL);
+      int dlopenMode = RTLD_LAZY | RTLD_LOCAL | (_noDelete ? RTLD_NODELETE : 0);


Apparently, the behavior of dlopen is different between Linux and macOS. On Linux, if you dlopen (with RTLD_NODELETE) , dlclose, and dlopen again, since the plugin is not deleted, dlopen will reuse the existing plugin. On macOS, however, even though the plugin is not deleted, dlopen will treat the second dlopen as though a new library is being loaded and end up loading it into a new memory location. Thus, the type of test I was using won't work. Since we're running out of time, I've decided to enable the test only on Linux. If there are other ways to test this, I'm open to ideas.

Should we add __APPLE__ to the ifdef?

azeey · 2022-08-30T18:56:56Z

@jennuine , RTLD_NODELETE still works on macOS, it just that dlopen behaves differently when trying to load a plugin that's already in memory. I think the main thing issue with the segfaults we were getting was that some class destructors were being deleted when a plugin got unloaded, and being accessed by the system afterward. Using RTLD_NODELETE should prevent the segfault on macOS as well, although I haven't tested this.

azeey · 2022-08-30T20:07:22Z

@ahcorde, do you have time to give this another look?

ahcorde

Some minor style comments

test/integration/plugin_unload.hh

loader/include/gz/plugin/Loader.hh

Signed-off-by: Louise Poubel <[email protected]>

Add option to use RTLD_NODELETE when loading a library

ecb936f

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

azeey requested review from mxgrey and ahcorde as code owners August 15, 2022 21:08

github-actions bot added the 🌱 garden Ignition Garden label Aug 15, 2022

chapulina added the bug Something isn't working label Aug 15, 2022

azeey mentioned this pull request Aug 15, 2022

Fix crash at exit of gz-sim by using RTLD_NODELETE=true when loading libraries gazebosim/gz-gui#469

Merged

8 tasks

ahcorde requested changes Aug 16, 2022

View reviewed changes

chapulina reviewed Aug 16, 2022

View reviewed changes

loader/include/gz/plugin/Loader.hh Show resolved Hide resolved

azeey mentioned this pull request Aug 16, 2022

Use RTLD_NODELETE=true when loading libraries gazebosim/gz-sim#1649

Merged

8 tasks

Merge branch 'gz-plugin2' into use_nodelte

32199ba

scpeters reviewed Aug 18, 2022

View reviewed changes

azeey added 2 commits August 23, 2022 17:51

Add test

739ae12

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

Add #ifdef guard for windows

644f613

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

Disable test on macOS

591243e

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

azeey requested a review from ahcorde August 26, 2022 21:50

jennuine approved these changes Aug 30, 2022

View reviewed changes

ahcorde approved these changes Aug 31, 2022

View reviewed changes

test/integration/plugin_unload.hh Outdated Show resolved Hide resolved

test/integration/plugin_unload.hh Show resolved Hide resolved

loader/include/gz/plugin/Loader.hh Outdated Show resolved Hide resolved

PR feedback

4f988f0

Signed-off-by: Louise Poubel <[email protected]>

chapulina enabled auto-merge (squash) August 31, 2022 16:18

chapulina merged commit c189b78 into gazebosim:gz-plugin2 Aug 31, 2022

mjcarroll mentioned this pull request Aug 31, 2022

Fix segfault when destroying Event objects gazebosim/gz-common#367

Closed

8 tasks

azeey mentioned this pull request Jan 31, 2024

Gazebo (Fortress) GUI crashes at exit when lto is enabled gazebosim/gz-sim#2301

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to use RTLD_NODELETE when loading a library #102

Add option to use RTLD_NODELETE when loading a library #102

azeey commented Aug 15, 2022

codecov bot commented Aug 15, 2022 •

edited

Loading

chapulina commented Aug 15, 2022

ahcorde left a comment

scpeters Aug 18, 2022

azeey Aug 23, 2022

jennuine Aug 30, 2022

azeey commented Aug 23, 2022

azeey commented Aug 26, 2022

jennuine left a comment

jennuine Aug 30, 2022

azeey commented Aug 30, 2022

azeey commented Aug 30, 2022

ahcorde left a comment

Add option to use RTLD_NODELETE when loading a library #102

Add option to use RTLD_NODELETE when loading a library #102

Conversation

azeey commented Aug 15, 2022

🎉 New feature

Summary

Test it

Checklist

codecov bot commented Aug 15, 2022 • edited Loading

Codecov Report

chapulina commented Aug 15, 2022

ahcorde left a comment

Choose a reason for hiding this comment

scpeters Aug 18, 2022

Choose a reason for hiding this comment

azeey Aug 23, 2022

Choose a reason for hiding this comment

jennuine Aug 30, 2022

Choose a reason for hiding this comment

azeey commented Aug 23, 2022

azeey commented Aug 26, 2022

jennuine left a comment

Choose a reason for hiding this comment

jennuine Aug 30, 2022

Choose a reason for hiding this comment

azeey commented Aug 30, 2022

azeey commented Aug 30, 2022

ahcorde left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 15, 2022 •

edited

Loading