Force RegisterStandardPasses to construct std::function in the IPO library.

[marsupial]

Summary

Fixes an issue using RegisterStandardPasses from a statically linked object before PassManagerBuilder::addGlobalExtension is called from a dynamic library.

RegisterStandardPasses used to accept a lambda, which when used from a dynamic library/plugin was allocated in the address space of the library.
If RegisterStandardPasses was used from the within the owning process first it would create the GlobalExtensions ManagedStatic.
GlobalExtensions would then exist in the ManagedStatic list before the DynamicLibrary::OpenedHandles Managed static.
So when the ManagedStatics were tore down, the memory holding the destructor of the lambda would have been released before GlobalExtensions attempts to use it to destruct the lambda.

To fix this we disallow using lambdas from RegisterStandardPasses.

[marsupial]

Added test for this specific case (ManagedStatic).
The target_link_libraries is valid when building as components, but I'm unclear if it is ok for static builds or platforms other than *nix.

[efriedma]

This doesn't fix the crash I described in https://reviews.llvm.org/D30107#763683 .

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

Works fine for me.

---

Regardless, the patch seems like a good idea.

[marsupial]

I'm sorry.
You're saying adding the changes to Hello.cpp,
make -j8 opt LLVMHello
bin/opt -load=lib/LLVMHello.so -S /dev/null
is still crashing?

This patch definitively fixes that problem here.

I'll try building clean with the instruction for polly (i.e. no cmake flags).
Could you do the same?

[efriedma]

In D33515#763847, @marsupial wrote:

I'm sorry.
You're saying adding the changes to Hello.cpp,
make -j8 opt LLVMHello
bin/opt -load=lib/LLVMHello.so -S /dev/null
is still crashing?

This patch definitively fixes that problem here.

I'll try building clean with the instruction for polly (i.e. no cmake flags).
Could you do the same?

On Ubuntu 14.04. Built with no CMake flags, r303808, with this patch plus my changes to LLVMHello.

$ bin/opt -load=lib/LLVMHello.so -S /dev/null
; ModuleID = '/dev/null'
source_filename = "/dev/null"
Segmentation fault

[marsupial]

addGlobalExtension is documented to be used by plugins, where you are using it for a statically linked lib.
There doesn't seem to be any great solution and I going back to how it was (leaking) is one of the worse options.

All I can see is:

1. Add an argument to RegisterStandardPasses that will choose a call to addGlobalExtension or addExtension.
2. Make RegisterManagedStatic and llvm_shutdown explicitly re-initialize/destroy the DynamicLibrary state.

[marsupial]

Perhaps we can try to close this one out before moving on to the larger problem?

I'm not trying to pass the buck here, this has in fact lead to finding two interesting bugs, but it certainly seems there's an API misuse/deficiency that super-cedes this issue. Even if the ManagedStatic ordering is completely sorted out, how is Polly handling the valid is case of a user using llvm_shutdown. That is once llvm_shutdown has been GlobalExtensions will have been cleared and the passes no longer available.

[efriedma]

addGlobalExtension is documented to be used by plugins, where you are using it for a statically linked lib.

polly has historically worked as a plugin. In this context, polly isn't technically a "plugin" in the sense that it's not dynamically loaded, but it works like a plugin in every other sense (using the same extension points a plugin would use to integrate with LLVM).

Add an argument to RegisterStandardPasses that will choose a call to addGlobalExtension or addExtension.

addGlobalExtension is static, addExtension is not; I don't follow how this would work.

Make RegisterManagedStatic and llvm_shutdown explicitly re-initialize/destroy the DynamicLibrary state.

This should work.

[efriedma]

I'm not trying to pass the buck here, this has in fact lead to finding two interesting bugs, but it certainly seems there's an API misuse/deficiency that super-cedes this issue. Even if the ManagedStatic ordering is completely sorted out, how is Polly handling the valid is case of a user using llvm_shutdown. That is once llvm_shutdown has been GlobalExtensions will have been cleared and the passes no longer available.

That might be a problem...? I'm not sure how people use llvm_shutdown in practice. (For clang/opt/etc. this doesn't matter because we don't run any LLVM passes after the call to llvm_shutdown.)

