This is an archive of the discontinued LLVM Phabricator instance.

Refactor DynamicLibrary so searching for a symbol will have a defined order and libraries are properly unloaded when llvm_shutdown is called.
ClosedPublic

Authored by marsupial on Feb 17 2017, 11:25 AM.

Details

Summary

This was mostly affecting usage of the JIT, where storing the library handles in
a set made iteration unordered/undefined. This lead to disagreement between the
JIT and native code as to what the address and implementation of particularly on
Windows with stdlib functions:

JIT: putenv_s("TEST", "VALUE") called msvcrt.dll, putenv_s
JIT: getenv("TEST") -> "VALUE"
called msvcrt.dll, getenv
Native: getenv("TEST") -> NULL // called ucrt.dll, getenv

Also fixed is the issue of DynamicLibrary::getPermanentLibrary(0,0) on Windows
not giving priority to the process' symbols as it did on Unix.

Diff Detail

Repository
rL LLVM

Event Timeline

marsupial created this revision.Feb 17 2017, 11:25 AM
marsupial edited subscribers, added: llvm-commits; removed: cfe-commits.Feb 17 2017, 11:42 AM
marsupial added a subscriber: cfe-commits.
marsupial removed a subscriber: cfe-commits.
marsupial edited the summary of this revision. (Show Details)Feb 19 2017, 5:52 PM
vsk added a subscriber: vsk.Feb 20 2017, 11:42 AM
vsk added a comment.Feb 20 2017, 3:03 PM

Some comments inline. Could you re-upload your diff with context ('git diff -U9999')?

