Skip to content

Accept null dlhandle and support custom dlopen flags#35

Closed
chapulina wants to merge 12 commits intoign-plugin1from
chapulina/global/symbols
Closed

Accept null dlhandle and support custom dlopen flags#35
chapulina wants to merge 12 commits intoign-plugin1from
chapulina/global/symbols

Conversation

@chapulina
Copy link
Contributor

@chapulina chapulina commented Feb 5, 2021

After digging through #30, I was able to get ign-sensors to work with only this change, without touching the local / global variables. It seems to me that this change is backwards compatible, because it only tries the nullptr after trying the usual method first. Update: this PR also allows changing setting to global.

We know no previous use cases were getting a null handle because we haven't run into that assert. So I think this is safe to go into ign-plugin1.

Unfortunately, I have no idea what leads us into this situation, so I'm not able to write a test case for it 😕

Let me know what you think, @ahcorde .

Here's the ign-sensors PR that uses this: gazebosim/gz-sensors#90

ahcorde and others added 6 commits February 4, 2021 17:23
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina requested a review from ahcorde February 5, 2021 01:42
@chapulina chapulina requested a review from mxgrey as a code owner February 5, 2021 01:42
@github-actions github-actions bot added 🏢 edifice Ignition Edifice 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Feb 5, 2021
@codecov
Copy link

codecov bot commented Feb 5, 2021

Codecov Report

Merging #35 (0317f4c) into ign-plugin1 (a80598b) will decrease coverage by 1.18%.
The diff coverage is 30.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-plugin1      #35      +/-   ##
===============================================
- Coverage        99.82%   98.64%   -1.19%     
===============================================
  Files               15       15              
  Lines              584      592       +8     
===============================================
+ Hits               583      584       +1     
- Misses               1        8       +7     
Impacted Files Coverage Δ
loader/src/Loader.cc 96.75% <30.00%> (-3.25%) ⬇️

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 a80598b...54e2981. Read the comment docs.

@ahcorde
Copy link
Contributor

ahcorde commented Feb 5, 2021

I made a cimment about this here gazebosim/gz-sensors#90 (comment)

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina changed the title Accept null dlhandle Accept null dlhandle and support custom dlopen flags Feb 10, 2021
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Copy link
Contributor Author

I made a cimment about this here

@ahcorde , I think the current version of this PR addresses those test failures. This PR adds support for RTLD_GLOBAL just like #30 does, with the advantage of:

  • not requiring a version bump
  • allowing the flexibility of not applying the change to all libraries
  • allowing the flexibility of restoring the RTLD_LOCAL behaviour in the future if we want

Let me know what you think.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Copy link
Contributor Author

I started writing some tests using RLTD_GLOBAL in 54e2981 and it's not looking good. Tests crash on shutdown, crash when loading libraries depending on the flag combination.

I'm afraid that we get this in, start using RTLD_GLOBAL, and start seeing runtime failures throughout the stack.

@chapulina chapulina added the beta Targeting beta release of upcoming collection label Mar 17, 2021
@chapulina chapulina self-assigned this Mar 19, 2021
@chapulina chapulina removed the beta Targeting beta release of upcoming collection label Mar 23, 2021
@chapulina
Copy link
Contributor Author

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

@chapulina
Copy link
Contributor Author

Let's table this idea for now, tweaking the flags is probably not the best way to go about this.

@chapulina chapulina closed this May 13, 2021
@scpeters scpeters deleted the chapulina/global/symbols branch March 6, 2023 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

📜 blueprint Ignition Blueprint 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants