Add support for Semantic Analysis plugins #4430

JohanEngelen · 2023-06-29T23:06:14Z

This adds support for semantic analysis plugins, that receive the frontend modules ASTs.
The plugin's runSemanticAnalysis(Module) function is called for each module, after all other semantic analysis steps (also after DCompute SemA).

~~TODO~~ I'm not going to implement these things here:

Add explicit frontend source version checking (plus IN_LLVM check), to make sure that the user's plugin is compiled against the correct source. It needs to be the exact frontend source version, because of ABI incompatibility between versions.
- this is not trivial to do well, the CMake-generated source files with version info are not readily available by the user.
I'm thinking that the user wants a tool like ldc-build-runtime to build plugins...
- ~~Maybe, but let's not complicate things for now. It is not that hard to compile a plugin well, and we should document that.~~ tool is now part of the PR

The plugin's runSemanticAnalysis(Module) function is called for each module, after all other semantic analysis steps (also after DCompute SemA).

JohanEngelen · 2023-07-06T22:38:41Z

I'm stuck and don't know how to get this to work on Linux. Somehow some symbols are not exported on Linux, even when passing -export-dynamic to the linker.
Testing on Ubuntu 20 in docker container

> nm obj/ldc2.o | grep "_ZN16ParseTimeVisitorI10ASTCodegenE5visitEP9AliasThis"
0000000000000000 W _ZN16ParseTimeVisitorI10ASTCodegenE5visitEP9AliasThis
# capital W means it is a global weak symbol

> /opt/llvm/bin/clang++  -DDMDV2 -DNDEBUG  obj/ldc2.o  -o bin/ldc2  lib/libldc.a  -lLLVM-14  -L/usr/lib/llvm-14/lib  -Wl,--export-dynamic  -fvisibility=public  --target=aarch64-linux-gnu  -L/opt/ldc2/lib/aarch64-linux-gnu  -lphobos2-ldc  -ldruntime-ldc  -Wl,--gc-sections  -lrt  -ldl  -lpthread  -lm  -ldl -rdynamic
> nm bin/ldc2 | grep "_ZN16ParseTimeVisitorI10ASTCodegenE5visitEP9AliasThis"
00000000006ebac0 t _ZN16ParseTimeVisitorI10ASTCodegenE5visitEP9AliasThis
# lower case "t" means that it is a local (not exported) text symbol.

kinke · 2023-07-07T09:44:55Z

Oh, an instantiated symbol. For C++, this would require extra stuff too (an 'explicit instantiation'): https://discourse.llvm.org/t/clang-and-c-exporting-member-function-template-from-library-using-attribute-visibility-default/54914/2

JohanEngelen · 2023-07-07T13:48:19Z

Oh, an instantiated symbol. For C++, this would require extra stuff too (an 'explicit instantiation'): https://discourse.llvm.org/t/clang-and-c-exporting-member-function-template-from-library-using-attribute-visibility-default/54914/2

I'll have a look, but the symbol is already instantiated and emitted into the object file ldc2.o (as template linkonce symbol, nm calls it "W", so it is a public symbol). Then when linking ldc2.o into ldc2 binary, with --export-dynamic, then the symbol is not discarded (good!), but its visibility is set to local (nm reports "t").
I don't think we emit template symbols differently whether they were implicitly or explicitly instantiated? (do we even have a way of knowing that from the AST that reaches us?)

kinke · 2023-07-07T15:29:53Z

If I glanced over https://en.cppreference.com/w/cpp/language/function_template#Explicit_instantiation correctly, my understanding is that an explicit instantiation in C++ (nothing we'd have in D AFAIK) is a total special thing, probably getting an external linkage for IR, not weak_odr (or linkonce_odr). AFAIK, C++ also has -fvisibility-inlines-hidden to hide (implicitly) instantiated symbols.

JohanEngelen · 2023-07-16T01:06:32Z

I found the problem using readelf -s.
With -linkonce-templates:
19251: 0000000000000000 12 FUNC WEAK HIDDEN 1154 _ZN16ParseTimeVisitorI10A
Without -linkonce-templates:
23456: 0000000000000000 12 FUNC WEAK DEFAULT 1064 _ZN16ParseTimeVisitorI10A

-linkonce-templates is used by default when building LDC and makes the symbols non-public. Removing the flag makes more symbols global, but still a number of other symbols stay hidden... The search continues.

JohanEngelen · 2023-07-19T21:56:57Z

