Page MenuHomePhabricator

Allow libraries to be loaded with RTLD_LOCAL on Unix.
AcceptedPublic

Authored by marsupial on May 29 2017, 1:53 PM.

Details

Diff Detail

Event Timeline

marsupial created this revision.May 29 2017, 1:53 PM
v.g.vassilev edited edge metadata.Jul 5 2017, 2:36 PM

This patch doesn't apply for me on top of D33529.

Works fine here against master, though D33658 did have a conflict.

joerg added a subscriber: joerg.Jul 6 2017, 5:43 AM

The bigger question for me is: what depends on the RTLD_GLOBAL behavior? I generally consider using it a design bug. I.e. what breaks if we just change to RTLD_LOCAL?

From what I can tell, in LLVM the clients of this are solely clang and lldb.
Nothing should break for them as they explicitly search single libraries for symbols.

D33529 attempts to mitigate any problems that might occur for external usage by giving greater control of how symbols are searched.

But moving to RTLD_LOCAL by default though, may cause breakage for clients of the JIT as a normal pattern would be:

1. DynamicLibrary::getPermanentLibrary(nullptr); // synonym for Process = dlopen(nullptr, RTLD_LAZY  | RTLD_GLOBAL);
2. // Other code...
3. DynamicLibrary::getAddressOfSymbol("SomeSymbol"); // synonym for dlsym(Process, "SomeSymbol");

A. If any other libraries were loaded RTLD_LOCAL by default in step 2, step 3 would no longer find the symbols. (though again, D33529 should make it easy to update their implementation).
B. On Windows this behaviour would differ as there is no concept of RTLD_LOCAL and that would have to be tracked by hand if parity was wanted.

I kind of think the cost against A is worth it as the injection of symbols globally is generally bad, but the disparity of behaviour from B gives me pause.
The easiest solution for B would rely on Module Handles being the same across calls which I don't think is documented but perhaps worth the risk?

In other words D33529 broke the cases where we dlopen with RTLD_LOCAL (like cling and ROOT) and this should fix it.

@marsupial I've tested these patches and it looks like they do not fix the regression. I'd be interested in solving this issue for the upcoming release. If nothing works I will have to revert D33529 :(

This has nothing to do with fixing your issue and is solely to allow clients to load via RTLD_LOCAL.
I don't see how D33529 could have broken anything as it is not committed yet. D33529 attempts to address what I understand is your issue.
If it's not working perhaps you could explain or provide a bit more info what doesn't work...(on that thread not here).
Perhaps @pcanal can help with that?

I meant to reference D30107 instead of D33529, sorry.

@joerg Any further thoughts/desire to review this?
Finishing this off would be nice and at least opens the door to using RTLD_LOCAL.

v.g.vassilev added a subscriber: lhames.

This LGTM but I'd like to hear from @lhames.

v.g.vassilev accepted this revision.Aug 27 2017, 11:54 AM
This revision is now accepted and ready to land.Aug 27 2017, 11:54 AM