[Plugins] Add a slim plugin API to work together with the new PM

[philip.pfaffe]

Summary

Add a new plugin API. This closes the gap between pass registration and out-of-tree passes for the new PassManager.

Unlike with the existing API, interaction with a plugin is always initiated from the tools perspective. I.e., when a plugin is loaded by the PluginLoader, it resolves and calls a well-known symbol llvmPluginGetInfo to obtain details about the plugin. The fundamental motivation is to get rid of as many global constructors as possible.
The API exposed by the plugin info is kept intentionally minimal.

[philip.pfaffe]

Retain backwards compatibility with plugins for the legacy PM.

[grosser]

Very cool to see this working!

[philip.pfaffe]

Correctly value-initialize the PluginInfo.

[philip.pfaffe]

Humble Ping

[chandlerc]

include/llvm/Support/Plugin.h

34

This seems like a somewhat nasty layering...

If plugins fundamentally deal in pass builders, I wouldn't expect the plugin infrastructure to be in the Support library at all. I think the only reason the plugin stuff started off in the support library is because it relied exclusively on global ctors to connect to any functionality. I really like the idea of moving away from that for plugins and having a more proper callback mechanism, but I think it means we need to either use some abstraction or just make the plugin logic (somewhat) specific to *pass builder* plugins.

Beyond getting the layering and pass build stuff designed nicely, I suspect we should rope some folks more broadly in the community in to make sure the high level approach to plugins here makes sense long term... I'll try to ask around and see who has opinions here.

[philip.pfaffe]

Improve Layering: Move New-PM Pass Plugins into the Passes library

[philip.pfaffe]

[NFC] clang-format

[chandlerc]

include/llvm/Passes/PassPlugin.h

23 ↗

