Page MenuHomePhabricator

[flang][driver] Add support for Frontend Plugins
ClosedPublic

Authored by stuartellis on Jul 16 2021, 3:12 AM.

Details

Summary

Introducing a plugin API and a simple HelloWorld Plugin example.
This patch adds the -load and -plugin flags to frontend driver and
the code around using custom frontend actions from within a plugin
shared library object.

It also adds to the Driver-help test to check the help option with the
updated driver flags.

Additionally, the patch creates a plugin-example test to check the
HelloWorld plugin example runs correctly. As part of this, a new CMake
flag (FLANG_BUILD_EXAMPLES) is added to allow the example to be built
and for the test to run.

This Plugin API has only been tested on Linux.

Diff Detail

Event Timeline

stuartellis created this revision.Jul 16 2021, 3:12 AM
stuartellis requested review of this revision.Jul 16 2021, 3:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2021, 3:12 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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

flang/examples/HelloWorld/CMakeLists.txt
2–6

I think that this should be fine as is, but will not work on Windows. Check this example: 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).

flang/examples/HelloWorld/HelloWorldPlugin.cpp
2
flang/include/flang/Frontend/FrontendPluginRegistry.h
27

FIXME

flang/lib/Frontend/CompilerInvocation.cpp
202

[nit] Perhaps worth adding a comment to highlight _which_ option is being parsed?

flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
70

Lower case P

75

Could you add a test for this diagnostic?

flang/test/Driver/plugin-example.f90
8

[tiny nit] Could you make it Flang specific? E.g. Hello World from your new Flang plugin

flang/test/lit.cfg.py
52

Could llvm_plugin_ext be ever empty? Perhaps I'm missing something, but it feels that if config.has_plugins should be sufficient here.

flang/tools/flang-driver/CMakeLists.txt
30

We should make it possible to turn this off, perhaps:

# Some clever comment
if(FLANG_PLUGIN_SUPPORT)
  export_executable_symbols_for_plugins(flang-new)
endif()
coti added a subscriber: coti.Jul 19 2021, 6:29 PM

Address review comments
Added some comments to a couple of places
Added new FLANG_PLUGIN_SUPPORT CMake flag to control exporting executable symbols

stuartellis marked 9 inline comments as done.Jul 22 2021, 11:05 AM
stuartellis added inline comments.
flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
75

Added diagnostic test to the end of plugin-example.f90 test

stuartellis edited the summary of this revision. (Show Details)Jul 22 2021, 11:09 AM
stuartellis added a project: Restricted Project.

I think that this is almost ready to be merged. Just a couple more comments.

flang/examples/CMakeLists.txt
5

Could you add the following at the top of this file:

if(NOT FLANG_BUILD_EXAMPLES)
  set(EXCLUDE_FROM_ALL ON)
endif()

This will make sure that the examples are not build with e.g. ninja when FLANG_BUILD_EXAMPLES is Off.

In Clang, CMake also contains

set_property(DIRECTORY PROPERTY EXCLUDE_FROM_ALL ON)

I think that set(EXCLUDE_FROM_ALL ON) should be sufficient though (see CMake docs for EXCLUDE_FROM_ALL).

Note that in LLVM, there is add_llvm_example_library. It would be nice to use that, be we'd need to make sure that LLVM_BUILD_EXAMPLES is set correctly. But that's not required just now and could be added later.

flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
129

This will silence this warning:

if (flang->diagnostics().hasErrorOccurred()) {
    return false;
}
stuartellis marked an inline comment as done.

Address review comments
Small changes due to rebase

This revision is now accepted and ready to land.Aug 10 2021, 5:06 AM
This revision was landed with ongoing or failed builds.Aug 12 2021, 3:56 AM
This revision was automatically updated to reflect the committed changes.

This has broken cmake config on the out of tree flang bot:
https://lab.llvm.org/buildbot/#/builders/175/builds/2026

(with perhaps the least helpful cmake error I've ever seen)

Not personally familiar with what "out of tree" means for flang but you should be able to pull the steps from the buildbot UI. (perhaps your change assumes it's "in tree")

awarzynski added a comment.EditedAug 12 2021, 8:54 AM

This has broken cmake config on the out of tree flang bot:
https://lab.llvm.org/buildbot/#/builders/175/builds/2026

(with perhaps the least helpful cmake error I've ever seen)

Not personally familiar with what "out of tree" means for flang but you should be able to pull the steps from the buildbot UI. (perhaps your change assumes it's "in tree")

Thank you David, I've been able to reproduce this locally. I hope to submit a workaround shortly. This is failing because out-of-tree SHLIBDIR is not defined.

EDIT: Fixed in https://reviews.llvm.org/D107973