[PATCH] D71959: [PassManagerBuilder] Remove global extension when a plugin is unloaded

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a comment.

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.



================
Comment at: llvm/include/llvm/Transforms/IPO/PassManagerBuilder.h:200
+                                              ExtensionFn Fn);
+  static void removeGlobalExtension(GlobalExtensionID ExtensionID);
   void addExtension(ExtensionPointTy Ty, ExtensionFn Fn);
----------------
While the documentation is sparse already, I feel it would be helpful to document the newly introduced GlobalExtensionID.


================
Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:211
+    PassManagerBuilder::GlobalExtensionID ExtensionID) {
+  if (GlobalExtensionsNotEmpty()) {
+    GlobalExtensions->erase(
----------------
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71959/new/

https://reviews.llvm.org/D71959