Load global symbols #30

ahcorde · 2020-11-02T15:39:50Z

This is the basic PR to make use of ign-plugin in other libraries.

This comment in this other PR gazebosim/gz-sensors#38 (comment) explains what is going on. Basically some libraries pre-load some symbols of other libraries (for example libignition-gazebo4-sensors-system.so pre-load some ign-sensor symbols) and when you need to load some of the ign-sensor libraries you can't access these symbols because it was load previously.

Using RTLD_GLOBAL we can access theses symbols but we should filter in all the libraries and look for the right one.

Signed-off-by: ahcorde ahcorde@gmail.com

traversaro · 2020-11-02T15:49:16Z

Just as a curiosity, which version of dlfcn-win32 is used in the CI ? vcpkg has 1.1.1 while conda-forge has 1.2.0, and the 1.2.0 had some changes w.r.t. 1.1.1 related also to the RTLD_GLOBAL flag (see dlfcn-win32/dlfcn-win32#44).

chapulina

Do you think it would be feasible to write a test case that fails without this PR?

loader/src/Loader.cc

codecov · 2020-11-02T17:32:25Z

Codecov Report

Merging #30 (7ff136d) into main (03d6551) will decrease coverage by 0.33%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##             main      #30      +/-   ##
==========================================
- Coverage   99.82%   99.48%   -0.34%     
==========================================
  Files          15       15              
  Lines         584      587       +3     
==========================================
+ Hits          583      584       +1     
- Misses          1        3       +2

Impacted Files	Coverage Δ
loader/src/Loader.cc	`99.05% <60.00%> (-0.95%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03d6551...7ff136d. Read the comment docs.

ahcorde · 2021-01-13T09:15:51Z

Thinking a little bit more about this I think this should target main. There are no changes in the API but the behaviour of the library has changed and this will required to modify the existing code that makes use of ign-plugin1. What do you think @mjcarroll and @chapulina ?

mjcarroll · 2021-01-13T14:20:04Z

I agree that main probably makes more sense. @chapulina are we bumping plugin for Edifice?

chapulina · 2021-01-13T17:38:21Z

are we bumping plugin for Edifice?

If you peeps are certain that this change is needed to get ign-sensors to work, I'm ok bumping ign-plugin in Edifice. Every downstream library is already being bumped, so this shouldn't have a large impact.

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde · 2021-01-14T17:05:07Z

Retargeted to main.

ahcorde · 2021-01-26T09:44:02Z

friendly ping @mjcarroll and @chapulina

chapulina · 2021-01-30T00:58:49Z

Just as a curiosity, which version of dlfcn-win32 is used in the CI ?

That's a question for @j-rivero

chapulina · 2021-01-30T01:04:17Z

test/integration/EnablePluginFromThis_TEST.cc

@@ -136,8 +134,6 @@ TEST(EnablePluginFromThis, LibraryManagement)
  }

  EXPECT_TRUE(weak.IsExpired());
-
-  CHECK_FOR_LIBRARY(libraryPath, false);


I believe these checks are here to make sure the library is unloaded correctly. But @mxgrey's comment on the function suggests that it may not be reliable, so that could explain why you had to remove these. What do you think @mxgrey , should we remove the false checks, or adjust the CHECK_FOR_LIBRARY function somehow?

https://github.com/ignitionrobotics/ign-plugin/blob/07c20cbc61007bf0982583b3eecb7761ca5cf7c9/test/integration/utils.hh#L32-L42

chapulina · 2021-01-30T01:05:58Z

test/integration/plugin.cc

+}
+
+/////////////////////////////////////////////////
+TEST(Loader, LoadBadPlugins)


Was there a reason to move this test down here? Was it failing above?

chapulina · 2021-01-30T01:08:47Z

loader/src/Loader.cc

@@ -368,7 +368,7 @@ namespace ignition
 #endif

      void *dlHandle = dlopen(_pathToLibrary.c_str(),
-                              RTLD_NOLOAD | RTLD_LAZY | RTLD_LOCAL);
+                              RTLD_NOLOAD | RTLD_LAZY | RTLD_GLOBAL);


What do you think, @mxgrey , are we trying to do something that ign-plugin wasn't meant to do, or is this a reasonable change?

See gazebosim/gz-sensors#38 (comment) for the reason why this change is needed.

j-rivero · 2021-02-02T13:51:39Z

Just as a curiosity, which version of dlfcn-win32 is used in the CI ?

That's a question for @j-rivero

Uhm. We are using tarball in the ign-plugin build, I can not find the version on the code. The date of the tarball is Nov 2017, probably 1.1.1?

chapulina · 2021-02-02T17:20:13Z

I talked to @mxgrey and he had the nice suggestion of adding a flag to enable the new behaviour. If we do this, we may not need to bump the version and only enable the new behaviour for the libraries that need it.

Then I started looking more closely and noticed that the dlopen call being changed is inside ForgetLibrary. I hadn't noticed this detail before. I think it explains the changes in the tests. But now I'm confused about how this helps ign-sensors. Can you elaborate a bit more on why this is needed, @ahcorde ?

ahcorde · 2021-02-02T18:08:33Z

@chapulina

Adding a flag required to keep the two functionalities at the same time but the idea it's to keep just one way to load libraries (remove the code in ign-common if I remember correctly).

The function dlclose() decrements the reference count on the dynamic library handle. If the reference count drops to zero and no other loaded libraries use symbols in it, then the dynamic library is unloaded. But this is not reliable. Anyhow I'm also using NODELETE to avoid unload something that it's already running. I remember I got some segfault without this .

I not really sure about how this could help the different libraries (ign-sensors, ign-gui, ign-rendering or ign-gazebo). But the idea it's to use ign-plugin which is the library focuses on loading shared libraries. Maybe there is a deeper implementation issue inside ign-plugin and we don't want to move forward with this implemention, because we nned to load all the symbols an filter them in each library.

traversaro · 2021-02-03T07:40:36Z

Just as a curiosity, which version of dlfcn-win32 is used in the CI ?

That's a question for @j-rivero

Uhm. We are using tarball in the ign-plugin build, I can not find the version on the code. The date of the tarball is Nov 2017, probably 1.1.1?

Thanks! I am asking as in the journey to get a reliable working Gazebo with vcpkg-installed dependencies I became the dlfcn-win32 maintainer, even if I am definitively not an expert of Windows shared library interface. However, several users over time contributed quite a lot to the library. In particular in the release 1.2.0 that came out in 2019 (https://github.com/dlfcn-win32/dlfcn-win32/releases/tag/v1.2.0) there have been some works on loading of global symbols (see dlfcn-win32/dlfcn-win32#44). I would expect for everything to work fine for both dlfcn-win32 1.1.1 and 1.2.0, but with this tricky issues problems can always emerge.

Actually I already wrote most of this in the original comment ( #30 (comment) ), but I forgot about that, sorry for the noise. : )

…/symbols

chapulina · 2021-03-23T16:36:07Z

Removing beta label, we won't have time to wrap this up before code freeze. Let's retarget at Ignition-F.

chapulina · 2021-08-17T00:27:33Z

This isn't necessary anymore as we solved the ign-sensors needs by removing plugin functionality.

ahcorde added the 🏢 edifice Ignition Edifice label Nov 2, 2020

ahcorde requested a review from mjcarroll November 2, 2020 15:39

ahcorde requested a review from mxgrey as a code owner November 2, 2020 15:39

ahcorde self-assigned this Nov 2, 2020

github-actions bot added 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Nov 2, 2020

chapulina reviewed Nov 2, 2020

View reviewed changes

loader/src/Loader.cc Outdated Show resolved Hide resolved

loader/src/Loader.cc Show resolved Hide resolved

ahcorde removed 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Nov 3, 2020

ahcorde force-pushed the ahcorde/global/symbols branch 2 times, most recently from c804ef4 to 1b70d52 Compare November 3, 2020 15:57

ahcorde mentioned this pull request Nov 3, 2020

Updated to work with ign-plugin gazebosim/gz-rendering#171

Closed

ahcorde requested a review from chapulina November 4, 2020 09:17

chapulina removed their request for review December 30, 2020 04:04

ahcorde changed the base branch from ign-plugin1 to main January 14, 2021 16:58

ahcorde added 5 commits January 14, 2021 18:04

Load global symbols

3fa65b7

Signed-off-by: ahcorde <ahcorde@gmail.com>

make linters happy

ba7dfa4

Signed-off-by: ahcorde <ahcorde@gmail.com>

Fix some issues

fa3fc44

Signed-off-by: ahcorde <ahcorde@gmail.com>

Fixed tests

f0baa76

Signed-off-by: ahcorde <ahcorde@gmail.com>

Defined RTLD_NONDELETE if not using glibc-2.2

a8160b2

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde force-pushed the ahcorde/global/symbols branch from bb4eb14 to a8160b2 Compare January 14, 2021 17:04

chapulina reviewed Jan 30, 2021

View reviewed changes

chapulina mentioned this pull request Feb 5, 2021

Accept null dlhandle and support custom dlopen flags #35

Closed

Merge remote-tracking branch 'origin/ign-plugin1' into ahcorde/global…

dd19257

…/symbols

chapulina added the beta Targeting beta release of upcoming collection label Mar 17, 2021

Merge branch 'main' into ahcorde/global/symbols

7ff136d

chapulina removed the beta Targeting beta release of upcoming collection label Mar 23, 2021

chapulina added 🏯 fortress Ignition Fortress and removed 🏢 edifice Ignition Edifice labels Apr 1, 2021

chapulina closed this Aug 17, 2021

chapulina deleted the ahcorde/global/symbols branch August 17, 2021 00:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load global symbols #30

Load global symbols #30

ahcorde commented Nov 2, 2020

traversaro commented Nov 2, 2020

chapulina left a comment

codecov bot commented Nov 2, 2020 •

edited

Loading

ahcorde commented Jan 13, 2021

mjcarroll commented Jan 13, 2021

chapulina commented Jan 13, 2021

ahcorde commented Jan 14, 2021

ahcorde commented Jan 26, 2021

chapulina commented Jan 30, 2021

chapulina Jan 30, 2021

chapulina Jan 30, 2021

chapulina Jan 30, 2021

j-rivero commented Feb 2, 2021

chapulina commented Feb 2, 2021

ahcorde commented Feb 2, 2021

traversaro commented Feb 3, 2021 •

edited

Loading

chapulina commented Mar 23, 2021

chapulina commented Aug 17, 2021

Load global symbols #30

Load global symbols #30

Conversation

ahcorde commented Nov 2, 2020

traversaro commented Nov 2, 2020

chapulina left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 2, 2020 • edited Loading

Codecov Report

ahcorde commented Jan 13, 2021

mjcarroll commented Jan 13, 2021

chapulina commented Jan 13, 2021

ahcorde commented Jan 14, 2021

ahcorde commented Jan 26, 2021

chapulina commented Jan 30, 2021

chapulina Jan 30, 2021

Choose a reason for hiding this comment

chapulina Jan 30, 2021

Choose a reason for hiding this comment

chapulina Jan 30, 2021

Choose a reason for hiding this comment

j-rivero commented Feb 2, 2021

chapulina commented Feb 2, 2021

ahcorde commented Feb 2, 2021

traversaro commented Feb 3, 2021 • edited Loading

chapulina commented Mar 23, 2021

chapulina commented Aug 17, 2021

codecov bot commented Nov 2, 2020 •

edited

Loading

traversaro commented Feb 3, 2021 •

edited

Loading