[PassManagerBuilder] Remove global extension when a plugin is unloaded

[EliaGeretto]

Summary

This commit fixes PR39321.

GlobalExtensions is not guaranteed to be destroyed when optimizer plugins are unloaded. If it is indeed destroyed after a plugin is dlclose-d, the destructor of the corresponding ExtensionFn is not mapped anymore, causing a call to unmapped memory during destruction.

This commit guarantees that extensions coming from external plugins are removed from GlobalExtensions when the plugin is unloaded if GlobalExtensions has not been destroyed yet.

[grosser]

This looks like a better solution to the earlier reported bug. Thanks @EliaGeretto for working on this! I like the general approach. I only have two minor suggestions: (a) add some comments -- especially one that explains why the correct removal order is important for external plugins, (b) consider replacing an emptiness check with a targetted assert.

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

200

While the documentation is sparse already, I feel it would be helpful to document the newly introduced GlobalExtensionID.

llvm/lib/Transforms/IPO/PassManagerBuilder.cpp

211

Why do we check if GlobalExtensions is not empty? Should this condition not always be true assuming removeGlobalExtension was called with a valid and registered ExtensionID? If this is the case, would it not make more sense to 'assert' in case this invariant is broken rather than silently ignoring the request to remove an invalid ExtensionID?

Also, rather than checking if GlobalExtensionsNotEmpty holds, should we not check that the specific ExtensionID is in the GlobalExtensions vector?

[EliaGeretto]

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

200

I will do this in the next revision.

llvm/lib/Transforms/IPO/PassManagerBuilder.cpp

211

It was my mistake to use the GlobalExtensionsNotEmpty function. I am relying on the fact that isConstructed will return false if GlobalExtensions has already been destructed using destroy. In that case, nothing should be done; this is the case if LLVM is compiled without Polly. removeGlobalExtension is meant to do something only if GlobalExtensions was not destroyed yet; this is instead the case when compiling with Polly. I will use isConstructed directly and add a comment.

[EliaGeretto]

I have added comments to explain the reason of some of the changes and added an assert so that, if the removeGlobalExtension function is called with an invalid ID, the error will be more noticeable.

Please, let me know if you think the comments are clear enough.

[grosser]

This looks good from my side.

As this is touching some of the core parts of LLVM it would be good to get another review on this. Either @chandlerc or @philip.pfaffe maybe?

[EliaGeretto]

Ping.

[EliaGeretto]

Ping.

[EliaGeretto]

Ping.

[mehdi_amini]

llvm/lib/Transforms/IPO/PassManagerBuilder.cpp

213

Nit: please use early-exit.

215

llvm::find_if(GlobalExtensions, ...

[EliaGeretto]

removeGlobalExtension now early-exits when GlobalExtensions is not constructed and uses llvm::find_if to iterate over the collection.

[EliaGeretto]

@mehdi_amini Thank you for your review. I wanted to let you know that I do not have commit access, so someone else should commit this change for me. In addition, I think that this change could be backported to older LLVM releases, at least those that still get bugfixes. The bug addressed here is around since LLVM 5 it seems, as discussed here: https://bugs.llvm.org/show_bug.cgi?id=39321

[Meinersbur]

@serge-sans-paille Could you check whether this also solves D72493?

[EliaGeretto]

@Meinersbur @serge-sans-paille I have quickly taken a look at the other pull request and yes, it seems a fix for the exact same problem, discussed in PR39321.

[serge-sans-paille]

@Meinersbur I confirm it seems to solve the same issue, and I prefer the locality of this solution :-) Thanks @EliaGeretto

[jkorous]

@serge-sans-paille The test was still failing on macOS.

http://lab.llvm.org:8080/green/job/clang-stage1-cmake-RA-incremental/9061/consoleFull

******************** TEST 'LLVM :: Feature/load_extension.ll' FAILED ********************
Script:
--
: 'RUN: at line 1';   /Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/clang-build/bin/opt /Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/test/Feature/load_extension.ll -load=/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/clang-build/./lib/Bye.dylib -goodbye -wave-goodbye -disable-output 2>&1 | /Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/clang-build/bin/FileCheck /Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/test/Feature/load_extension.ll
--
Exit Code: 1
Hide Details


Command Output (stderr):
--
/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/test/Feature/load_extension.ll:3:10: error: CHECK: expected string not found in input
; CHECK: Bye
         ^
<stdin>:1:1: note: scanning from here
opt: CommandLine Error: Option 'attributor-max-iterations' registered more than once!
^
<stdin>:1:14: note: possible intended match here
opt: CommandLine Error: Option 'attributor-max-iterations' registered more than once!

I disabled it again.