And there was a further problem with "weak hidden" symbols in libldc.a (e.g. _ZN7Visitor5visitEP14ErrorStatement), that override visibility of the same global symbol in ldc2.o; this was due to -fvisibility-inlines-hidden.
With the current fixes, plugins now work in a docker vm ubuntu.

…ing -fvisibility-inlines-hidden

JohanEngelen · 2023-07-19T22:35:50Z

@kinke For ease of use, I think that we should actually ship the DMD frontend source. That is: we should make it part of the install package. DMD also does this. We cannot put it in the import folder directly, because import dmd.xxx should also allow other frontend sources (i.e. obtained through dub). Can we add a src directory, like dmd does? And then we only put dmd frontend code in there. For example src/dmd/visitor.d.

kinke · 2023-07-20T09:58:25Z

I'm not convinced wrt. shipping the frontend sources - is there any other usage except for building such plugins? I guess plugins are going to be extremely niche anyway, not working on Windows etc.

JohanEngelen · 2023-07-20T10:17:13Z

I'm not convinced wrt. shipping the frontend sources - is there any other usage except for building such plugins? I guess plugins are going to be extremely niche anyway, not working on Windows etc.

OK. The use case is kind-of niche indeed.

By the way, the reason I want this is:

allow custom extra semantic checks, to prevent bugs (i.e. Weka will have its own plugin for that)
be able to detect and fix known regressions upon updating the compiler. For example, see this extreme bug: Destructor is not called for AliasSeq (type tuple) #4427 (https://issues.dlang.org/show_bug.cgi?id=24010). Updating to a compiler that has that bug fixed will have a high risk of hard-to-detect bugs in user code: suddenly destructors are called where they weren't in the past. To detect such cases a simple plugin can be used to catch all cases in the code that may be affected (that's the visitor_example.d in this PR!).

…1.30 does work)

JohanEngelen · 2023-07-21T22:52:25Z

I have added a ldc-build-plugin tool :-) It downloads LDC source if its not already there and puts the right cmdline flags in the call to LDC.

