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

[marsupial]

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.

[vsk]

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]

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]

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]

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

[marsupial]

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

[vsk]

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]

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]

lib/Support/DynamicLibrary.cpp

57 ↗

(On Diff #89263)

Ohh thats right..it complicates the iterator interface!

[marsupial]

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

[marsupial]

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]

[marsupial]

Ping.

[v.g.vassilev]

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.

[marsupial]

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]

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]

ping

[davide]

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]

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

Reading through this now.

[lhames]

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]

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.

[marsupial]

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.

[marsupial]

Fixes for warning on i686-mingw32 bot.

[marsupial]

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.

[marsupial]

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]

In D30107#735022, @marsupial wrote:

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).

[marsupial]

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.

[marsupial]

Fix case sensitive include for mingw on Linux.

[marsupial]

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

[chapuni]

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

[efriedma]

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]

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]

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]

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]

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]

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]

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]

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".

[v.g.vassilev]

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]

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?

[marsupial]

In D30107#762698, @v.g.vassilev wrote:

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]

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);

[v.g.vassilev]

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]

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]

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]

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?

[marsupial]

In D30107#763741, @v.g.vassilev wrote:

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"

[v.g.vassilev]

In D30107#763811, @marsupial wrote:

In D30107#763741, @v.g.vassilev wrote:

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"

[marsupial]

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.

[marsupial]

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

[pcanal]

In D30107#763829, @marsupial wrote:

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.

[marsupial]

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

[pcanal]

In D30107#765751, @marsupial wrote:

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'.

[marsupial]

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?

[marsupial]

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

[v.g.vassilev]

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.

[marsupial]

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.

[v.g.vassilev]

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...

[pcanal]

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.

[marsupial]

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?

[marsupial]

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.

[pcanal]

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/include/llvm/Support/DynamicLibrary.h

public:
     /// found, the address of that symbol is returned. If not, NULL is returned.
     /// Note that NULL will also be returned if the library failed to load.
     /// Use isValid() to distinguish these cases if it is important.
     /// Note that this will \e not search symbols explicitly registered by
     /// AddSymbol().
     void *getAddressOfSymbol(const char *symbolName);
 
     /// This function permanently loads the dynamic library at the given path.
-    /// The library will only be unloaded when the program terminates.
+    /// The library will only be unloaded when llvm_shutdown() is called.
     /// This returns a valid DynamicLibrary instance on success and an invalid
     /// instance on failure (see isValid()). \p *errMsg will only be modified
     /// if the library fails to load.
     ///
     /// It is safe to call this function multiple times for the same library.
     /// @brief Open a dynamic library permanently.
     static DynamicLibrary getPermanentLibrary(const char *filename,
                                               std::string *errMsg = nullptr);
 
     /// Registers an externally loaded library. The library will be unloaded
     /// when the program terminates.
     ///
-    /// It is safe to call this function multiple times for the same library.
+    /// It is safe to call this function multiple times for the same library,
+    /// though ownership is only taken if there was no error.
     ///
     /// \returns An empty \p DynamicLibrary if the library was already loaded.
     static DynamicLibrary addPermanentLibrary(void *handle,
                                               std::string *errMsg = nullptr);
 
     /// This function permanently loads the dynamic library at the given path.
     /// Use this instead of getPermanentLibrary() when you won't need to get
     /// symbols from the library itself.
static void *SearchForAddressOfSymbol(const std::string &symbolName) {
       return SearchForAddressOfSymbol(symbolName.c_str());
     }
 
     /// This functions permanently adds the symbol \p symbolName with the
     /// value \p symbolValue.  These symbols are searched before any
     /// libraries.
     /// @brief Add searchable symbol/value pair.
     static void AddSymbol(StringRef symbolName, void *symbolValue);
+
+    class HandleSet;
   };
 
 } // End sys namespace
 } // End llvm namespace
 
 #endif

llvm/trunk/lib/Support/CMakeLists.txt

# System
   Errno.cpp
   Host.cpp
   Memory.cpp
   Mutex.cpp
   Path.cpp
   Process.cpp
   Program.cpp
   RWMutex.cpp
-  SearchForAddressOfSpecialSymbol.cpp
   Signals.cpp
   TargetRegistry.cpp
   ThreadLocal.cpp
   Threading.cpp
   Valgrind.cpp
   Watchdog.cpp
 
   ADDITIONAL_HEADER_DIRS

llvm/trunk/lib/Support/DynamicLibrary.cpp

 #include "llvm-c/Support.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Config/config.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/Mutex.h"
 #include <cstdio>
 #include <cstring>
+#include <vector>
 
