[PATCH] D106137: [flang][driver] Add support for Frontend Plugins

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

@stuartellis , thank you for working on this! I've left a few comments - mostly nits.



================
Comment at: flang/examples/HelloWorld/CMakeLists.txt:1-5
+add_llvm_library(
+    flangHelloWorldPlugin
+    MODULE
+    HelloWorldPlugin.cpp
+)
----------------
I think that this should be fine as is, but will not work on Windows. Check this example: [[ https://github.com/llvm/llvm-project/blob/main/clang/examples/PrintFunctionNames/CMakeLists.txt | PrintFunctionNames ]]. I think that it's fine not to support Windows for now, but we should document this. Also, your tests need to marked as Linux-only (e.g. with `// REQUIRES: shell` or something similar).


================
Comment at: flang/examples/HelloWorld/HelloWorldPlugin.cpp:1
+#include "flang/Frontend/FrontendActions.h"
+#include "flang/Frontend/FrontendPluginRegistry.h"
----------------
Missing LLVM file header: https://llvm.org/docs/CodingStandards.html#file-headers


================
Comment at: flang/include/flang/Frontend/FrontendPluginRegistry.h:27
+#endif // LLVM_FLANG_FRONTEND_FRONTENDPLUGINREGISTRY_H
\ No newline at end of file

----------------
FIXME


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:202
 
+  if (llvm::opt::Arg *a = args.getLastArg(clang::driver::options::OPT_load)) {
+    opts.plugins.push_back(a->getValue());
----------------
[nit] Perhaps worth adding a comment to highlight _which_ option is being parsed?


================
Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:87
+      if (plugin.getName() == ci.frontendOpts().ActionName) {
+        std::unique_ptr<PluginParseTreeAction> P(plugin.instantiate());
+        return std::move(P);
----------------
Lower case `P`


================
Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:92
+    unsigned diagID = ci.diagnostics().getCustomDiagID(
+        clang::DiagnosticsEngine::Error, "unable to find plugin '%0'");
+    ci.diagnostics().Report(diagID) << ci.frontendOpts().ActionName;
----------------
Could you add a test for this diagnostic?


================
Comment at: flang/test/Driver/plugin-example.f90:7
+! RUN: %flang_fc1 -load %llvmshlibdir/flangHelloWorldPlugin%pluginext -plugin -hello-world %s 2>&1 | FileCheck %s
+! CHECK: Hello World from your new plugin
----------------
[tiny nit] Could you make it Flang specific? E.g. `Hello World from your new Flang plugin`


================
Comment at: flang/test/lit.cfg.py:55
+# Plugins (loadable modules)
+if config.has_plugins and config.llvm_plugin_ext:
+    config.available_features.add('plugins')
----------------
Could `llvm_plugin_ext` be ever empty? Perhaps I'm missing something, but it feels that `if config.has_plugins` should be sufficient here.


================
Comment at: flang/tools/flang-driver/CMakeLists.txt:30
 
+export_executable_symbols_for_plugins(flang-new)
+
----------------
We should make it possible to turn this off, perhaps:
```lang=clamg
# Some clever comment
if(FLANG_PLUGIN_SUPPORT)
  export_executable_symbols_for_plugins(flang-new)
endif()
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106137