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/Transforms/CMakeLists.txt

 add_subdirectory(IPO)
+add_subdirectory(Registration)
 add_subdirectory(Scalar)
 add_subdirectory(Utils)

unittests/Transforms/Registration/CMakeLists.txt

+set(LLVM_LINK_COMPONENTS
+  Support
+  IPO
+  )
+
+add_llvm_unittest(IPOLoading
+  Registration.cpp
+  )
+
+
+if ( LLVM_INCLUDE_TESTS )
+  add_library(LoadablePass SHARED LoadablePass.cxx)
+
+  set_output_directory(LoadablePass
+    BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}
+    LIBRARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}
+    )
+
+  set_target_properties(LoadablePass
+    PROPERTIES PREFIX ""
+    SUFFIX ".so"
+    )
+
+  add_dependencies(IPOLoading LoadablePass)
+  add_dependencies(LoadablePass LLVMCore)
+
+  if ( LLVM_LINK_LLVM_DYLIB )
+    target_compile_definitions(LoadablePass PRIVATE LLVM_DYLIB_PASSREG_TEST_2)
+    target_link_libraries(LoadablePass LLVM)
+  elseif ( BUILD_SHARED_LIBS )
+    # Broken in Debug mode when building static libraries
+    target_compile_definitions(LoadablePass PRIVATE LLVM_DYLIB_PASSREG_TEST_2)
+    target_link_libraries(LoadablePass LLVMipo)
+  endif ()
+
+endif ()

unittests/Transforms/Registration/LoadablePass.cxx

+//===- llvm/unittest/Transforms/Registration/LoadablePass.cxx -------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Transforms/IPO/PassManagerBuilder.h"
+
+#ifdef _WIN32
+#define LLVM_PASS_EXPORT_TEST extern "C" __declspec(dllexport)
+#else
+#define LLVM_PASS_EXPORT_TEST extern "C"
+#endif
+
+using namespace llvm;
+
+static void EmptyPass2(const PassManagerBuilder &, legacy::PassManagerBase &) {}
+
+LLVM_PASS_EXPORT_TEST void TestPassReg(
+    void (*addGlobalExtension)(PassManagerBuilder::ExtensionPointTy,
+                               PassManagerBuilder::ExtensionProc)) {
+  addGlobalExtension(PassManagerBuilder::EP_EarlyAsPossible,
+                     EmptyPass2);
+}
+
+#ifdef LLVM_DYLIB_PASSREG_TEST_2
+static RegisterStandardPasses sLocalPass(PassManagerBuilder::EP_EarlyAsPossible,
+                                         EmptyPass2);
+#endif

unittests/Transforms/Registration/Registration.cpp

+//===-- Registration.cpp - Unit tests for pass registration & shutdown ----===//
+//
+//                     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/Support/FileSystem.h"
+#include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Transforms/IPO/PassManagerBuilder.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace llvm::sys;
+
+static std::string LibPath(const std::string Name = "LoadablePass") {
+  const std::vector<testing::internal::string>& Argvs = testing::internal::GetArgvs();
+  const char *Argv0 = Argvs.size() > 0 ? Argvs[0].c_str() : "IPOTests";
+  void *Ptr = (void*)(intptr_t)LibPath;
+  std::string Path = fs::getMainExecutable(Argv0, Ptr);
+  llvm::SmallString<256> Buf(path::parent_path(Path));
+  path::append(Buf, (Name+".so").c_str());
+  return Buf.str();
+}
+
+template <class T> static T FuncPtr(void *Ptr) {
+  union {
+    T F;
+    void *P;
+  } Tmp;
+  Tmp.P = Ptr;
+  return Tmp.F;
+}
+
+static void
+EmptyPass(const PassManagerBuilder &, legacy::PassManagerBase &) {
+}
+
+static RegisterStandardPasses LocalPass(PassManagerBuilder::EP_LoopOptimizerEnd,
+                                        EmptyPass);
+
+typedef void (*TestPassReg)(void (*)(PassManagerBuilder::ExtensionPointTy,
+                                     PassManagerBuilder::ExtensionProc));
+
+TEST(Registration, shutdownOrder) {
+  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"));
+  EXPECT_TRUE(RP != nullptr);
+  if (RP)
+    RP(&PassManagerBuilder::addGlobalExtension);
+}