+using namespace llvm;
+using namespace llvm::sys;
+
+// All methods for HandleSet should be used holding SymbolsMutex.
+class DynamicLibrary::HandleSet {
+  typedef std::vector<void *> HandleList;
+  HandleList Handles;
+  void *Process;
+
+public:
+  static void *DLOpen(const char *Filename, std::string *Err);
+  static void DLClose(void *Handle);
+  static void *DLSym(void *Handle, const char *Symbol);
+
+  HandleSet() : Process(nullptr) {}
+  ~HandleSet();
+
+  HandleList::iterator Find(void *Handle) {
+    return std::find(Handles.begin(), Handles.end(), Handle);
+  }
+
+  bool Contains(void *Handle) {
+    return Handle == Process || Find(Handle) != Handles.end();
+  }
+
+  bool AddLibrary(void *Handle, bool IsProcess = false, bool CanClose = true) {
+#ifdef LLVM_ON_WIN32
+    assert((Handle == this ? IsProcess : !IsProcess) && "Bad Handle.");
+#endif
+
+    if (LLVM_LIKELY(!IsProcess)) {
+      if (Find(Handle) != Handles.end()) {
+        if (CanClose)
+          DLClose(Handle);
+        return false;
+      }
+      Handles.push_back(Handle);
+    } else {
+#ifndef LLVM_ON_WIN32
+      if (Process) {
+        if (CanClose)
+          DLClose(Process);
+        if (Process == Handle)
+          return false;
+      }
+#endif
+      Process = Handle;
+    }
+    return true;
+  }
+
+  void *Lookup(const char *Symbol) {
+    // Process handle gets first try.
+    if (Process) {
+      if (void *Ptr = DLSym(Process, Symbol))
+        return Ptr;
+#ifndef NDEBUG
+      for (void *Handle : Handles)
+        assert(!DLSym(Handle, Symbol) && "Symbol exists in non process handle");
+#endif
+    } else {
+      // Iterate in reverse, so newer libraries/symbols override older.
+      for (auto &&I = Handles.rbegin(), E = Handles.rend(); I != E; ++I) {
+        if (void *Ptr = DLSym(*I, Symbol))
+          return Ptr;
+      }
+    }
+    return nullptr;
+  }
+};
+
+namespace {
 // Collection of symbol name/value pairs to be searched prior to any libraries.
-static llvm::ManagedStatic<llvm::StringMap<void *> > ExplicitSymbols;
-static llvm::ManagedStatic<llvm::sys::SmartMutex<true> > SymbolsMutex;
-
-void llvm::sys::DynamicLibrary::AddSymbol(StringRef symbolName,
-                                          void *symbolValue) {
-  SmartScopedLock<true> lock(*SymbolsMutex);
-  (*ExplicitSymbols)[symbolName] = symbolValue;
+static llvm::ManagedStatic<llvm::StringMap<void *>> ExplicitSymbols;
+// Collection of known library handles.
+static llvm::ManagedStatic<DynamicLibrary::HandleSet> OpenedHandles;
+// Lock for ExplicitSymbols and OpenedHandles.
+static llvm::ManagedStatic<llvm::sys::SmartMutex<true>> SymbolsMutex;
 }
 
-char llvm::sys::DynamicLibrary::Invalid = 0;
-
 #ifdef LLVM_ON_WIN32
 
 #include "Windows/DynamicLibrary.inc"
 
 #else
 
-#if defined(HAVE_DLFCN_H) && defined(HAVE_DLOPEN)
-#include <dlfcn.h>
-using namespace llvm;
-using namespace llvm::sys;
-
-//===----------------------------------------------------------------------===//
-//=== WARNING: Implementation here must contain only TRULY operating system
-//===          independent code.
-//===----------------------------------------------------------------------===//
-
-static llvm::ManagedStatic<DenseSet<void *> > OpenedHandles;
+#include "Unix/DynamicLibrary.inc"
 
-DynamicLibrary DynamicLibrary::getPermanentLibrary(const char *filename,
-                                                   std::string *errMsg) {
-  SmartScopedLock<true> lock(*SymbolsMutex);
-
-  void *handle = dlopen(filename, RTLD_LAZY|RTLD_GLOBAL);
-  if (!handle) {
-    if (errMsg) *errMsg = dlerror();
-    return DynamicLibrary();
-  }
-
-#ifdef __CYGWIN__
-  // Cygwin searches symbols only in the main
-  // with the handle of dlopen(NULL, RTLD_GLOBAL).
-  if (!filename)
-    handle = RTLD_DEFAULT;
 #endif
 
-  // If we've already loaded this library, dlclose() the handle in order to
-  // keep the internal refcount at +1.
-  if (!OpenedHandles->insert(handle).second)
-    dlclose(handle);
-
-  return DynamicLibrary(handle);
-}
+char DynamicLibrary::Invalid;
 
-DynamicLibrary DynamicLibrary::addPermanentLibrary(void *handle,
-                                                   std::string *errMsg) {
-  SmartScopedLock<true> lock(*SymbolsMutex);
-  // If we've already loaded this library, tell the caller.
-  if (!OpenedHandles->insert(handle).second) {
-    if (errMsg) *errMsg = "Library already loaded";
-    return DynamicLibrary();
-  }
-
-  return DynamicLibrary(handle);
+namespace llvm {
+void *SearchForAddressOfSpecialSymbol(const char *SymbolName) {
+  return DoSearch(SymbolName); // DynamicLibrary.inc
 }
 
-void *DynamicLibrary::getAddressOfSymbol(const char *symbolName) {
-  if (!isValid())
-    return nullptr;
-  return dlsym(Data, symbolName);
+void DynamicLibrary::AddSymbol(StringRef SymbolName, void *SymbolValue) {
+  SmartScopedLock<true> Lock(*SymbolsMutex);
+  (*ExplicitSymbols)[SymbolName] = SymbolValue;
 }
 
-#else
-
-using namespace llvm;
-using namespace llvm::sys;
+DynamicLibrary DynamicLibrary::getPermanentLibrary(const char *FileName,
+                                                   std::string *Err) {
+  SmartScopedLock<true> Lock(*SymbolsMutex);
+  void *Handle = HandleSet::DLOpen(FileName, Err);
+  if (Handle != &Invalid)
+    OpenedHandles->AddLibrary(Handle, /*IsProcess*/ FileName == nullptr);
 
-DynamicLibrary DynamicLibrary::getPermanentLibrary(const char *filename,
-                                                   std::string *errMsg) {
-  if (errMsg) *errMsg = "dlopen() not supported on this platform";
-  return DynamicLibrary();
+  return DynamicLibrary(Handle);
 }
 
-void *DynamicLibrary::getAddressOfSymbol(const char *symbolName) {
-  return NULL;
-}
+DynamicLibrary DynamicLibrary::addPermanentLibrary(void *Handle,
+                                                   std::string *Err) {
+  SmartScopedLock<true> Lock(*SymbolsMutex);
+  // If we've already loaded this library, tell the caller.
+  if (!OpenedHandles->AddLibrary(Handle, /*IsProcess*/false, /*CanClose*/false))
+    *Err = "Library already loaded";
 
-#endif
+  return DynamicLibrary(Handle);
+}
 
-namespace llvm {
-void *SearchForAddressOfSpecialSymbol(const char* symbolName);
+void *DynamicLibrary::getAddressOfSymbol(const char *SymbolName) {
+  if (!isValid())
+    return nullptr;
+  return HandleSet::DLSym(Data, SymbolName);
 }
 
-void* DynamicLibrary::SearchForAddressOfSymbol(const char *symbolName) {
+void *DynamicLibrary::SearchForAddressOfSymbol(const char *SymbolName) {
+  {
     SmartScopedLock<true> Lock(*SymbolsMutex);
 
     // First check symbols added via AddSymbol().
     if (ExplicitSymbols.isConstructed()) {
-    StringMap<void *>::iterator i = ExplicitSymbols->find(symbolName);
+      StringMap<void *>::iterator i = ExplicitSymbols->find(SymbolName);
 
       if (i != ExplicitSymbols->end())
         return i->second;
     }
 
-#if defined(HAVE_DLFCN_H) && defined(HAVE_DLOPEN)
     // Now search the libraries.
     if (OpenedHandles.isConstructed()) {
-    for (DenseSet<void *>::iterator I = OpenedHandles->begin(),
-         E = OpenedHandles->end(); I != E; ++I) {
-      //lt_ptr ptr = lt_dlsym(*I, symbolName);
-      void *ptr = dlsym(*I, symbolName);
-      if (ptr) {
-        return ptr;
-      }
-    }
-  }
-#endif
+      if (void *Ptr = OpenedHandles->Lookup(SymbolName))
+        return Ptr;
 
-  if (void *Result = llvm::SearchForAddressOfSpecialSymbol(symbolName))
-    return Result;
-
-// This macro returns the address of a well-known, explicit symbol
-#define EXPLICIT_SYMBOL(SYM) \
-   if (!strcmp(symbolName, #SYM)) return &SYM
-
-// On linux we have a weird situation. The stderr/out/in symbols are both
-// macros and global variables because of standards requirements. So, we
-// boldly use the EXPLICIT_SYMBOL macro without checking for a #define first.
-#if defined(__linux__) and !defined(__ANDROID__)
-  {
-    EXPLICIT_SYMBOL(stderr);
-    EXPLICIT_SYMBOL(stdout);
-    EXPLICIT_SYMBOL(stdin);
-  }
-#else
-  // For everything else, we want to check to make sure the symbol isn't defined
-  // as a macro before using EXPLICIT_SYMBOL.
-  {
-#ifndef stdin
-    EXPLICIT_SYMBOL(stdin);
-#endif
-#ifndef stdout
-    EXPLICIT_SYMBOL(stdout);
-#endif
-#ifndef stderr
-    EXPLICIT_SYMBOL(stderr);
-#endif
-  }
-#endif
-#undef EXPLICIT_SYMBOL
-
-  return nullptr;
-}
-
-#endif // LLVM_ON_WIN32
+  return llvm::SearchForAddressOfSpecialSymbol(SymbolName);
 
 //===----------------------------------------------------------------------===//
 // C API.
 //===----------------------------------------------------------------------===//
 
-LLVMBool LLVMLoadLibraryPermanently(const char* Filename) {
+LLVMBool LLVMLoadLibraryPermanently(const char *Filename) {
   return llvm::sys::DynamicLibrary::LoadLibraryPermanently(Filename);
 }
 
 void *LLVMSearchForAddressOfSymbol(const char *symbolName) {
   return llvm::sys::DynamicLibrary::SearchForAddressOfSymbol(symbolName);
 }
 
 void LLVMAddSymbol(const char *symbolName, void *symbolValue) {
   return llvm::sys::DynamicLibrary::AddSymbol(symbolName, symbolValue);
 }

llvm/trunk/lib/Support/SearchForAddressOfSpecialSymbol.cpp

-//===- SearchForAddressOfSpecialSymbol.cpp - Function addresses -*- C++ -*-===//
-//
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-//
-//  This file pulls the addresses of certain symbols out of the linker.  It must
-//  include as few header files as possible because it declares the symbols as
-//  void*, which would conflict with the actual symbol type if any header
-//  declared it.
-//
-//===----------------------------------------------------------------------===//
-
-#include <string.h>
-
-// Must declare the symbols in the global namespace.
-static void *DoSearch(const char* symbolName) {
-#define EXPLICIT_SYMBOL(SYM) \
-   extern void *SYM; if (!strcmp(symbolName, #SYM)) return &SYM
-
-  // If this is darwin, it has some funky issues, try to solve them here.  Some
-  // important symbols are marked 'private external' which doesn't allow
-  // SearchForAddressOfSymbol to find them.  As such, we special case them here,
-  // there is only a small handful of them.
-
-#ifdef __APPLE__
-  {
-    // __eprintf is sometimes used for assert() handling on x86.
-    //
-    // FIXME: Currently disabled when using Clang, as we don't always have our
-    // runtime support libraries available.
-#ifndef __clang__
-#ifdef __i386__
-    EXPLICIT_SYMBOL(__eprintf);
-#endif
-#endif
-  }
-#endif
-
-#ifdef __CYGWIN__
-  {
-    EXPLICIT_SYMBOL(_alloca);
-    EXPLICIT_SYMBOL(__main);
-  }
-#endif
-
-#undef EXPLICIT_SYMBOL
-  return nullptr;
-}
-
-namespace llvm {
-void *SearchForAddressOfSpecialSymbol(const char* symbolName) {
-  return DoSearch(symbolName);
-}
-}  // namespace llvm

llvm/trunk/lib/Support/Unix/DynamicLibrary.inc

+//===- Unix/DynamicLibrary.cpp - Unix DL Implementation ---------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file provides the UNIX specific implementation of DynamicLibrary.
+//
+//===----------------------------------------------------------------------===//
+
+#if defined(HAVE_DLFCN_H) && defined(HAVE_DLOPEN)
+#include <dlfcn.h>
+
+DynamicLibrary::HandleSet::~HandleSet() {
+  for (void *Handle : Handles)
+    ::dlclose(Handle);
+  if (Process)
+    ::dlclose(Process);
+}
+
+void *DynamicLibrary::HandleSet::DLOpen(const char *File, std::string *Err) {
+  void *Handle = ::dlopen(File, RTLD_LAZY|RTLD_GLOBAL);
+  if (!Handle) {
+    if (Err) *Err = ::dlerror();
+    return &DynamicLibrary::Invalid;
+  }
+
+#ifdef __CYGWIN__
+  // Cygwin searches symbols only in the main
+  // with the handle of dlopen(NULL, RTLD_GLOBAL).
+  if (!Filename)
+    Handle = RTLD_DEFAULT;
+#endif
+
+  return Handle;
+}
+
+void DynamicLibrary::HandleSet::DLClose(void *Handle) {
+  ::dlclose(Handle);
+}
+
+void *DynamicLibrary::HandleSet::DLSym(void *Handle, const char *Symbol) {
+  return ::dlsym(Handle, Symbol);
+}
+
+#else // !HAVE_DLOPEN
+
+DynamicLibrary::HandleSet::~HandleSet() {}
+
+void *DynamicLibrary::HandleSet::DLOpen(const char *File, std::string *Err) {
+  if (Err) *Err = "dlopen() not supported on this platform";
+  return &Invalid;
+}
+
+void DynamicLibrary::HandleSet::DLClose(void *Handle) {
+}
+
+void *DynamicLibrary::HandleSet::DLSym(void *Handle, const char *Symbol) {
+  return nullptr;
+}
+
+#endif
+
+// Must declare the symbols in the global namespace.
+static void *DoSearch(const char* SymbolName) {
+#define EXPLICIT_SYMBOL(SYM) \
+   extern void *SYM; if (!strcmp(SymbolName, #SYM)) return &SYM
+
+  // If this is darwin, it has some funky issues, try to solve them here.  Some
+  // important symbols are marked 'private external' which doesn't allow
+  // SearchForAddressOfSymbol to find them.  As such, we special case them here,
+  // there is only a small handful of them.
+
+#ifdef __APPLE__
+  {
+    // __eprintf is sometimes used for assert() handling on x86.
+    //
+    // FIXME: Currently disabled when using Clang, as we don't always have our
+    // runtime support libraries available.
+#ifndef __clang__
+#ifdef __i386__
+    EXPLICIT_SYMBOL(__eprintf);
+#endif
+#endif
+  }
+#endif
+
+#ifdef __CYGWIN__
+  {
+    EXPLICIT_SYMBOL(_alloca);
+    EXPLICIT_SYMBOL(__main);
+  }
+#endif
+
+#undef EXPLICIT_SYMBOL
+
+// This macro returns the address of a well-known, explicit symbol
+#define EXPLICIT_SYMBOL(SYM) \
+   if (!strcmp(SymbolName, #SYM)) return &SYM
+
+// On linux we have a weird situation. The stderr/out/in symbols are both
+// macros and global variables because of standards requirements. So, we
+// boldly use the EXPLICIT_SYMBOL macro without checking for a #define first.
+#if defined(__linux__) and !defined(__ANDROID__)
+  {
+    EXPLICIT_SYMBOL(stderr);
+    EXPLICIT_SYMBOL(stdout);
+    EXPLICIT_SYMBOL(stdin);
+  }
+#else
+  // For everything else, we want to check to make sure the symbol isn't defined
+  // as a macro before using EXPLICIT_SYMBOL.
+  {
+#ifndef stdin
+    EXPLICIT_SYMBOL(stdin);
+#endif
+#ifndef stdout
+    EXPLICIT_SYMBOL(stdout);
+#endif
+#ifndef stderr
+    EXPLICIT_SYMBOL(stderr);
+#endif
+  }
+#endif
+#undef EXPLICIT_SYMBOL
+
+  return nullptr;
+}

llvm/trunk/lib/Support/Windows/DynamicLibrary.inc

 //===- Win32/DynamicLibrary.cpp - Win32 DL Implementation -------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file provides the Win32 specific implementation of DynamicLibrary.
 //
 //===----------------------------------------------------------------------===//
 
 #include "WindowsSupport.h"
+#include "llvm/Support/raw_ostream.h"
 
-#ifdef __MINGW32__
- #include <imagehlp.h>
-#else
- #include <dbghelp.h>
-#endif
-
-#ifdef _MSC_VER
- #include <ntverp.h>
-#endif
-
-namespace llvm {
+#include <psapi.h>
 
 //===----------------------------------------------------------------------===//
 //=== WARNING: Implementation here must contain only Win32 specific code
 //===          and must not be UNIX code.
 //===----------------------------------------------------------------------===//
 
-typedef BOOL (WINAPI *fpEnumerateLoadedModules)(HANDLE,PENUMLOADED_MODULES_CALLBACK64,PVOID);
-static fpEnumerateLoadedModules fEnumerateLoadedModules;
-static llvm::ManagedStatic<DenseSet<HMODULE> > OpenedHandles;
 
-static bool loadDebugHelp(void) {
-  HMODULE hLib = ::LoadLibraryW(L"Dbghelp.dll");
-  if (hLib) {
-    fEnumerateLoadedModules = (fpEnumerateLoadedModules)
-      ::GetProcAddress(hLib, "EnumerateLoadedModules64");
-  }
-  return fEnumerateLoadedModules != 0;
-}
+DynamicLibrary::HandleSet::~HandleSet() {
+  for (void *Handle : Handles)
+    FreeLibrary(HMODULE(Handle));
+
+  // 'Process' should not be released on Windows.
+  assert((!Process || Process==this) && "Bad Handle");
+}
+
+void *DynamicLibrary::HandleSet::DLOpen(const char *File, std::string *Err) {
+  // Create the instance and return it to be the *Process* handle
+  // simillar to dlopen(NULL, RTLD_LAZY|RTLD_GLOBAL)
+  if (!File)
+    return &(*OpenedHandles);
 
-static BOOL CALLBACK
-ELM_Callback(PCSTR ModuleName, DWORD64 ModuleBase,
-             ULONG ModuleSize, PVOID UserContext) {
-  OpenedHandles->insert((HMODULE)ModuleBase);
-  return TRUE;
+  SmallVector<wchar_t, MAX_PATH> FileUnicode;
+  if (std::error_code ec = windows::UTF8ToUTF16(File, FileUnicode)) {
+    SetLastError(ec.value());
+    MakeErrMsg(Err, std::string(File) + ": Can't convert to UTF-16");
+    return &DynamicLibrary::Invalid;
   }
 
-sys::DynamicLibrary
-sys::DynamicLibrary::getPermanentLibrary(const char *filename,
-                                         std::string *errMsg) {
-  SmartScopedLock<true> lock(*SymbolsMutex);
-
-  if (!filename) {
-    // When no file is specified, enumerate all DLLs and EXEs in the process.
-    if (!fEnumerateLoadedModules) {
-      if (!loadDebugHelp()) {
-        assert(false && "These APIs should always be available");
-        return DynamicLibrary();
-      }
-    }
+  HMODULE Handle = LoadLibraryW(FileUnicode.data());
+  if (Handle == NULL) {
+    MakeErrMsg(Err, std::string(File) + ": Can't open");
+    return &DynamicLibrary::Invalid;
 
-    fEnumerateLoadedModules(GetCurrentProcess(), ELM_Callback, 0);
-    // Dummy library that represents "search all handles".
-    // This is mostly to ensure that the return value still shows up as "valid".
-    return DynamicLibrary(&OpenedHandles);
-  }
+  return reinterpret_cast<void*>(Handle);
 
-  SmallVector<wchar_t, MAX_PATH> filenameUnicode;
-  if (std::error_code ec = windows::UTF8ToUTF16(filename, filenameUnicode)) {
-    SetLastError(ec.value());
-    MakeErrMsg(errMsg, std::string(filename) + ": Can't convert to UTF-16");
-    return DynamicLibrary();
-  }
+static DynamicLibrary::HandleSet *IsOpenedHandlesInstance(void *Handle) {
+  if (!OpenedHandles.isConstructed())
+    return nullptr;
+  DynamicLibrary::HandleSet &Inst = *OpenedHandles;
+  return Handle == &Inst ? &Inst : nullptr;
 
-  HMODULE a_handle = LoadLibraryW(filenameUnicode.data());
-
-  if (a_handle == 0) {
-    MakeErrMsg(errMsg, std::string(filename) + ": Can't open");
-    return DynamicLibrary();
-  }
+void DynamicLibrary::HandleSet::DLClose(void *Handle) {
+  if (HandleSet* HS = IsOpenedHandlesInstance(Handle))
+    HS->Process = nullptr; // Just drop the *Process* handle.
+  else
+    FreeLibrary((HMODULE)Handle);
 
-  // If we've already loaded this library, FreeLibrary() the handle in order to
-  // keep the internal refcount at +1.
-  if (!OpenedHandles->insert(a_handle).second)
-    FreeLibrary(a_handle);
+static bool GetProcessModules(HANDLE H, DWORD &Bytes, HMODULE *Data = nullptr) {
+  // EnumProcessModules will fail on Windows 64 while some versions of
+  // MingW-32 don't have EnumProcessModulesEx.
+  if (
+#ifdef _WIN64
+      !EnumProcessModulesEx(H, Data, Bytes, &Bytes, LIST_MODULES_64BIT)
+#else
+      !EnumProcessModules(H, Data, Bytes, &Bytes)
+#endif
+     ) {
+    std::string Err;
+    if (MakeErrMsg(&Err, "EnumProcessModules failure"))
+      llvm::errs() << Err << "\n";
+    return false;
+  }
+  return true;
+}
+
+void *DynamicLibrary::HandleSet::DLSym(void *Handle, const char *Symbol) {
+  HandleSet* HS = IsOpenedHandlesInstance(Handle);
+  if (!HS)
+    return (void *)uintptr_t(GetProcAddress((HMODULE)Handle, Symbol));
+
+  // Could have done a dlclose on the *Process* handle
+  if (!HS->Process)
+    return nullptr;
 
-  return DynamicLibrary(a_handle);
+  // Trials indicate EnumProcessModulesEx is consistantly faster than using
+  // EnumerateLoadedModules64 or CreateToolhelp32Snapshot.
+  //
+  // | Handles | DbgHelp.dll | CreateSnapshot | EnumProcessModulesEx
+  // |=========|=============|========================================
+  // | 37      | 0.0000585 * | 0.0003031      | 0.0000152
+  // | 1020    | 0.0026310 * | 0.0121598      | 0.0002683
+  // | 2084    | 0.0149418 * | 0.0369936      | 0.0005610
+  //
+  // * Not including the load time of Dbghelp.dll (~.005 sec)
+  //
+  // There's still a case to somehow cache the result of EnumProcessModulesEx
+  // across invocations, but the complication of doing that properly...
+  // Possibly using LdrRegisterDllNotification to invalidate the cache?
+
+  DWORD Bytes = 0;
+  HMODULE Self = HMODULE(GetCurrentProcess());
+  if (!GetProcessModules(Self, Bytes))
+    return nullptr;
+
+  // Get the most recent list in case any modules added/removed between calls
+  // to EnumProcessModulesEx that gets the amount of, then copies the HMODULES.
+  // MSDN is pretty clear that if the module list changes during the call to
+  // EnumProcessModulesEx the results should not be used.
+  std::vector<HMODULE> Handles;
+  do {
+    assert(Bytes && ((Bytes % sizeof(HMODULE)) == 0) &&
+           "Should have at least one module and be aligned");
+    Handles.resize(Bytes / sizeof(HMODULE));
+    if (!GetProcessModules(Self, Bytes, Handles.data()))
+      return nullptr;
+  } while (Bytes != (Handles.size() * sizeof(HMODULE)));
+
+  // Try EXE first, mirroring what dlsym(dlopen(NULL)) does.
+  if (FARPROC Ptr = GetProcAddress(HMODULE(Handles.front()), Symbol))
+    return (void *) uintptr_t(Ptr);
+
+  if (Handles.size() > 1) {
+    // This is different behaviour than what Posix dlsym(dlopen(NULL)) does.
+    // Doing that here is causing real problems for the JIT where msvc.dll
+    // and ucrt.dll can define the same symbols. The runtime linker will choose
+    // symbols from ucrt.dll first, but iterating NOT in reverse here would
+    // mean that the msvc.dll versions would be returned.
+
+    for (auto I = Handles.rbegin(), E = Handles.rend()-1; I != E; ++I) {
+      if (FARPROC Ptr = GetProcAddress(HMODULE(*I), Symbol))
+        return (void *) uintptr_t(Ptr);
     }
-
-sys::DynamicLibrary
-sys::DynamicLibrary::addPermanentLibrary(void *handle, std::string *errMsg) {
-  SmartScopedLock<true> lock(*SymbolsMutex);
-  // If we've already loaded this library, tell the caller.
-  if (!OpenedHandles->insert((HMODULE)handle).second) {
-    MakeErrMsg(errMsg, "Library already loaded");
-    return DynamicLibrary();
   }
-
-  return DynamicLibrary(handle);
+  return nullptr;
 }
 
+
 // Stack probing routines are in the support library (e.g. libgcc), but we don't
 // have dynamic linking on windows. Provide a hook.
 #define EXPLICIT_SYMBOL(SYM)                    \
   extern "C" { extern void *SYM; }
 #define EXPLICIT_SYMBOL2(SYMFROM, SYMTO) EXPLICIT_SYMBOL(SYMTO)
 
 #ifdef _M_IX86
 // Win32 on x86 implements certain single-precision math functions as macros.

 
 #include "explicit_symbols.inc"
 
 #undef EXPLICIT_SYMBOL
 #undef EXPLICIT_SYMBOL2
 #undef INLINE_DEF_SYMBOL1
 #undef INLINE_DEF_SYMBOL2
 
-void *sys::DynamicLibrary::SearchForAddressOfSymbol(const char *symbolName) {
-  SmartScopedLock<true> Lock(*SymbolsMutex);
-
-  // First check symbols added via AddSymbol().
-  if (ExplicitSymbols.isConstructed()) {
-    StringMap<void *>::iterator i = ExplicitSymbols->find(symbolName);
-
-    if (i != ExplicitSymbols->end())
-      return i->second;
-  }
-
-  // Now search the libraries.
-  if (OpenedHandles.isConstructed()) {
-    for (DenseSet<HMODULE>::iterator I = OpenedHandles->begin(),
-         E = OpenedHandles->end(); I != E; ++I) {
-      FARPROC ptr = GetProcAddress((HMODULE)*I, symbolName);
-      if (ptr) {
-        return (void *)(intptr_t)ptr;
-      }
-    }
-  }
+static void *DoSearch(const char *SymbolName) {
 
 #define EXPLICIT_SYMBOL(SYM)                                                   \
-  if (!strcmp(symbolName, #SYM))                                               \
+  if (!strcmp(SymbolName, #SYM))                                               \
     return (void *)&SYM;
 #define EXPLICIT_SYMBOL2(SYMFROM, SYMTO)                                       \
-  if (!strcmp(symbolName, #SYMFROM))                                           \
+  if (!strcmp(SymbolName, #SYMFROM))                                           \
     return (void *)&SYMTO;
 
 #ifdef _M_IX86
 #define INLINE_DEF_SYMBOL1(TYP, SYM)                                           \
-  if (!strcmp(symbolName, #SYM))                                               \
+  if (!strcmp(SymbolName, #SYM))                                               \
     return (void *)&inline_##SYM;
 #define INLINE_DEF_SYMBOL2(TYP, SYM) INLINE_DEF_SYMBOL1(TYP, SYM)
 #endif
 
   {
 #include "explicit_symbols.inc"
   }
 
 #undef EXPLICIT_SYMBOL
 #undef EXPLICIT_SYMBOL2
 #undef INLINE_DEF_SYMBOL1
 #undef INLINE_DEF_SYMBOL2
 
-  return 0;
-}
-
-void *sys::DynamicLibrary::getAddressOfSymbol(const char *symbolName) {
-  if (!isValid())
-    return NULL;
-  if (Data == &OpenedHandles)
-    return SearchForAddressOfSymbol(symbolName);
-  return (void *)(intptr_t)GetProcAddress((HMODULE)Data, symbolName);
-}
-
+  return nullptr;
 }

llvm/trunk/unittests/Support/CMakeLists.txt

add_llvm_unittest(SupportTests
   raw_ostream_test.cpp
   raw_pwrite_stream_test.cpp
   raw_sha1_ostream_test.cpp
   xxhashTest.cpp
   )
 
 # ManagedStatic.cpp uses <pthread>.
 target_link_libraries(SupportTests ${LLVM_PTHREAD_LIB})
+
+add_subdirectory(DynamicLibrary)

llvm/trunk/unittests/Support/DynamicLibrary/CMakeLists.txt

+set(LLVM_LINK_COMPONENTS Support)
+
+add_llvm_unittest(DynamicLibraryTests DynamicLibraryTest.cpp)
+
+export_executable_symbols(DynamicLibraryTests)
+
+add_library(PipSqueak SHARED PipSqueak.cxx)
+
+set_output_directory(PipSqueak
+  BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}
+  LIBRARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}
+  )
+
+set_target_properties(PipSqueak
+  PROPERTIES PREFIX ""
+  SUFFIX ".so"
+  )
+
+add_dependencies(DynamicLibraryTests PipSqueak)

llvm/trunk/unittests/Support/DynamicLibrary/DynamicLibraryTest.cpp

+//===- llvm/unittest/Support/DynamicLibrary/DynamicLibraryTest.cpp --------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Config/config.h"
+#include "llvm/Support/DynamicLibrary.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+
+#include "PipSqueak.h"
+#include <string>
+
+using namespace llvm;
+using namespace llvm::sys;
+
+extern "C" PIPSQUEAK_EXPORT const char *TestA() { return "ProcessCall"; }
+
+std::string LibPath() {
+  std::string Path =
+      fs::getMainExecutable("DynamicLibraryTests", (void *)&TestA);
+  llvm::SmallString<256> Buf(path::parent_path(Path));
+  path::append(Buf, "PipSqueak.so");
+  return Buf.str();
+}
+
+#if defined(_WIN32) || (defined(HAVE_DLFCN_H) && defined(HAVE_DLOPEN))
+
+typedef void (*SetStrings)(std::string &GStr, std::string &LStr);
+typedef const char *(*GetString)();
+
+template <class T> static T FuncPtr(void *Ptr) {
+  union {
+    T F;
+    void *P;
+  } Tmp;
+  Tmp.P = Ptr;
+  return Tmp.F;
+}
+template <class T> static void* PtrFunc(T *Func) {
+  union {
+    T *F;
+    void *P;
+  } Tmp;
+  Tmp.F = Func;
+  return Tmp.P;
+}
+
+static const char *OverloadTestA() { return "OverloadCall"; }
+
+std::string StdString(const char *Ptr) { return Ptr ? Ptr : ""; }
+
+TEST(DynamicLibrary, Overload) {
+  {
+    std::string Err;
+    llvm_shutdown_obj Shutdown;
+    DynamicLibrary DL =
+        DynamicLibrary::getPermanentLibrary(LibPath().c_str(), &Err);
+    EXPECT_TRUE(DL.isValid());
+    EXPECT_TRUE(Err.empty());
+
+    GetString GS = FuncPtr<GetString>(DL.getAddressOfSymbol("TestA"));
+    EXPECT_TRUE(GS != nullptr && GS != &TestA);
+    EXPECT_EQ(StdString(GS()), "LibCall");
+
+    GS = FuncPtr<GetString>(DynamicLibrary::SearchForAddressOfSymbol("TestA"));
+    EXPECT_TRUE(GS != nullptr && GS != &TestA);
+    EXPECT_EQ(StdString(GS()), "LibCall");
+
+    DL = DynamicLibrary::getPermanentLibrary(nullptr, &Err);
+    EXPECT_TRUE(DL.isValid());
+    EXPECT_TRUE(Err.empty());
+
+    GS = FuncPtr<GetString>(DynamicLibrary::SearchForAddressOfSymbol("TestA"));
+    EXPECT_TRUE(GS != nullptr && GS == &TestA);
+    EXPECT_EQ(StdString(GS()), "ProcessCall");
+
+    GS = FuncPtr<GetString>(DL.getAddressOfSymbol("TestA"));
+    EXPECT_TRUE(GS != nullptr && GS == &TestA);
+    EXPECT_EQ(StdString(GS()), "ProcessCall");
+
+    DynamicLibrary::AddSymbol("TestA", PtrFunc(&OverloadTestA));
+    GS = FuncPtr<GetString>(DL.getAddressOfSymbol("TestA"));
+    EXPECT_TRUE(GS != nullptr && GS != &OverloadTestA);
+
+    GS = FuncPtr<GetString>(DynamicLibrary::SearchForAddressOfSymbol("TestA"));
+    EXPECT_TRUE(GS != nullptr && GS == &OverloadTestA);
+    EXPECT_EQ(StdString(GS()), "OverloadCall");
+  }
+  EXPECT_TRUE(FuncPtr<GetString>(DynamicLibrary::SearchForAddressOfSymbol(
+                  "TestA")) == nullptr);
+}
+
+TEST(DynamicLibrary, Shutdown) {
+  std::string A, B;
+  {
+    std::string Err;
+    llvm_shutdown_obj Shutdown;
+    DynamicLibrary DL =
+        DynamicLibrary::getPermanentLibrary(LibPath().c_str(), &Err);
+    EXPECT_TRUE(DL.isValid());
+    EXPECT_TRUE(Err.empty());
+
+    SetStrings SS = FuncPtr<SetStrings>(
+        DynamicLibrary::SearchForAddressOfSymbol("SetStrings"));
+    EXPECT_TRUE(SS != nullptr);
+
+    SS(A, B);
+    EXPECT_EQ(B, "Local::Local");
+  }
+  EXPECT_EQ(A, "Global::~Global");
+  EXPECT_EQ(B, "Local::~Local");
+  EXPECT_TRUE(FuncPtr<SetStrings>(DynamicLibrary::SearchForAddressOfSymbol(
+                  "SetStrings")) == nullptr);
+}
+
+#else
+
+TEST(DynamicLibrary, Unsupported) {
+  std::string Err;
+  DynamicLibrary DL =
+      DynamicLibrary::getPermanentLibrary(LibPath().c_str(), &Err);
+  EXPECT_FALSE(DL.isValid());
+  EXPECT_EQ(Err, "dlopen() not supported on this platform");
+}
+
+#endif

llvm/trunk/unittests/Support/DynamicLibrary/PipSqueak.h

+//===- llvm/unittest/Support/DynamicLibrary/PipSqueak.h -------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_PIPSQUEAK_H
+#define LLVM_PIPSQUEAK_H
+
+#ifdef _WIN32
+#define PIPSQUEAK_EXPORT __declspec(dllexport)
+#else
+#define PIPSQUEAK_EXPORT
+#endif
+
+#endif

llvm/trunk/unittests/Support/DynamicLibrary/PipSqueak.cxx

+//===- llvm/unittest/Support/DynamicLibrary/PipSqueak.cxx -----------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "PipSqueak.h"
+#include <string>
+
+struct Global {
+  std::string *Str;
+  Global() : Str(nullptr) {}
+  ~Global() {
+    if (Str)
+      *Str = "Global::~Global";
+  }
+};
+
+struct Local {
+  std::string &Str;
+  Local(std::string &S) : Str(S) { Str = "Local::Local"; }
+  ~Local() { Str = "Local::~Local"; }
+};
+
+static Global Glb;
+
+extern "C" PIPSQUEAK_EXPORT void SetStrings(std::string &GStr,
+                                            std::string &LStr) {
+  static Local Lcl(LStr);
+  Glb.Str = &GStr;
+}
+
+extern "C" PIPSQUEAK_EXPORT const char *TestA() { return "LibCall"; }