commit 3524755a1a25c18ecf47e50e5186232da0751cfe (HEAD -> master, origin/master, origin/HEAD)
Author: Jan Korous <[e-mail]>
Date:   Wed Feb 5 11:03:38 2020 -0800

    Revert "Activate extension loading test on Darwin now that the underlying fix has landed"

    This reverts commit 058070893428a480b76a137f647ae6b9c89ac2d4.

[serge-sans-paille]

In D71959#1860286, @jkorous wrote:

@serge-sans-paille The test was still failing on macOS.

As far as I can tell, it's no longer the case after the various patch update. I re-enabled the test to assert that in d00900801aae04e6dfde7eb4ca2811d9a6887cf8

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

 ///   ...
 class PassManagerBuilder {
 public:
   /// Extensions are passed to 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 int GlobalExtensionID;
+
   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);
+  ///
+  /// \returns A global extension identifier that can be used to remove the
+  /// extension.
+  static GlobalExtensionID addGlobalExtension(ExtensionPointTy Ty,
+                                              ExtensionFn Fn);
+  /// Removes an extension that was previously added using addGlobalExtension.
+  /// This is also intended to be used by plugins, to remove any extension that
+  /// was previously registered before being unloaded.
+  ///
+  /// \param ExtensionID Identifier of the extension to be removed.
+  static void removeGlobalExtension(GlobalExtensionID ExtensionID);
   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.
-struct RegisterStandardPasses {
+class RegisterStandardPasses {
+  PassManagerBuilder::GlobalExtensionID ExtensionID;
+
+public:
   RegisterStandardPasses(PassManagerBuilder::ExtensionPointTy Ty,
                          PassManagerBuilder::ExtensionFn Fn) {
-    PassManagerBuilder::addGlobalExtension(Ty, std::move(Fn));
+    ExtensionID = PassManagerBuilder::addGlobalExtension(Ty, std::move(Fn));
+  }
+
+  ~RegisterStandardPasses() {
+    // If the collection holding the global extensions is destroyed after the
+    // plugin is unloaded, the extension has to be removed here. Indeed, the
+    // destructor of the ExtensionFn may reference code in the plugin.
+    PassManagerBuilder::removeGlobalExtension(ExtensionID);
   }
 };
 
 } // end namespace llvm
 #endif

llvm/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;
+static ManagedStatic<
+    SmallVector<std::tuple<PassManagerBuilder::ExtensionPointTy,
+                           PassManagerBuilder::ExtensionFn,
+                           PassManagerBuilder::GlobalExtensionID>,
+                8>>
+    GlobalExtensions;
+static PassManagerBuilder::GlobalExtensionID GlobalExtensionsCounter;
 
 /// Check if GlobalExtensions is constructed and not empty.
 /// Since GlobalExtensions is a managed static, calling 'empty()' will trigger
 /// the construction of the object.
 static bool GlobalExtensionsNotEmpty() {
   return GlobalExtensions.isConstructed() && !GlobalExtensions->empty();
 }
 
-void PassManagerBuilder::addGlobalExtension(
-    PassManagerBuilder::ExtensionPointTy Ty,
+PassManagerBuilder::GlobalExtensionID
+PassManagerBuilder::addGlobalExtension(PassManagerBuilder::ExtensionPointTy Ty,
                                        PassManagerBuilder::ExtensionFn Fn) {
-  GlobalExtensions->push_back(std::make_pair(Ty, std::move(Fn)));
+  auto ExtensionID = GlobalExtensionsCounter++;
+  GlobalExtensions->push_back(std::make_tuple(Ty, std::move(Fn), ExtensionID));
+  return ExtensionID;
+}
+
+void PassManagerBuilder::removeGlobalExtension(
+    PassManagerBuilder::GlobalExtensionID ExtensionID) {
+  // RegisterStandardPasses may try to call this function after GlobalExtensions
+  // has already been destroyed; doing so should not generate an error.
+  if (GlobalExtensions.isConstructed()) {
+    auto GlobalExtension =
+        std::find_if(GlobalExtensions->begin(), GlobalExtensions->end(),
+                     [ExtensionID](const auto &elem) {
+                       return std::get<2>(elem) == ExtensionID;
+                     });
+    assert(GlobalExtension != GlobalExtensions->end() &&
+           "The extension ID to be removed should always be valid.");
+
+    GlobalExtensions->erase(GlobalExtension);
+  }
 }
 
 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 {
   if (GlobalExtensionsNotEmpty()) {
     for (auto &Ext : *GlobalExtensions) {
-      if (Ext.first == ETy)
-        Ext.second(*this, PM);
+      if (std::get<0>(Ext) == ETy)
+        std::get<1>(Ext)(*this, PM);
     }
   }
   for (unsigned i = 0, e = Extensions.size(); i != e; ++i)
     if (Extensions[i].first == ETy)
       Extensions[i].second(*this, PM);
 }
 
 void PassManagerBuilder::addInitialAliasAnalysisPasses(