[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:
> > > 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.
> 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*) ()`
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?


Repository:
  rL LLVM

https://reviews.llvm.org/D30107