Edit: investigating why things no longer work on the macOS arm64 cirrus testers... (the complicated test works, but the simple test doesn't..?!)
Edit2: I have no clue why, but running the tests multiple times fixes it ("flakypass").

JohanEngelen · 2023-07-22T21:06:38Z

@kinke the PR is ready now.

kinke · 2023-07-23T09:02:46Z

cmake/Modules/BuildDExecutable.cmake

@@ -40,7 +40,8 @@ function(build_d_executable target_name output_exe d_src_files compiler_args lin
 # Compile all D modules to a single object.
 set(object_file ${PROJECT_BINARY_DIR}/obj/${target_name}${CMAKE_CXX_OUTPUT_EXTENSION})
 # Default to -linkonce-templates with LDMD host compiler, to speed-up optimization.
- if("${D_COMPILER_ID}" STREQUAL "LDMD")
+ # Note: for plugin support we need the symbols to be global, don't use -linkonce-templates.
+ if("${D_COMPILER_ID}" STREQUAL "LDMD" AND NOT LDC_ENABLE_PLUGINS)


This is actually only required for ldc2, no other binary. The dependency on foreign-defined LDC_ENABLE_PLUGINS would ideally be added to the comment starting in line 21.

changed it so it only removes the flag for ldc2. (zooming out, perhaps it's not so nice that this method adds flags, should be added elsewhere I feel)

Another option would be to compile the plugins with either -linkonce-templates or at least -allinst - all instantiated stuff should be defined in the plugin itself then.

kinke · 2023-07-23T09:12:25Z

tests/plugins/basic_sema_plugin.d

+}
+
+//--- testcase.d
+// CHECK: testcase.d([[@LINE+1]]): Warning: It works!


So does the split-file utility keep the original line numbers?

yes, with --leading-lines

kinke · 2023-07-23T09:21:18Z

CMakeLists.txt

+ # For plugins, we shouldn't apply this flag because it hides the inline methods of e.g. Visitor.
+ check_cxx_compiler_flag("-fvisibility-inlines-hidden" SUPPORTS_FVISIBILITY_INLINES_HIDDEN_FLAG)
+ if (${SUPPORTS_FVISIBILITY_INLINES_HIDDEN_FLAG} AND NOT LDC_ENABLE_PLUGINS)
+ append("-fvisibility-inlines-hidden" LDC_CXXFLAGS)


The macOS builds (both x64 and arm64) now produce something like ~500 warnings:

ld: warning: direct access in function 'llvm::cl::AddLiteralOption(llvm::cl::Option&, llvm::StringRef)' from file '/Users/runner/work/ldc/llvm/lib/libLLVMSupport.a(CommandLine.cpp.o)' to global weak symbol 'llvm::object_deleter<llvm::cl::SubCommand>::call(void*)' from file 'utils/CMakeFiles/split-file.dir/split-file.cpp.o' means the weak symbol cannot be overridden at runtime. This was likely caused by different translation units being compiled with different visibility settings.

I don't understand how the macOS CI testers are different from my macOS..... I don't see any of this, and also did not need to remove this flag.

I guess it's related to how LLVM was built. The CI build in our llvm-project fork is pretty standard though...

Let's see if I can only remove this flag for linux builds.

UNIX includes APPLE; I've pushed an according commit really testing macOS with that flag.

kinke · 2023-07-23T09:25:55Z

tools/ldc-build-plugin.d.in

+ "--d-version=IN_LLVM",
+ "-J" ~ buildPath(config.ldcSourceDir, "dmd", "res"),
+ "--shared",
+ "--defaultlib=",


Hmm, so do the plugins depend on statically-linked druntime+Phobos being exported from ldc2 too?

I'm not sure how well that works, both in terms of different host compilers for plugin/ldc2, and more complex plugins with TLS data, module ctors, GC usage etc. We normally require all D binaries of some process to be linked against shared druntime/Phobos...

The plugin should ideally be built by the same compiler that it will be used with. Any other compiler might give issues due to ABI differences. For the simple plugin in the test, apparently LDC 1.30 is a good enough match with LDC master, but that might change.

I intentionally do not link with the standard library, because (1) it caused dynamic load problems when the plugin was linked with stdlib (macos has statically linked stdlib), and (2) ldc2 exports stdlib symbols (shared or static druntime), which is good enough for now. Module ctors are probably not supported this way, but those are easily worked around. Having some plugin support is already so much better than having none at all!

Just saying that one might quite easily end up with hard-to-troubleshoot issues, as the plugin DSO isn't registered with druntime etc. The GC being disabled by default probably helps though wrt. unscanned static data ranges etc.

hm, so if we want to support plugins, we must link ldc2 with shared stdlib. Is there another option of manually letting ldc2 call the module registration upon loading a plugin?

I don't think there's a way around the shared libs; static druntime isn't able to handle multiple D binaries AFAIK. Plus the current official packages even have LTO'd druntime+Phobos, so hopefully excluding most unused parts, which a plugin might happen to need.

We currently link druntime+Phobos statically into each bundled D executable (incl. ldc-build-plugin etc.) for the official packages, so switching to the bundled shared libs could be somewhat nicer wrt. bundle size anyway. We'd have to take care of the rpath though, to keep the package portable etc.

fully agree with not making the shared lib change in this release, let's figure that out in new beta later :)
I'll need to write up a blog post about the plugin stuff, and then list all the shortcomings right now (due to no module init). I think it is still very very useful with that limitation.

kinke · 2023-07-23T10:00:59Z

Note that I plan on releasing a v1.34 beta soon (next weekend or so) - #4440 is basically ready, a single importC dmd-testsuite test wrt. __declspec(naked) fails. Just in case you'd be okay to postpone this a bit.

…atform dependent)

kinke · 2023-07-23T16:46:40Z

I have no clue why, but running the tests multiple times fixes it ("flakypass").

Hmm, possibly due to sharing the same temporary --od object-files dir when running multiple ldc-build-plugin processes in parallel

JohanEngelen · 2023-07-23T22:55:22Z

Reminder for ourselves: the visitor_example.d plugin will stop working after this fix for tuples: dlang/dmd#15351 . We can then change the plugin to trigger on some other AST thing.
Which I believe will be part of dlang 2.105 .

Add support for Semantic Analysis plugins

JohanEngelen added 3 commits June 29, 2023 23:50

[NFC] cleanup plugins.cpp a little

08c3371

Add support for semantic analysis (dlang frontend) plugins.

e86ef5b

The plugin's runSemanticAnalysis(Module) function is called for each module, after all other semantic analysis steps (also after DCompute SemA).

fixups

9cf7d91

JohanEngelen added the B-new-feature label Jun 29, 2023

JohanEngelen added 3 commits July 5, 2023 15:49

Add visitor test plugin

f1b38d7

fixup non-plugin build

c1e895e

fixups

89f43fd

Fix symbol visibility issues on Linux for plugins

fde0bf8

Reorder CMakeLists such that LDC_ENABLE_PLUGINS is available when add…

4256475

…ing -fvisibility-inlines-hidden

JohanEngelen marked this pull request as ready for review July 19, 2023 22:29

JohanEngelen added 3 commits July 20, 2023 00:39

fixup remnant from testing

86e677a

fixup host version check

e566bdb

fix python when compiler is not LDC

582ebec

disable plugin test for macOS x86(-64)

f7dbb67

JohanEngelen added 3 commits July 20, 2023 18:53

Increase plugin ABI LDC version. (1.27.1 did not work on my Mac, but …

82b365e

…1.30 does work)

Add changelog entry, and fix order of plugins vs dcompute.

c3b56d7

Add ldc-build-plugin tool, and make use of it in plugins lit tests.

b0d8748

JohanEngelen added 3 commits July 22, 2023 00:54

Update Changelog

a643e1d

try to fix CI by running tests multiple times

2c91ab1

Add ldc-build-plugin to cross build packaging

a45b0d8

kinke reviewed Jul 23, 2023

View reviewed changes

JohanEngelen and others added 2 commits July 23, 2023 12:45

Reduce the buildflag impact, try to only make changes when needed (pl…

e3b605e

…atform dependent)

[really re-add -fvisibility-inlines-hidden on Apple targets]

04fa0d5

[fix plugin tests flakiness]

64655c2

kinke merged commit ef7789a into ldc-developers:master Jul 23, 2023
23 of 24 checks passed

JohanEngelen deleted the sema_plugin branch July 24, 2023 20:22

JohanEngelen pushed a commit to weka/ldc that referenced this pull request Jul 31, 2023

Merge pull request ldc-developers#4430 from JohanEngelen/sema_plugin

f5462a2

Add support for Semantic Analysis plugins

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Semantic Analysis plugins #4430

Add support for Semantic Analysis plugins #4430

JohanEngelen commented Jun 29, 2023 •

edited

Loading

JohanEngelen commented Jul 6, 2023

kinke commented Jul 7, 2023

JohanEngelen commented Jul 7, 2023

kinke commented Jul 7, 2023

JohanEngelen commented Jul 16, 2023

JohanEngelen commented Jul 19, 2023

JohanEngelen commented Jul 19, 2023

kinke commented Jul 20, 2023

JohanEngelen commented Jul 20, 2023 •

edited

Loading

JohanEngelen commented Jul 21, 2023 •

edited

Loading

JohanEngelen commented Jul 22, 2023

kinke Jul 23, 2023

JohanEngelen Jul 23, 2023

kinke Jul 23, 2023

kinke Jul 23, 2023 •

edited

Loading

JohanEngelen Jul 23, 2023

kinke Jul 23, 2023

JohanEngelen Jul 23, 2023

kinke Jul 23, 2023

JohanEngelen Jul 23, 2023

JohanEngelen Jul 23, 2023

kinke Jul 23, 2023

kinke Jul 23, 2023

kinke Jul 23, 2023

JohanEngelen Jul 23, 2023

kinke Jul 23, 2023

JohanEngelen Jul 23, 2023

kinke Jul 23, 2023

JohanEngelen Jul 23, 2023

kinke commented Jul 23, 2023

kinke commented Jul 23, 2023

JohanEngelen commented Jul 23, 2023

Add support for Semantic Analysis plugins #4430

Add support for Semantic Analysis plugins #4430

Conversation

JohanEngelen commented Jun 29, 2023 • edited Loading

JohanEngelen commented Jul 6, 2023

kinke commented Jul 7, 2023

JohanEngelen commented Jul 7, 2023

kinke commented Jul 7, 2023

JohanEngelen commented Jul 16, 2023

JohanEngelen commented Jul 19, 2023

JohanEngelen commented Jul 19, 2023

kinke commented Jul 20, 2023

JohanEngelen commented Jul 20, 2023 • edited Loading

JohanEngelen commented Jul 21, 2023 • edited Loading

JohanEngelen commented Jul 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinke Jul 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinke commented Jul 23, 2023

kinke commented Jul 23, 2023

JohanEngelen commented Jul 23, 2023

JohanEngelen commented Jun 29, 2023 •

edited

Loading

JohanEngelen commented Jul 20, 2023 •

edited

Loading

JohanEngelen commented Jul 21, 2023 •

edited

Loading

kinke Jul 23, 2023 •

edited

Loading