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

[Frederich Munch via Phabricator via llvm-commits]

marsupial added inline comments.


================
Comment at: llvm/trunk/include/llvm/Support/DynamicLibrary.h:61
     /// 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
----------------
efriedma wrote:
> marsupial wrote:
> > efriedma wrote:
> > > efriedma wrote:
> > > > marsupial wrote:
> > > > > efriedma wrote:
> > > > > > marsupial wrote:
> > > > > > > efriedma wrote:
> > > > > > > > marsupial wrote:
> > > > > > > > > efriedma wrote:
> > > > > > > > > > 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.
> > > > > > > > > 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.
> > > > > > > > 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?
> > > > > > > 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.
> > > > > > 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.
> > > > > 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.
> > > > > 
> > > > That doesn't help my problem: we also have uses of RegisterManagedStatic in statically linked code.
> > > Err, sorry, typo.  Meant to say "we also have uses of llvm::RegisterStandardPasses in statically linked code".
> > 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?
> 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);
> ```
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D30107