This is an archive of the discontinued LLVM Phabricator instance.

Load compiler plugins in ASTUnit, too
Changes PlannedPublic

Authored by kfunk on Dec 22 2015, 3:53 PM.

Details

Summary

This change makes it possible to load plugins by passing extra clang
compiler arguments to libclang's clang_parseTranslation entry point.

Example: -Xclang -load -Xclang $LLVM_ROOT/lib/ClangLazy.so -Xclang
-add-plugin -Xclang clang-lazy

This makes it possible to introduce additional diagnostics, which can
then be shown by IDEs

See discussion on cfe-dev: http://lists.llvm.org/pipermail/cfe-dev/2014-October/039380.html

Original patch by Stefan Kemnitz <kemnitz.stefan at gmail.com>

Diff Detail

Event Timeline

kfunk updated this revision to Diff 43490.Dec 22 2015, 3:53 PM
kfunk retitled this revision from to Load compiler plugins in ASTUnit, too.
kfunk updated this object.

Doing as requested by http://lists.llvm.org/pipermail/cfe-dev/2014-October/039381.html, let's put this up for review in its initial form, so we can discuss a better approach.

kfunk added a comment.EditedDec 22 2015, 3:58 PM

By the way; prove that it really works: http://wstaw.org/m/2015/12/23/kdevelop-clazy.png (loving it!) :)

kfunk updated this object.Dec 22 2015, 3:58 PM

lgtm, but someone else should approve

doug.gregor edited edge metadata.Dec 24 2015, 2:01 PM

It looks good. Are you able to include a test for this?

kfunk updated this revision to Diff 44449.Jan 11 2016, 2:01 AM
kfunk edited edge metadata.

Update, add (non-working) test

Just uploading my WIP patch, now that the branching comes close. I added a test, unfortunately it doesn't do what I expect.
Please see the FIXME.

Didn't have the time to investigate yet, so any hints welcome!

realincubus edited edge metadata.Jan 12 2016, 8:37 AM

Hi.

I am finally back at work and happy to see that some effort already went into the patch that i initially wrote some time ago (D5611)

I just looked through the files of the new patch and noticed that the libclang.exports file is missing.
As far as i understand this, the exports file defines which symbols will be visible to the linker when linking to the
libclang.so library.

Since some of the symbols that are needed for the plugin mechanism to work are not exported, i wrote a list of these symbols and appended theses to the libclang.exports file.

If i don't add these symbols it's impossible to load any plugin from libclang.so

I assume that if you write a test for loading plugins via libclang you should write a test that acually uses libclang for loading plugins via its C interface (clang_parseTranslationUnit).

@kfunk you said that this works for you without the additional symbols. it might be that something changed in clangs codebase and i dont have to export the symbols anymore. In the following days i will try to run my code without the symbols and write a test that uses libclangs C interface.

all the best stefan kemnitz

kfunk added a comment.Jan 12 2016, 9:28 AM

@realincubus: Sorry, I didn't know you had put this up for review already.

So back to this patch. Yes, it works fine for me *without* amending libclang.exports. I'm injecting the arguments to clang_parseTranslationUnit2.

See this patch here for reference: https://quickgit.kde.org/?p=kdevelop.git&a=commit&h=b299e550f6753667d415c6aea8c0b0806e954e36
(when you view the whole blob of *parsesession.cpp*, you can also see the call to clang_parseTranslationUnit2)

ok after i checked out a recent version of clang and applied the patch D15729 i can verify, that one does not have to add the symbols to the libclang.exports file.

i believe the plugin mechanism was changed between the first version of my plugin and how it works right now.
i will now try to find out why the test doesn't work like it should or write another one.

milianw accepted this revision.Mar 5 2016, 5:42 AM
milianw added reviewers: klimek, rsmith.
milianw added a reviewer: milianw.

Still good from my side. @klimek, @rsmith: Could you please review this as well?

Thanks

This revision is now accepted and ready to land.Mar 5 2016, 5:42 AM
kfunk planned changes to this revision.Mar 5 2016, 5:48 AM

Note: Unit test is still not functional. I'll have another look at it now.

kfunk updated this revision to Diff 49999.Mar 7 2016, 2:54 PM
kfunk edited edge metadata.