lib/Support/Windows/DynamicLibrary.inc
38 ↗(On Diff #88930)

Please use ManagedStatic.

66 ↗(On Diff #88930)

Could you explain with some more detail why EnumProcessModulesEx needs to be repeated until its results don't change? Is there any guarantee that its results won't change again after this loop terminates?

Apologies if my Windows ignorance is showing here...

73 ↗(On Diff #88930)

At this point, I'd really expect OpenedHandles to be initialized.

106 ↗(On Diff #88930)

Try to avoid unrelated whitespace changes.

marsupial added inline comments.Feb 20 2017, 4:29 PM
lib/Support/Windows/DynamicLibrary.inc
38 ↗(On Diff #88930)

Are you sure about that?
This code already has a lock guarding it.
What does ManagedStatic add?

66 ↗(On Diff #88930)

There is a small chance of thread A calling EnumProcessModulesEx to get the size of the list and in between that and EnumProcessModulesEx to get the actual list that thread B does something to explicitly or implicitly load another dll.

73 ↗(On Diff #88930)

Current has yet to be swapped into OpenedHandles there (line 90).

106 ↗(On Diff #88930)

Ok, but there was a spurious indent there.

marsupial updated this revision to Diff 89161.Feb 20 2017, 10:16 PM

Wound up splitting most of it into a separate class. Predominately so ManagedStatic can reinitialize properly if necessary, but seems to provide a nicer separation and less code duplication when D29955 adds addPermanentLibrary

marsupial marked 8 inline comments as done.Feb 20 2017, 10:17 PM
marsupial updated this revision to Diff 89259.Feb 21 2017, 12:01 PM

The problem of undefined ordering was affecting Unix as well so moved to using an llvm::SetVector there.
Updated diff to latest changes.

marsupial updated this revision to Diff 89263.Feb 21 2017, 12:43 PM
marsupial retitled this revision from Make DynamicLibrary::getPermanentLibrary on Windows have a defined ordering. to Make DynamicLibrary::getPermanentLibrary have a defined ordering..
marsupial edited the summary of this revision. (Show Details)

Store and search for symbols the same way on Unix (in reverse and giving priority to the owning process' symbols).

marsupial edited the summary of this revision. (Show Details)Feb 21 2017, 8:18 PM
marsupial added reviewers: vsk, v.g.vassilev.
vsk added inline comments.Feb 22 2017, 2:28 PM
lib/Support/DynamicLibrary.cpp
55 ↗(On Diff #89263)

Maybe vector is the wrong container to use here, because it forces us to do O(n) work in one case. How about using a SmallPtrSet to do uniqueness testing, and a std::forward_list to preserve order?

57 ↗(On Diff #89263)

Instead of having a 'KeepFront' switch, do you think it might be more convenient to store the RTLD_DEFAULT/IsProcess handle separately from the rest of the handles?

107 ↗(On Diff #89263)

The convention is to write e.g '/*IsProcess=*/filename==nullptr', so it's clear which parameter you're passing in.

lib/Support/Windows/DynamicLibrary.inc
83 ↗(On Diff #89263)

I'm hoping there's a way to reduce code duplication between the windows/unix logic. Have you tried factoring out some of the generic logic in HandleSet s.t it can call into platform-specific stubs?

66 ↗(On Diff #88930)

If there isn't a guarantee that, e.g a third thread C couldn't load another dylib after this loop terminates, I don't think we should have the loop.

marsupial added inline comments.Feb 22 2017, 5:46 PM
lib/Support/DynamicLibrary.cpp
55 ↗(On Diff #89263)

I'm of the opinion that search will probably be fast enough and in the cases where it would't be using twice the memory is a worse penalty.

57 ↗(On Diff #89263)

Seems reasonable. Hopefully I haven't already tried that and there is a reason not...

lib/Support/Windows/DynamicLibrary.inc
83 ↗(On Diff #89263)

Yes there is a lot that can be reduced...just didn't want the patch to keep growing.
If this is the direction, I would probably add Unix/DynamicLibrary.inc and both the DynamicLibrary.inc files would be the implementation of HandleSet.

Leaving DynamicLibrary.cpp the common implementation of the public class.

66 ↗(On Diff #88930)

Generally I agree, but the cost of one compare that will most likely succeed to deal with an obscure event that would be hard to debug seems prudent?

marsupial added inline comments.Feb 22 2017, 6:04 PM
lib/Support/DynamicLibrary.cpp
57 ↗(On Diff #89263)

Ohh thats right..it complicates the iterator interface!

marsupial updated this revision to Diff 89569.Feb 23 2017, 2:46 PM

Refactor platform specific code.
Remove Windows caching of libraries.
Add tests.

marsupial added inline comments.Feb 23 2017, 2:54 PM
lib/Support/Windows/DynamicLibrary.inc
66 ↗(On Diff #88930)

Turns out the MSDN docs are pretty clear about not using the returned list if there has been a change. That is it may be possible for this to occur when [libA, libB, libC] are loaded:

Thread0: EnumProcessModulesEx: returns 3 for number of elements required
Thread0: EnumProcessModulesEx: called with HMODULE[3]
Thread1: FreeLibrary("libB")
Thread0: EnumProcessModulesEx: returns with [libA, libB, libC]

v.g.vassilev edited edge metadata.Mar 6 2017, 8:10 AM

It looks like this patch is doing a lot more than what is described in the summary. I think it would make the review easier if you split the patch. I'd imagine that fixing the order of iteration is fairly minimal change and I'd recommend to start from there by opening another review request with just that.

I was asked to make the changes that grew it in size.
There doesn't seem to be an -easy- way to fix the issues at hand for a couple of reasons.
Just fixing the iteration order by using rbegin and rend is not enough, as the storage in a set is also a culprit.
Just fixing the Windows portion, means differing (and still broken) behavior on Unix.
The remaining broken behaviour on Unix means that the test cannot be added.

Though I do agree that this has stalled and do not know how to rectify that...

vsk resigned from this revision.Mar 6 2017, 6:42 PM

I was asked to make the changes that grew it in size.

I did not mean to suggest you needed to tackle everything in one shot. As it stands, I don't have the bandwidth to review a patch of this size, and I won't hold up the review any longer.

marsupial updated this revision to Diff 92398.Mar 20 2017, 4:23 PM
davide resigned from this revision.Apr 9 2017, 3:31 PM

I don't feel qualified enough to review this part of the code, therefore I resign. Realistically, Lang (@lhames) is the only person who can sign-off this change so you need to wait for him.

lhames edited edge metadata.Apr 12 2017, 7:02 PM

Sorry - I was traveling for a bit and didn't have bandwidth for patch review. I'm slowly catching up.

Reading through this now.

Hi Frederich,

Thanks for your patience, and sorry for the long turn-around time on this review.

I had to bring this up to speed with the mainline to test it and while it passed make-check on MacOS I'm seeing test failures on linux:

llvm/unittests/Support/DynamicLibrary/DynamicLibraryTest.cpp:81: Failure
Value of: GS != nullptr && GS == &TestA

Actual: false

Expected: true
llvm/unittests/Support/DynamicLibrary/DynamicLibraryTest.cpp:82: Failure

Expected: StdString(GS())
Which is: "LibCall"

To be equal to: "ProcessCall"
llvm/unittests/Support/DynamicLibrary/DynamicLibraryTest.cpp:85: Failure
Value of: GS != nullptr && GS == &TestA

Actual: false

Expected: true
llvm/unittests/Support/DynamicLibrary/DynamicLibraryTest.cpp:86: Failure

Expected: StdString(GS())
Which is: "LibCall"

To be equal to: "ProcessCall"

Those could be genuine failures, or I could have missed something while updating the patch. Do you have access to a linux box to reproduce these?

Other than that, this all looks good to me. If we can get the tests passing it should be good to go in.

Cheers,
Lang.

lhames accepted this revision.Apr 19 2017, 6:24 PM

Ahh - I see the problem: Linux doesn't export symbols from the binary by default.

Adding

export_executable_symbols(DynamicLibraryTests)

to unittests/Support/DynamicLibrary/CMakeLists.txt fixes the issue.

Pending that change, this Looks Good To Me. Frederich - Commit away, or let me know if you'd like me to commit it on your behalf.

This revision is now accepted and ready to land.Apr 19 2017, 6:24 PM
marsupial updated this revision to Diff 96175.Apr 21 2017, 10:42 AM

Thanks.
I've added the line for CMake and rebased against master.
The changes from https://reviews.llvm.org/rL299203 seem unnecessary, but will wait a few days for a response on that.

This revision was automatically updated to reflect the committed changes.
marsupial updated this revision to Diff 96340.Apr 23 2017, 9:09 PM
marsupial edited the summary of this revision. (Show Details)

Fixes for warning on i686-mingw32 bot.

marsupial reopened this revision.Apr 23 2017, 9:14 PM

This is causing some problems on the mingw32 build, and I'm a bit out of my depth there.
Hopefully the only remaining issue is that EnumProcessModulesEx seems not to be is not available in ming (possibly only the 32 bit version).

Any thoughts as to why or how that can be addressed?
Thanks.

This revision is now accepted and ready to land.Apr 23 2017, 9:14 PM
marsupial updated this revision to Diff 96343.Apr 23 2017, 10:10 PM

Ming seems to have EnumProcessModules which will also work on Win32, so only use EnumProcessModulesEx on Win64 (where EnumProcessModules will fail!).

Could someone who uses ming32 test if this is working?
If no one steps up, what is the etiquette for using the bots to test?

vsk added a comment.Apr 24 2017, 10:47 AM

Ming seems to have EnumProcessModules which will also work on Win32, so only use EnumProcessModulesEx on Win64 (where EnumProcessModules will fail!).

Could someone who uses ming32 test if this is working?

You may be able to reach someone more effectively by pinging a mingw32 bot owner.

If no one steps up, what is the etiquette for using the bots to test?

It's OK to try fixing bots 'speculatively' for a short period of time. However it's best to avoid leaving CI in a bad state for an extended period of time. If there's no immediate fix in sight, I always revert (see llvm/utils/git-svn/git-svnrevert).

Ok, thanks.
I've been able to setup a separate CI that could at least validate the changes compile properly on Ming.
I'll try to land it once more and if it fails again search for someone who can contribute more info.

This revision was automatically updated to reflect the committed changes.
marsupial updated this revision to Diff 96473.Apr 24 2017, 2:47 PM
marsupial retitled this revision from Make DynamicLibrary::getPermanentLibrary have a defined ordering. to Refactor DynamicLibrary so searching for a symbol will have a defined order and libraries are properly unloaded when llvm_shutdown is called..
marsupial edited the summary of this revision. (Show Details)

Fix case sensitive include for mingw on Linux.

marsupial reopened this revision.Apr 24 2017, 2:47 PM

Still looking for a tester for mingw32 on Linux...
Thanks.

This revision is now accepted and ready to land.Apr 24 2017, 2:47 PM
chapuni accepted this revision.Apr 24 2017, 3:33 PM

<psapi.h> would be just fine. It is an issue of cross compiler on case-sensitive filesystem.
I haven't tested on windows, though.

This revision was automatically updated to reflect the committed changes.
efriedma added inline comments.
llvm/trunk/include/llvm/Support/DynamicLibrary.h
61

Is this change necessary? I don't see any discussion of this in the review.

In addition to being used by the JIT, this API is used to load plugins, and unloading a plugin during llvm_shutdown() can cause a segfault, depending on the order globals get destroyed.

marsupial added inline comments.May 12 2017, 12:15 PM
llvm/trunk/include/llvm/Support/DynamicLibrary.h
61

I can see the benefit of DynamicLibrary::HandleSet::~HandleSet iterating in reverse order, but having LLVM release memory when I explicitly tell it I'm not using LLVM anymore is important.

Not releasing / deallocating the libraries seems to be a violation of the documentation:
"When you are done using the LLVM APIs, you should call llvm_shutdown() to deallocate memory used for internal structures."

If there are issues with plugins wouldn't it better handled in the plugin interface with a way to signal shutdown will/is occurring.

efriedma added inline comments.May 18 2017, 1:27 PM
llvm/trunk/include/llvm/Support/DynamicLibrary.h
61

Adding an interface to notify plugins of an impending shutdown wouldn't help. The problem is that llvm_shutdown itself tries to call into code in the plugin, and there's no way to unregister those handlers. It might be possible to refactor a bunch of code to allow unloading a plugin without calling llvm_shutdown, but that would be complicated, for little benefit.

Maybe we could delay unloading shared libraries until other code like the pass manager finishes shutdown?

marsupial added inline comments.May 19 2017, 9:55 AM
llvm/trunk/include/llvm/Support/DynamicLibrary.h
61

Do you have an example of this?

llvm_shutdown destroys in reverse order of construction so I don't get how a plugin could register a ManagedStatic that would be destroyed before the library is unloaded.

efriedma added inline comments.May 19 2017, 12:26 PM
llvm/trunk/include/llvm/Support/DynamicLibrary.h
61

It's not a ManagedStatic in the plugin. The plugin has an llvm::RegisterStandardPasses in it, which calls PassManagerBuilder::addGlobalExtension, which sticks an std::function into the GlobalExtensions with a vtable in the plugin.

marsupial added inline comments.May 20 2017, 1:02 PM
llvm/trunk/include/llvm/Support/DynamicLibrary.h
61

PassManagerBuilder::addGlobalExtension has exactly one line of implementation, storing the passed arguments into a ManagedStatic. Perhaps the problem is a poor usage of that ManagedStatic in other places, forcing it's construction earlier than necessary.

https://reviews.llvm.org/D33381 addresses the possibility of that bad ordering.

efriedma added inline comments.May 22 2017, 4:57 PM
llvm/trunk/include/llvm/Support/DynamicLibrary.h
61

That doesn't help my problem: we also have uses of RegisterManagedStatic in statically linked code.

efriedma added inline comments.May 22 2017, 4:58 PM
llvm/trunk/include/llvm/Support/DynamicLibrary.h
61

Err, sorry, typo. Meant to say "we also have uses of llvm::RegisterStandardPasses in statically linked code".

We are doing an upgrade on our internal vendor drop of LLVM and clang.

This patch causes many failures, preventing us from running llvm without custom patches on top. For instance, the symbol resolution seems to be very different (it cannot find symbols which exist in a shared library). The patch is rather big and I am not sure whether it requires couple of changes or it requires a design change.

I think cling and ROOT are one of the major users of this code. @lhames would it make sense to revert this patch? I can confirm this is the only blocker for us to move on and reverting it restores our system into green state.

The author has access to cling and ROOT and maybe it would be easier for him to debug it in that context.

marsupial added inline comments.May 24 2017, 11:16 AM
llvm/trunk/include/llvm/Support/DynamicLibrary.h
61

I understand the general premise of what you are saying, but don't get the call sequence that is allowing a ManagedStatic from statically linked code to interfere with ones from dynamically loaded objects.

Is there/can you auothor a test that demonstrates the problem(s) you are seeing?

We are doing an upgrade on our internal vendor drop of LLVM and clang.

This patch causes many failures, preventing us from running llvm without custom patches on top. For instance, the symbol resolution seems to be very different (it cannot find symbols which exist in a shared library). The patch is rather big and I am not sure whether it requires couple of changes or it requires a design change.

I think cling and ROOT are one of the major users of this code. @lhames would it make sense to revert this patch? I can confirm this is the only blocker for us to move on and reverting it restores our system into green state.

The author has access to cling and ROOT and maybe it would be easier for him to debug it in that context.

Symbol resolution is purposely different, as prior library search order was totally undefined which lead to serious problems (for cling in particular), see the Summary.
Do you have more information or a test demonstrating the failure?

I'd really like to make sure that these issues aren't easily fixed or relying on prior behavior which was poorly defined before reversion.

efriedma added inline comments.May 24 2017, 11:55 AM
llvm/trunk/include/llvm/Support/DynamicLibrary.h
61

Use the following patch, make an LLVM build including polly (see http://polly.llvm.org/get_started.html), then run "bin/opt -load=lib/LLVMHello.so -S /dev/null".

diff --git a/lib/Transforms/Hello/Hello.cpp b/lib/Transforms/Hello/Hello.cpp
index 29b9bb8..d0a0b61 100644
--- a/lib/Transforms/Hello/Hello.cpp
+++ b/lib/Transforms/Hello/Hello.cpp
@@ -16,6 +16,9 @@
 #include "llvm/IR/Function.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/raw_ostream.h"
+#include "llvm/IR/LegacyPassManager.h"
+#include "llvm/Transforms/IPO/PassManagerBuilder.h"
+
 using namespace llvm;

 #define DEBUG_TYPE "hello"
@@ -63,3 +66,10 @@ namespace {
 char Hello2::ID = 0;
 static RegisterPass<Hello2>
 Y("hello2", "Hello World Pass (with getAnalysisUsage implemented)");
+
+static void registerHello(const llvm::PassManagerBuilder &Builder,
+                                    llvm::legacy::PassManagerBase &PM) {
+  PM.add(new Hello2);
+}
+static llvm::RegisterStandardPasses RegisterHello(llvm::PassManagerBuilder::EP_LoopOptimizerEnd,
+                                  registerHello);

Symbol resolution is purposely different, as prior library search order was totally undefined which lead to serious problems (for cling in particular), see the Summary.

Right, all I am saying is it doesn't work and asking to revert because it makes the issue much worse on cling/ROOT end. I think you are mostly having in mind Windows which cling doesn't officially support, however, this patch breaks Unix.

Do you have more information or a test demonstrating the failure?

I think you should be able to reproduce it if you apply this patch and start up ROOT...

I'd really like to make sure that these issues aren't easily fixed or relying on prior behavior which was poorly defined before reversion.

I'd support that. What about reverting it now while you address the underlying out issues without pressure. If this patch was meant to address a cling issue, I'd have been happier to discuss and test it, before landing it.

marsupial added inline comments.May 24 2017, 1:31 PM
llvm/trunk/include/llvm/Support/DynamicLibrary.h
61

Ughs..could have save so much time just asking for that.
This revision actually isn't responsible, for the bug, but did make it more obvious.

Anyway reproduced here...and the fix is here:
https://reviews.llvm.org/D33515.

efriedma added inline comments.May 24 2017, 1:47 PM
llvm/trunk/include/llvm/Support/DynamicLibrary.h
61

I tried that patch, but it still segfaults for me, in the same way, in
llvm::object_deleter<llvm::SmallVector<std::__1::pair<llvm::PassManagerBuilder::ExtensionPointTy, std::__1::function<void (llvm::PassManagerBuilder const&, llvm::legacy::PassManagerBase&)> >, 8u> >::call(void*) ()

marsupial added inline comments.May 24 2017, 1:55 PM
llvm/trunk/include/llvm/Support/DynamicLibrary.h
61

To be clear you've tried the revision created today?https://reviews.llvm.org/D33515

What are the test results from running
unittests/Support/DynamicLibrary/DynamicLibraryTests

Can we move the discussion into that revision as this is getting a bit long, and moving OpenedHandles into a ManagedStatic did not occur in this revision?

Symbol resolution is purposely different, as prior library search order was totally undefined which lead to serious problems (for cling in particular), see the Summary.

Right, all I am saying is it doesn't work and asking to revert because it makes the issue much worse on cling/ROOT end. I think you are mostly having in mind Windows which cling doesn't officially support, however, this patch breaks Unix.

Do you have more information or a test demonstrating the failure?

I think you should be able to reproduce it if you apply this patch and start up ROOT...

I'd really like to make sure that these issues aren't easily fixed or relying on prior behavior which was poorly defined before reversion.

I'd support that. What about reverting it now while you address the underlying out issues without pressure. If this patch was meant to address a cling issue, I'd have been happier to discuss and test it, before landing it.

If you could provide an example of failure I'd be happy to look at it.

I get you're saying it doesn't work, but the tests this patch added show it does work on Unix.
Additionally a simple test in cling works as well:
$ .L ./unittests/Support/DynamicLibrary/PipSqueak.so
$ extern "C" const char *TestA();
$ TestA()
(const char *) "LibCall"

Symbol resolution is purposely different, as prior library search order was totally undefined which lead to serious problems (for cling in particular), see the Summary.

Right, all I am saying is it doesn't work and asking to revert because it makes the issue much worse on cling/ROOT end. I think you are mostly having in mind Windows which cling doesn't officially support, however, this patch breaks Unix.

Do you have more information or a test demonstrating the failure?

I think you should be able to reproduce it if you apply this patch and start up ROOT...

I'd really like to make sure that these issues aren't easily fixed or relying on prior behavior which was poorly defined before reversion.

I'd support that. What about reverting it now while you address the underlying out issues without pressure. If this patch was meant to address a cling issue, I'd have been happier to discuss and test it, before landing it.

If you could provide an example of failure I'd be happy to look at it.

You should test it with ROOT, there we do a dlopen(libcling, RTLD_LAZY|RTLD_LOCAL) and I think there the symbol resolution doesn't work as before.

I get you're saying it doesn't work, but the tests this patch added show it does work on Unix.

It seems that the test coverage is low.

Additionally a simple test in cling works as well:
$ .L ./unittests/Support/DynamicLibrary/PipSqueak.so
$ extern "C" const char *TestA();
$ TestA()
(const char *) "LibCall"

You should test it with ROOT, there we do a dlopen(libcling, RTLD_LAZY|RTLD_LOCAL) and I think there the symbol resolution doesn't work as before.

RTLD_LOCAL means any symbols loaded will not be available for lookup.
If you are relying on symbol lookup via a process handle [ dlsym(dlopen(NULL, Flags), "Symbol") ] then you need to use RTLD_GLOBAL.

It seems that the test coverage is low.

Add as many more as you can.

Can the discussion of this continue here https://reviews.llvm.org/D33529.

pcanal added a subscriber: pcanal.May 26 2017, 3:28 AM

You should test it with ROOT, there we do a dlopen(libcling, RTLD_LAZY|RTLD_LOCAL) and I think there the symbol resolution doesn't work as before.

RTLD_LOCAL means any symbols loaded will not be available for lookup.
If you are relying on symbol lookup via a process handle [ dlsym(dlopen(NULL, Flags), "Symbol") ] then you need to use RTLD_GLOBAL.

For the same reasons that dlsym has two modes, the equivalent LLVM routine must have the same two modes.
(1) dlsym by process which does a search in load ordering
(2) dlsym by handle which does a search in dependency ordering

I understand from marsupial that (1) is more performance for his use case but (2) is the order that was supported before and should stay the default.

One other major difference is that doing (1) followed by (2) is not equivalent to doing just (2) as it prevents override symbols.

One could argue that there is even a 3rd option to be added, which is whether the RTLD_LOCAL are search (again the legacy behavior and new default should be to search those).

As is the patch is introducing a (weakly or not) documented change in behavior and worse is preventing a return to the old (useful) behavior.

Cheers,
Philippe.

Why does https://reviews.llvm.org/D33529 not address your concerns?

Why does https://reviews.llvm.org/D33529 not address your concerns?

If I read correctly it always does
(1)
and depending on the argument does also (2)

which is functionally different from being able to do (2) then (1).

I.e. the patches forces the 'load-order search'.

It did neither (1) or (2) before, because the order the handles were stored was not defined.
This was particularly noticeable on Windows due to how it uses shim libs for runtime functions.
I assume you guys aren't arguing to going back to undefined behavior just because it worked better for you.

This patch explicitly states that symbol lookup is now done in the following order:
Process,
Libraries loaded from OS linker at runtime.
Libraries loaded from user at runtime.

How is this bad overall or in your use case in particular?

Lets take a simple example of libRuntime.so, libA.so, libB.so.
libRuntime.so is linked to the binaries and contains stdlib functions, such as printf.
libA.so, and libB.so also contain a locally exported symbols printf, doSomething

DynamicLibrary::getPermanentLibrary(NULL)
DynamicLibrary::getPermanentLibrary("libA.so")
X = DynamicLibrary::SearchForAddressOfSymbol("doSomething")
DynamicLibrary::getPermanentLibrary("libB.so")
Y = DynamicLibrary::SearchForAddressOfSymbol("doSomething")
Z = DynamicLibrary::SearchForAddressOfSymbol("printf")

Prior to this patch X was the only defined behavior, guaranteed to be from libA.so
Y, and Z could be resolved from any of libRuntime.so, libA.so, libB.so and not necessarily consistent across launches.

Now:
X always comes from libA.so
Y always comes from libB.so
Z always comes from libRuntime.so

All this is fine, what we would like is the if (Process) part to become a last resource resolution step. Currently this is either we resolve from the process or we iterate over the handles.

https://reviews.llvm.org/D33529 is attempting to address this can we continue discussion there?
I'm totally understand the order being reversed, though I would like to keep an explicit bool to signify Don't resolve as the OS would
Otherwise from the example above someone who loads a lib with printf will pull that symbol from libA.so or libB.so and they might not actually want to.
They should explicitly request this behaviour.

From what I see the OS resolves the symbol dependent on the position of the request. If the request comes from a shared library it has to start looking for the symbols local to the library first. I don't think we can get the position of the request so in order to get closer behavior (avoiding to specialcase the dlopen flags) we should put the executable as a last resort... This would also allow the shared libraries to overload weak symbols...

I assume you guys aren't arguing to going back to undefined behavior just because it worked better for you.

Indeed we are not. We are arguing the current set of patch (and proposed) patch imposed the order (1) then (maybe) (2) while we need (and the previous behavior was 'close' to) (2) then (1)

I'm totally understand the order being reversed, though I would like to keep an explicit bool

Yes, I think we *must* have such an option.

Now
Z always comes from libRuntime.so

Exactly, so there is no way to override the symbol at run-time. This counter-intuitive (well wrong in my opinion) in an interactive-compiler.

You can think of the cling-transaction being loaded as a (semantic) shared library that is explicitly linked against all the already loaded libraries. In such a case the OS would first search the library and then all the library it depends on (i.e is linked against) in reverse order and then finally the process.

Indeed we are not. We are arguing the current set of patch (and proposed) patch imposed the order (1) then (maybe) (2) while we need (and the previous behavior was 'close' to) (2) then (1)

Why does https://reviews.llvm.org/D33529 not address your concerns?

Exactly, so there is no way to override the symbol at run-time. This counter-intuitive (well wrong in my opinion) in an interactive-compiler.

I understand you folks feel it is wrong and I have been trying to work towards a solution.
But there are other use cases besides yours.

The problem I have and why I think it is not well wrong:
libc++.so, libA.so
libA.so defines an overload for global operator new/delete

DynamicLibrary::getPermanentLibrary(NULL)
// Other code
DynamicLibrary::getPermanentLibrary("libA.so")

In your scheme the JIT can pull operator new/delete from libA.so, and worse is that whether it pulls operator new/delete from libA.so is dependent on whether operator new/delete has been resolved in // Other code before libA.so has been loaded.

While not undefined behavior, in the setting of // Other code involving an interactive-compiler it could easily lead to behavior/bugs that would be hard to understand/debug.
By making order follow what the OS would do as much as possible by default this problem can be somewhat mitigated.

And can we please continue at https://reviews.llvm.org/D33529 which is devoted to this issue.

The new version looks promising. We are going to give a try. Thanks.

But there are other use cases besides yours.

yes, of course, I agree :)

llvm/trunk/lib/Support/DynamicLibrary.cpp