(On Diff #136808)

We should have some documentation around when/where this should be incremented...

25–29 ↗

(On Diff #136808)

We should probably stick this into Compiler.h

40 ↗

(On Diff #136808)

Probably some documentation here would be good. ;]

Also, should this be weak so that it is OK if we never load a library that defines it?

44 ↗

(On Diff #136808)

Out of curiosity -- why use a macro rather than just documenting that a plugin must define the above function?

include/llvm/Passes/PassPluginLoader.h

18 ↗

(On Diff #136808)

This whole class needs doxygen comments. =D

Also, should it be called PassPlugin? And maybe be in the same header as above?

lib/Passes/PassPluginLoader.cpp

37–39 ↗

(On Diff #136808)

I think this is indicative that optional isn't the right return type here... I think you want something more like llvm::Expected here so you can put the error message into return and make this more library friendly.

tools/opt/NewPMDriver.cpp

47

maybe load-pass-plugin?

[philip.pfaffe]

include/llvm/Passes/PassPlugin.h

44 ↗

(On Diff #136808)

Mostly so that clients don't forget the extern "C". I just thought it was convenient and liked the declarative style.

If you have strong feelings about this I can document it instead, I'm not really married to the idea.

include/llvm/Passes/PassPluginLoader.h

18 ↗

(On Diff #136808)

I put the two classes in different headers because they have different clients. The PassPlugin.h header contains what's required on the client side, i.e. in the actual pass plugin. The Plugin class here is on the other hand only used by the driver to represent loaded plugins.

[philip.pfaffe]

Address review comments, mostly adding commentary and renaming things.

[philip.pfaffe]

Ping

[chandlerc]

include/llvm/Passes/PassPlugin.h

25 ↗

(On Diff #138387)

Here and below, we turn on Doxygen's auto-brief feature so I suspect most of the \brief tags can be dropped. This one might be the exception, I don't recall if autobrief works for \macro blocks or not.

55 ↗

(On Diff #138387)

A separate question: should this be spelled to be more specific? I'm imagining: llvmGetPassPluginInfo. Similar question about the name of the struct as well -- should it be specific to this being a pass plugin?

44 ↗

(On Diff #136808)

You can close the block-level extern "C" above after the type, and use a direct one here so that copy/paste of the declaration Just Works:

extern "C" PluginInfo LLVM_ATTRIBUTE_WEAK LLVM_PLUGIN_EXPORT llvmPluginGetInfo();

But maybe better to just give an example definition for copy/paste because I suspect it needn't be so long. At least, I would hope that the declaration here suffices to get the attributes and exports correct and the user can just:

extern "C" PluginInfo llvmPluginGetInfo() {
  ...
}

I'm always hesitant to generate code with a macro. It feels ... icky to me. Maybe that's just my bias.

include/llvm/Passes/PassPluginLoader.h

18 ↗

(On Diff #136808)

I don't (personally) see a big problem with having these in one file. It seems unlikely to be a burden on the plugin author to have this code included and keeps it co-located.

I would try to use names that differentiate the loaded plugin object from the C-struct used by a plugin to describe itself. I'm not sure what the best naming pair is there....

[philip.pfaffe]

Rename, move around, and simplify some things.

Most importantly: Rename the plugin-provided callback descriptor to
PassPluginLibraryInfo, and the handle to the loaded library to
PassPlugin.

[chandlerc]

This is looking really awesome. Starting to get into more mundane / nit picky comments below.

include/llvm/Passes/PassPlugin.h

66–67 ↗

(On Diff #140067)

Rather than expose the info object, maybe expose nice APIs for everything that wrap it up?

93 ↗

(On Diff #140067)

I don't know anything about Windows or DLLs really... But I would naively expect that *here* we want __declspec(dllimport), and we want plugins to use __declspec(dllexport). Is that not the case? I'm happy to just be told "nope, this is how it works" without even a comment. Mostly asking to learn.

lib/Passes/PassPlugin.cpp

20–21 ↗

(On Diff #140067)

Bundle the error string provided by getPermanentLibrary here?

20–21 ↗

(On Diff #140067)

This will be substantially more efficient if you cast the first component to a Twine as then the +s will be saved and evaluated at the end. Something like: Twine("foo") + a + "bar" + c is ideal when dealing with Twine operands (and that is how StringError is built).

38–40 ↗

(On Diff #140067)

Here in particular using the Twine composition is likely especially good. You should be able to either directly pass the C-strings or build StringRef's around them and avoid any allocation at this layer.

tools/opt/NewPMDriver.cpp

29

Stale header.

163

Expand a bit? "Walk the pass plugins and let them register any pass builder callbacks." or some such.

166–168

Generally LLVM style would suggest to continue here rather than using an else. Nto a big deal in this case.

unittests/Passes/Plugin.cxx

1 ↗

(On Diff #140067)

.cpp?

Also, maybe "TestPlugin"?

unittests/Passes/PluginsTest.cpp

26–34 ↗

(On Diff #140067)

Do we have any tests that do this successfully already?

I'm worried it will be a nightmare to get this to work portably. But it is a really awesome level of testing, so seems worth at least trying, just wanted you to be prepared for the potential churn required to get it to work.

37 ↗

(On Diff #140067)

Use StringRef instead? Or see below...

[philip.pfaffe]

unittests/Passes/PluginsTest.cpp

26–34 ↗

(On Diff #140067)

I think I copied this from unittests/Support/DynamicLibrary. Maybe it makes sense to factor this out into a common header in llvm/Testing?

[philip.pfaffe]

Address review comments:

• Use more StringRef and Twine
• Hide the PassPluginLibraryInfo object

[chandlerc]

LGTM with doc fixes and Twine fixes below. Really excited about this!

Thanks for the ton of work getting us from zero plugins to rich plugins!

include/llvm/Passes/PassPlugin.h

37 ↗

(On Diff #140946)

Maybe expand this a bit to explain this defines the core interface used to interact with plugins but is primarily for plugin implementors (as opposed to plugin users) and point users to the class below that loads and wraps the plugin?

53 ↗

(On Diff #140946)

%Metadata?

lib/Passes/PassPlugin.cpp

38–40 ↗

(On Diff #140067)

Rather than std::to_string, you can just call Twine on integers.

In fact, I think the operator+ overload will do the right thing here, and you can just add these together and it will do twine-based concatenation.

[miyuki]

The PluginsTest test is failing in our builds:

/work/llvm/src/unittests/Passes/PluginsTest.cpp:42: Failure
Value of: !!Plugin
  Actual: false
Expected: true
Plugin path: /work/llvm/build/unittests/Passes/TestPlugin.so
Expected<T> must be checked before access or destruction.
Unchecked Expected<T> contained error:
Could not load library '/work/llvm/build/unittests/Passes/TestPlugin.so': /work/llvm/build/unittests/Passes/TestPlugin.so: undefined symbol: _ZN4llvm23EnableABIBreakingChecksE

This is probably related to the fact that we use -DLLVM_EXPORT_SYMBOLS_FOR_PLUGINS=ON. The host platform is x86_64-linux. Could you please check, if the test is supposed to work in such configuration.

include/llvm/Support/Plugin.h

+//===-- llvm/Support/PluginLoader.h - Plugin Loader for Tools ---*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_SUPPORT_PLUGIN_H
+#define LLVM_SUPPORT_PLUGIN_H
+
+#include <cstdint>
+#include <functional>
+
+namespace llvm {
+class PassBuilder;
+
+#define LLVM_PLUGIN_API_VERSION 1
+
+#ifdef WIN32
+#define LLVM_PLUGIN_EXPORT __declspec(dllexport)
+#else
+#define LLVM_PLUGIN_EXPORT
+#endif
+
+extern "C" {
+struct PluginInfo {
+  uint32_t APIVersion;
+  const char *PluginName;
+  const char *PluginVersion;
+
+  void (*RegisterPassBuilderCallbacks)(PassBuilder &);
+};
+
+PluginInfo LLVM_PLUGIN_EXPORT llvmPluginGetInfo();
+}
+}
+
+#define LLVM_PLUGIN(NAME, PLUGIN_VERSION, REGISTER_CALLBACKS)                  \
+  extern "C" {                                                                 \
+  ::llvm::PluginInfo LLVM_PLUGIN_EXPORT llvmPluginGetInfo() {                  \
+    return {LLVM_PLUGIN_API_VERSION, NAME, PLUGIN_VERSION,                     \
+            REGISTER_CALLBACKS};                                               \
+  }                                                                            \
+  }
+
+#endif /* LLVM_SUPPORT_PLUGIN_H */

include/llvm/Support/PluginLoader.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_SUPPORT_PLUGINLOADER_H
 #define LLVM_SUPPORT_PLUGINLOADER_H
 
 #include "llvm/Support/CommandLine.h"
 
 namespace llvm {
-  struct PluginLoader {
-    void operator=(const std::string &Filename);
-    static unsigned getNumPlugins();
-    static std::string& getPlugin(unsigned num);
-  };
+struct PluginInfo;
+
+  static std::string &getPlugin(unsigned num);
+  static const PluginInfo &getPluginInfo(unsigned num);
 
 #ifndef DONT_GET_PLUGIN_LOADER_OPTION
-  // This causes operator= above to be invoked for every -load option.
-  static cl::opt<PluginLoader, false, cl::parser<std::string> >
+static cl::opt<PluginLoader, false, cl::parser<std::string>>
     LoadOpt("load", cl::ZeroOrMore, cl::value_desc("pluginfilename"),
             cl::desc("Load the specified plugin"));
 #endif
 }
 
 #endif

lib/Support/PluginLoader.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #define DONT_GET_PLUGIN_LOADER_OPTION
 #include "llvm/Support/PluginLoader.h"
 #include "llvm/Support/DynamicLibrary.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/Mutex.h"
+#include "llvm/Support/Plugin.h"
 #include "llvm/Support/raw_ostream.h"
 #include <vector>
 using namespace llvm;
 
-static ManagedStatic<std::vector<std::string> > Plugins;
-static ManagedStatic<sys::SmartMutex<true> > PluginsLock;
+class Plugin {
+public:
+  static Optional<Plugin> Load(const std::string &Filename,
+                               const sys::DynamicLibrary &Library);
+  std::string &getFilename() { return Filename; }
+  const PluginInfo &getInfo() const { return Info; }
+
+private:
+  Plugin(const std::string &Filename, const sys::DynamicLibrary &Library)
+      : Filename(Filename), Library(Library) {}
+  std::string Filename;
+  sys::DynamicLibrary Library;
+  PluginInfo Info;
+};
+
+static ManagedStatic<std::vector<Plugin>> Plugins;
+static ManagedStatic<sys::SmartMutex<true>> PluginsLock;
 
+Optional<Plugin> Plugin::Load(const std::string &Filename,
+                              const sys::DynamicLibrary &Library) {
+  auto *getDetailsFn = Library.SearchForAddressOfSymbol("llvmPluginGetInfo");
+  if (!getDetailsFn) {
+    errs() << "Entrypoint 'llvmPluginGetDetails()' not defined in '" << Filename
+           << "'\n";
+    return None;
+  }
+
+  Plugin P{Filename, Library};
+
+  P.Info = reinterpret_cast<decltype(llvmPluginGetInfo) *>(getDetailsFn)();
+
+  if (P.Info.APIVersion != LLVM_PLUGIN_API_VERSION) {
+    errs() << "Wrong API version on plugin '" << Filename << "'. Got version "
+           << P.Info.APIVersion << ", supported version is "
+           << LLVM_PLUGIN_API_VERSION << ".\n";
+    return None;
+  }
+
+  return P;
+}
+
 void PluginLoader::operator=(const std::string &Filename) {
   sys::SmartScopedLock<true> Lock(*PluginsLock);
   std::string Error;
-  if (sys::DynamicLibrary::LoadLibraryPermanently(Filename.c_str(), &Error)) {
+  auto Library =
+      sys::DynamicLibrary::getPermanentLibrary(Filename.c_str(), &Error);
+  if (!Library.isValid()) {
     errs() << "Error opening '" << Filename << "': " << Error
            << "\n  -load request ignored.\n";
-  } else {
-    Plugins->push_back(Filename);
+    return;
+  }
+
+  auto P = Plugin::Load(Filename, Library);
+  if (!P) {
+    errs() << "Failed to load plugin '" << Filename << "'\n";
+    return;
   }
+
+  Plugins->emplace_back(std::move(*P));
 }
 
 unsigned PluginLoader::getNumPlugins() {
   sys::SmartScopedLock<true> Lock(*PluginsLock);
   return Plugins.isConstructed() ? Plugins->size() : 0;
 }
 
 std::string &PluginLoader::getPlugin(unsigned num) {
   sys::SmartScopedLock<true> Lock(*PluginsLock);
   assert(Plugins.isConstructed() && num < Plugins->size() &&
          "Asking for an out of bounds plugin");
-  return (*Plugins)[num];
+  return (*Plugins)[num].getFilename();
+}
+const PluginInfo &PluginLoader::getPluginInfo(unsigned num) {
+  sys::SmartScopedLock<true> Lock(*PluginsLock);
+  assert(Plugins.isConstructed() && num < Plugins->size() &&
+         "Asking for an out of bounds plugin");
+  return (*Plugins)[num].getInfo();
 }

tools/opt/NewPMDriver.cpp

 #include "llvm/IR/Dominators.h"
 #include "llvm/IR/IRPrintingPasses.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/IR/Verifier.h"
 #include "llvm/Passes/PassBuilder.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ToolOutputFile.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Transforms/IPO/ThinLTOBitcodeWriter.h"
 #include "llvm/Transforms/Scalar/LoopPassManager.h"
 
+#define DONT_GET_PLUGIN_LOADER_OPTION
+#include "llvm/Support/Plugin.h"
+#include "llvm/Support/PluginLoader.h"
+
 using namespace llvm;
 using namespace opt_tool;
 
 static cl::opt<bool>
     DebugPM("debug-pass-manager", cl::Hidden,
             cl::desc("Print pass management debugging information"));
 
 // This flag specifies a textual description of the alias analysis pipeline to
 // use when querying for aliasing information. It only works in concert with
 // the "passes" flag above.
 static cl::opt<std::string>
     AAPipeline("aa-pipeline",
                cl::desc("A textual description of the alias analysis "
                         "pipeline for handling managed aliasing queries"),
                cl::Hidden);
 
 /// {{@ These options accept textual pipeline descriptions which will be
bool llvm::runPassPipeline(StringRef Arg0, Module &M, TargetMachine *TM,
                            VerifierKind VK,
                            bool ShouldPreserveAssemblyUseListOrder,
                            bool ShouldPreserveBitcodeUseListOrder,
                            bool EmitSummaryIndex, bool EmitModuleHash) {
   bool VerifyEachPass = VK == VK_VerifyEachPass;
   PassBuilder PB(TM);
   registerEPCallbacks(PB, VerifyEachPass, DebugPM);
 
+  for (unsigned p = 0; p < PluginLoader::getNumPlugins(); ++p)
+    if (PluginLoader::getPluginInfo(p).RegisterPassBuilderCallbacks)
+      PluginLoader::getPluginInfo(p).RegisterPassBuilderCallbacks(PB);
+
   // Specially handle the alias analysis manager so that we can register
   // a custom pipeline of AA passes with it.
   AAManager AA;
   if (!PB.parseAAPipeline(AA, AAPipeline)) {
     errs() << Arg0 << ": unable to parse AA pipeline description.\n";
     return false;
   }
 
   LoopAnalysisManager LAM(DebugPM);
   FunctionAnalysisManager FAM(DebugPM);

unittests/Support/CMakeLists.txt

 # as it does things intentionally, and there is no reliable way of
 # disabling all warnings for all the compilers by using pragmas.
 set_source_files_properties(AlignOfTest.cpp PROPERTIES COMPILE_FLAGS -w)
 
 # ManagedStatic.cpp uses <pthread>.
 target_link_libraries(SupportTests LLVMTestingSupport ${LLVM_PTHREAD_LIB})
 
 add_subdirectory(DynamicLibrary)
+add_subdirectory(Plugins)

unittests/Support/Plugins/CMakeLists.txt

+set(LLVM_LINK_COMPONENTS Support Passes Core)
+
+add_llvm_unittest(PluginsTests PluginsTest.cpp)
+export_executable_symbols(PluginsTests)
+
+add_library(TestPlugin SHARED Plugin.cxx)
+
+set_output_directory(TestPlugin
+  BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}
+  LIBRARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}
+  )
+
+set_target_properties(TestPlugin
+  PROPERTIES PREFIX ""
+  SUFFIX ".so"
+  )
+
+llvm_map_components_to_libnames(LLVMDependencies ${LLVM_LINK_COMPONENTS})
+target_link_libraries(TestPlugin ${LLVMDependencies})
+
+add_dependencies(PluginsTests TestPlugin)

unittests/Support/Plugins/Plugin.cxx

+#include "llvm/Support/Plugin.h"
+#include "llvm/Config/config.h"
+#include "llvm/Passes/PassBuilder.h"
+
+#include "TestPlugin.h"
+
+using namespace llvm;
+
+struct TestModulePass : public PassInfoMixin<TestModulePass> {
+  PreservedAnalyses run(Module &M, ModuleAnalysisManager &MAM) {
+    return PreservedAnalyses::all();
+  }
+};
+
+void registerCallbacks(PassBuilder &PB) {
+  PB.registerPipelineParsingCallback(
+      [](StringRef Name, ModulePassManager &PM,
+         ArrayRef<PassBuilder::PipelineElement> InnerPipeline) {
+        if (Name == "plugin-pass") {
+          PM.addPass(TestModulePass());
+          return true;
+        }
+        return false;
+      });
+}
+
+LLVM_PLUGIN(TEST_PLUGIN_NAME, TEST_PLUGIN_VERSION, registerCallbacks)

unittests/Support/Plugins/PluginsTest.cpp

+//===- llvm/unittest/Support/Plugins/PluginsTest.cpp --------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#define DONT_GET_PLUGIN_LOADER_OPTION
+#include "llvm/Analysis/CGSCCPassManager.h"
+#include "llvm/Config/config.h"
+#include "llvm/IR/PassManager.h"
+#include "llvm/Passes/PassBuilder.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/Plugin.h"
+#include "llvm/Support/PluginLoader.h"
+#include "llvm/Transforms/Scalar/LoopPassManager.h"
+#include "gtest/gtest.h"
+
+#include "TestPlugin.h"
+
+using namespace llvm;
+
+void anchor() {}
+
+static std::string LibPath(const std::string Name = "TestPlugin") {
+  const std::vector<testing::internal::string> &Argvs =
+      testing::internal::GetArgvs();
+  const char *Argv0 = Argvs.size() > 0 ? Argvs[0].c_str() : "PluginsTests";
+  void *Ptr = (void *)anchor;
+  std::string Path = sys::fs::getMainExecutable(Argv0, Ptr);
+  llvm::SmallString<256> Buf{sys::path::parent_path(Path)};
+  sys::path::append(Buf, (Name + ".so").c_str());
+  return Buf.str();
+}
+
+static std::string StdString(const char *c_str) { return c_str ? c_str : ""; }
+
+TEST(PluginsTests, LoadPlugin) {
+  ASSERT_EQ(0u, PluginLoader::getNumPlugins());
+
+  auto PluginPath = LibPath();
+  ASSERT_NE("", PluginPath);
+
+  PluginLoader() = PluginPath;
+  ASSERT_EQ(1u, PluginLoader::getNumPlugins()) << "Plugin path: " << PluginPath;
+
+  ASSERT_EQ(PluginPath, PluginLoader::getPlugin(0));
+  const auto &Info = PluginLoader::getPluginInfo(0);
+
+  ASSERT_EQ(TEST_PLUGIN_NAME, StdString(Info.PluginName));
+  ASSERT_EQ(TEST_PLUGIN_VERSION, StdString(Info.PluginVersion));
+  ASSERT_NE(nullptr, Info.RegisterPassBuilderCallbacks);
+
+  PassBuilder PB;
+  ModulePassManager PM;
+  ASSERT_FALSE(PB.parsePassPipeline(PM, "plugin-pass"));
+
+  Info.RegisterPassBuilderCallbacks(PB);
+  ASSERT_TRUE(PB.parsePassPipeline(PM, "plugin-pass"));
+}

unittests/Support/Plugins/TestPlugin.h

+#define TEST_PLUGIN_NAME "TestPlugin"
+#define TEST_PLUGIN_VERSION "0.1-unit"