Remove TODO statement

This revision is now accepted and ready to land.Mar 7 2016, 2:54 PM
kfunk added a comment.Mar 7 2016, 2:57 PM

Okay, just gave this another look. Unit test seems fine as-is. Let's call loadPlugins there explicitly.

@LLVM/Clang devs: Please review!

I'm especially interested in whether you think there's a better location for calling CompilerInstance::loadPlugins than in ASTUnit::Parse.

kfunk added a comment.Mar 10 2016, 9:38 AM

@bkramer: Thoughts about my patch? I wonder if this makes your "work-around" in D17808 redundant.

kfunk updated this revision to Diff 50720.Mar 15 2016, 6:08 AM

Remove unrelated hunk

bkramer accepted this revision.Mar 15 2016, 6:13 AM
bkramer edited edge metadata.

This looks good, thanks for the patch. When this goes in please watch the windows buildbots as plugin support on windows is somewhat broken and we may have to disable tests there.

Also, while this is a strict improvement and should go in I'm not so sure that it solves the "plugins in libclang" issue. I couldn't get it to work even with this patch due to the way we build libclang in Release mode.

@bkramer I can verify that after building this with clang 3.9 in release mode it does not work anymore like expected. everything is fine in debug mode

if i look at the output generated from

llvm::sys::DynamicLibrary::LoadLibraryPermanently

in 1761 CompilerInstance::loadPlugins()

i get errors like

/home/incubus/llvm_patch_test/build/lib/ClangPlugin.so /home/incubus/llvm_patch_test/build/lib/ClangPlugin.so: undefined symbol: _ZN5clang17ASTFrontendAction13ExecuteActionEv

In my original patch D5611 I added a list of symbols that had to be exported in order to make libclang work with plugins like expected.
After adding the symbols from the original patch and 2 additional ones it is ok now with clang version 3.9 in release mode.

Symbols needed:

_ZN5clang15PluginASTAction6anchorEv
_ZN4llvm11raw_ostream5writeEPKcm
_ZN4llvm4errsEv
_ZN5clang11ASTConsumer33HandleTopLevelDeclInObjCContainerENS_12DeclGroupRefE
_ZN5clang14FrontendActionD2Ev
_ZN5clang13DiagnosticIDs15getCustomDiagIDENS0_5LevelEN4llvm9StringRefE
_ZN5clang17ASTFrontendAction13ExecuteActionEv
_ZN5clang11ASTConsumer24HandleImplicitImportDeclEPNS_10ImportDeclE
_ZN5clang14FrontendAction22shouldEraseOutputFilesEv
_ZNK5clang15DeclarationName11getAsStringEv
_ZN5clang17DiagnosticsEngine21EmitCurrentDiagnosticEb
_ZN5clang11ASTConsumer21HandleInterestingDeclENS_12DeclGroupRefE
_ZN5clang14FrontendActionC2Ev
_ZN4llvm8RegistryIN5clang15PluginASTActionENS_14RegistryTraitsIS2_EEE4nodeC1ERKNS_19SimpleRegistryEntryIS2_EE
_ZTVN5clang15PluginASTActionE
_ZTVN5clang17ASTFrontendActionE
_ZTVN5clang11ASTConsumerE
_ZN4llvm8RegistryIN5clang15PluginASTActionEE4HeadE
_ZN4llvm8RegistryIN5clang15PluginASTActionEE4TailE

Please tell me if I am wrong, but "tools/libclang/libclang.exports" is kind of a filter.
Everything that is not listed in this file is not exported as a symbol in the resulting libclang.so file.

Is there a way to automatically add the needed symbols to the exports file ?

If we really want to go down this route we should drop the exports file, putting random symbols in there is not going to fly. I'm not sure what that would break for libclang users though.

Also make sure that your building without BUILD_SHARED_LIBS, otherwise you'll get a very distorted view of how libclang works.

would it be possible to add some cmake flag that would allow the user to choose whether to respect the libclang.exports file or not ?

kfunk planned changes to this revision.Nov 27 2017, 10:30 PM

Marking this Diff properly -- it needs more work.

nik added subscribers: yvvan, nik.Nov 28 2017, 12:13 AM