[marsupial]

polly has historically worked as a plugin. In this context, polly isn't technically a "plugin" in the sense that it's not dynamically loaded, but it works like a plugin in every other sense (using the same extension points a plugin would use to integrate with LLVM).

I get that but as per the comment above how does it handle the use case (regardless of this bug):

{
  llvm_shutdown_obj Shutdown;
  // Do some stuff (Polly passes available via RegisterStandardPasses)
}
// All passes added via RegisterStandardPasses have now been removed
{
  llvm_shutdown_obj Shutdown;
  // Do some other stuff (Polly passes no longer available)
}

[marsupial]

This should fix the issue and adds a test case of what you're seeing.
Not sure if I want to be the one committing this as it's a bit ugly...

[efriedma]

In D33515#764769, @marsupial wrote:

{
  llvm_shutdown_obj Shutdown;
  // Do some stuff (Polly passes available via RegisterStandardPasses)
}
// All passes added via RegisterStandardPasses have now been removed
{
  llvm_shutdown_obj Shutdown;
  // Do some other stuff (Polly passes no longer available)
}

Yes, this doesn't work at the moment.

The tricky part here is that we support both building both polly as a plugin and statically linking polly as part of the same build... but we could make it work, I guess, if people actually use llvm_shutdown_obj like this. (We could split the RegisterStandardPasses off into a separate object file, and only link it in when we're building the plugin.)

lib/Transforms/IPO/PassManagerBuilder.cpp

201

This change is assuming addGlobalExtension is special, i.e. destructors for other ManagedStatics won't run into this issue. That seems a little fragile... would it make sense to do this in ManagedStatic itself instead (i.e. ensure HandleSet is always constructed first/destroyed last)?

[marsupial]

This works here and is way less of a hack.

Overall I agree there should probably be a better mechanism to avoid this for other possibilities but that will require a lot more time.
If this is good enough maybe close it out and can report a bug on the underlying ManagedStatic issue.

[efriedma]

include/llvm/Transforms/IPO/PassManagerBuilder.h

183

Overloading like this seems a little dubious; if the ExtensionProc overload is the only one that's actually reliably usable, we shouldn't expose the ExtensionFn overload.

[marsupial]

include/llvm/Transforms/IPO/PassManagerBuilder.h

183

There is code that uses this via lambdas in LLVM.
Updated to limit RegisterStandardPasses to only accept a function pointer.

[marsupial]

Formatting errors.

[efriedma]

Oh, sorry, about the delay, I lost track of this.

LGTM.

[marsupial]

Conflict resolution & additional comment.

[mehdi_amini]

llvm/trunk/include/llvm/Transforms/IPO/PassManagerBuilder.h

213 ↗

(On Diff #102359)

I'm missing the *why* using a "real function" (I guess you meant "function pointer") matter?

[marsupial]

Add explicit dependency for CMake when built with LLVM_ENABLE_MODULES.

[marsupial]

Having trouble with the test part of this and CMake.

The idea of creating a shared lib with no dependencies on LLVM is breaking down for -DCMAKE_BUILD_TYPE=Debug and -DLLVM_ENABLE_MODULES=1.

[marsupial]

Relocate tests into Transforms/IPO.

[marsupial]

Formatting.

[marsupial]

I'm missing the *why* using a "real function" (I guess you meant "function pointer") matter?

Don't understand this.
Is the comment really confusing/unhelpful?
Otherwise I'd kind of like to land this and move on.

[marsupial]

Format.

[mehdi_amini]

In D33515#780599, @marsupial wrote:

I'm missing the *why* using a "real function" (I guess you meant "function pointer") matter?

Don't understand this.
Is the comment really confusing/unhelpful?

Yes. First "real" doesn't seem accurate. I'm unsure what it means.
Then I still have no idea why this patch is needed. I don't see any explanation anywhere.

[marsupial]

From the summary:

Fixes an issue using RegisterStandardPasses from a statically linked object before PassManagerBuilder::addGlobalExtension is called from a dynamic library.

What would you like either the comment or the summary it to say?

[mehdi_amini]

In D33515#780816, @marsupial wrote:

From the summary:

Fixes an issue using RegisterStandardPasses from a statically linked object before PassManagerBuilder::addGlobalExtension is called from a dynamic library.

What would you like either the comment or the summary it to say?

"fix an issue" is not precise enough. What issue? How does it manifest? What is the way to solve it?

[mehdi_amini]

If you need examples about what I'm expecting to read from a commit message (same applies to comment in the code though), you may look at these commit messages:

• http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170522/457153.html
• http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170206/427422.html

[mehdi_amini]

(If @efriedma thinks that the fix is the right one for the provided explanation that's fine with me)

include/llvm/Transforms/IPO/PassManagerBuilder.h

 ///   ...
 ///   Builder.addExtension(PassManagerBuilder::EP_LoopOptimizerEnd,
 ///                        addMyLoopPass);
 ///   ...
 class PassManagerBuilder {
 public:
   /// Extensions are passed the builder itself (so they can see how it is
   /// configured) as well as the pass manager to add stuff to.
-  typedef std::function<void(const PassManagerBuilder &Builder,
-                             legacy::PassManagerBase &PM)>
-      ExtensionFn;
+  typedef void ExtensionProc(const PassManagerBuilder &Builder,
+                             legacy::PassManagerBase &PM);
+  typedef std::function<ExtensionProc> ExtensionFn;
+
   enum ExtensionPointTy {
     /// EP_EarlyAsPossible - This extension point allows adding passes before
     /// any other transformations, allowing them to see the code as it is coming
     /// out of the frontend.
     EP_EarlyAsPossible,
 
     /// EP_ModuleOptimizerEarly - This extension point allows adding passes
     /// just before the main module-level optimization passes.
private:
   std::vector<std::pair<ExtensionPointTy, ExtensionFn>> Extensions;
 
 public:
   PassManagerBuilder();
   ~PassManagerBuilder();
   /// Adds an extension that will be used by all PassManagerBuilder instances.
   /// This is intended to be used by plugins, to register a set of
   /// optimisations to run automatically.
-  static void addGlobalExtension(ExtensionPointTy Ty, ExtensionFn Fn);
+  static void addGlobalExtension(ExtensionPointTy Ty, ExtensionProc Fn);
   void addExtension(ExtensionPointTy Ty, ExtensionFn Fn);
 
 private:
   void addExtensionsToPM(ExtensionPointTy ETy,
                          legacy::PassManagerBase &PM) const;
   void addInitialAliasAnalysisPasses(legacy::PassManagerBase &PM) const;
   void addLTOOptimizationPasses(legacy::PassManagerBase &PM);
   void addLateLTOOptimizationPasses(legacy::PassManagerBase &PM);
public:
   void populateLTOPassManager(legacy::PassManagerBase &PM);
   void populateThinLTOPassManager(legacy::PassManagerBase &PM);
 };
 
 /// Registers a function for adding a standard set of passes.  This should be
 /// used by optimizer plugins to allow all front ends to transparently use
 /// them.  Create a static instance of this class in your plugin, providing a
 /// private function that the PassManagerBuilder can use to add your passes.
+/// Currently limited to real functions to avoid crashes when used within the
+/// main executable before a loaded plugin has a chance to use this.
 struct RegisterStandardPasses {
   RegisterStandardPasses(PassManagerBuilder::ExtensionPointTy Ty,
-                         PassManagerBuilder::ExtensionFn Fn) {
-    PassManagerBuilder::addGlobalExtension(Ty, std::move(Fn));
+                         PassManagerBuilder::ExtensionProc Fn) {
+    PassManagerBuilder::addGlobalExtension(Ty, Fn);
   }
 };
 
 } // end namespace llvm
 #endif

lib/Transforms/IPO/PassManagerBuilder.cpp

PassManagerBuilder::~PassManagerBuilder() {
   delete LibraryInfo;
   delete Inliner;
 }
 
 /// Set of global extensions, automatically added as part of the standard set.
 static ManagedStatic<SmallVector<std::pair<PassManagerBuilder::ExtensionPointTy,
    PassManagerBuilder::ExtensionFn>, 8> > GlobalExtensions;
 
-void PassManagerBuilder::addGlobalExtension(
-    PassManagerBuilder::ExtensionPointTy Ty,
-    PassManagerBuilder::ExtensionFn Fn) {
-  GlobalExtensions->push_back(std::make_pair(Ty, std::move(Fn)));
+void PassManagerBuilder::addGlobalExtension(ExtensionPointTy Ty,
+                                            ExtensionProc Fn) {
+  GlobalExtensions->push_back(std::make_pair(Ty, Fn));
 }
 
 void PassManagerBuilder::addExtension(ExtensionPointTy Ty, ExtensionFn Fn) {
   Extensions.push_back(std::make_pair(Ty, std::move(Fn)));
 }
 
 void PassManagerBuilder::addExtensionsToPM(ExtensionPointTy ETy,
                                            legacy::PassManagerBase &PM) const {

unittests/Support/DynamicLibrary/CMakeLists.txt

-set(LLVM_LINK_COMPONENTS Support)
+set(LLVM_LINK_COMPONENTS Support IPO)
 
 add_library(DynamicLibraryLib STATIC ExportedFuncs.cxx)
 
 add_llvm_unittest(DynamicLibraryTests DynamicLibraryTest.cpp)
 target_link_libraries(DynamicLibraryTests DynamicLibraryLib)
 export_executable_symbols(DynamicLibraryTests)
 
 function(dynlib_add_module NAME)

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/Support/DynamicLibrary.h"
 #include "llvm/Config/config.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Transforms/IPO/PassManagerBuilder.h"
 #include "gtest/gtest.h"
 
 #include "PipSqueak.h"
 
 using namespace llvm;
 using namespace llvm::sys;
 
 std::string LibPath(const std::string Name = "PipSqueak") {
template <class T> static void* PtrFunc(T *Func) {
   union {
     T *F;
     void *P;
   } Tmp;
   Tmp.F = Func;
   return Tmp.P;
 }
 
+static void
+NoPassRegistration(const PassManagerBuilder &, legacy::PassManagerBase &) {
+}
+
+static RegisterStandardPasses LocalPass(PassManagerBuilder::EP_LoopOptimizerEnd,
+                                        NoPassRegistration);
+
+typedef void (*TestPassReg)(void (*)(PassManagerBuilder::ExtensionPointTy,
+                                     PassManagerBuilder::ExtensionProc));
+
+TEST(DynamicLibrary, PassRegistration) {
+  std::string Err;
+  llvm_shutdown_obj Shutdown;
+  DynamicLibrary DL =
+      DynamicLibrary::getPermanentLibrary(LibPath().c_str(), &Err);
+  EXPECT_TRUE(DL.isValid());
+  EXPECT_TRUE(Err.empty());
+
+  TestPassReg RP = FuncPtr<TestPassReg>(DL.getAddressOfSymbol("TestPassReg"));
+  RP(&PassManagerBuilder::addGlobalExtension);
+}
+
 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;

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 "llvm/Transforms/IPO/PassManagerBuilder.h"
+
+#define PIPSQUEAK_TESTA_RETURN "LibCall"
+#include "ExportedFuncs.cxx"
 
 struct Global {
   std::string *Str;
   std::vector<std::string> *Vec;
   Global() : Str(nullptr), Vec(nullptr) {}
   ~Global() {
     if (Str) {
       if (Vec)
extern "C" PIPSQUEAK_EXPORT void SetStrings(std::string &GStr,
   Glb.Str = &GStr;
   static Local Lcl(LStr);
 }
 
 extern "C" PIPSQUEAK_EXPORT void TestOrder(std::vector<std::string> &V) {
   Glb.Vec = &V;
 }
 
-#define PIPSQUEAK_TESTA_RETURN "LibCall"
-#include "ExportedFuncs.cxx"
+
+static void LibPassRegistration(const llvm::PassManagerBuilder &,
+                                llvm::legacy::PassManagerBase &) {}
+
+extern "C" PIPSQUEAK_EXPORT void TestPassReg(
+    void (*addGlobalExtension)(llvm::PassManagerBuilder::ExtensionPointTy,
+                               llvm::PassManagerBuilder::ExtensionProc)) {
+  addGlobalExtension(llvm::PassManagerBuilder::EP_EarlyAsPossible,
+                     LibPassRegistration);
+}