This is an archive of the discontinued LLVM Phabricator instance.

[NewPM] Add support for new-PM plugins to clang
ClosedPublic

Authored by melver on Jan 18 2019, 2:02 PM.

Details

Summary

This adds support for new-PM plugin loading to clang. The option
-fpass-plugin= may be used to specify a dynamic shared object file
that adheres to the PassPlugin API.

Tested: created simple plugin that registers an EP callback; with optimization level > 0, the pass is run as expected.

Diff Detail

Repository
rC Clang

Event Timeline

melver created this revision.Jan 18 2019, 2:02 PM
melver edited the summary of this revision. (Show Details)Jan 18 2019, 2:10 PM
melver added reviewers: philip.pfaffe, chandlerc.
melver set the repository for this revision to rC Clang.
melver added a project: Restricted Project.

This generally looks sane. What will happen on windows though? Will it silently fail?

clang/include/clang/Basic/CodeGenOptions.h
292 ↗(On Diff #182603)

This should be SmallVector.

melver updated this revision to Diff 182974.Jan 22 2019, 1:40 PM
melver marked 2 inline comments as done.
  • Use SmallVector in CodeGenOptions.h

Thanks!

This generally looks sane. What will happen on windows though? Will it silently fail?

AFAIK PassPlugin::Load uses sys::DynamicLibrary::getPermanentLibrary, which uses DynamicLibrary::HandleSet::AddLibrary which works for Windows as well. (The story is similar to legacy -fplugin=).

clang/include/clang/Basic/CodeGenOptions.h
292 ↗(On Diff #182603)

Not sure if this is better. getAllArgValues returns a vector<string>, which is why I think the above members are also vector<string>. And std::vector cannot be assigned to SmallVector, which required an extra line in CompilerInvocation.cpp.

Let me know what you think.

This generally looks sane. What will happen on windows though? Will it silently fa

AFAIK PassPlugin::Load uses sys::DynamicLibrary::getPermanentLibrary, which uses DynamicLibrary::HandleSet::AddLibrary which works for Windows as well. (The story is similar to legacy -fplugin=).

s/AddLibrary/DLOpen/ -- DLOpen and others in DynamicLibrary:: are wrappers around platform-specific code. On Windows the implementation is in: llvm/lib/Support/Windows/DynamicLibrary.inc

I'm not sure what the current state of plugins on windows is. They were broken and disabled last time I worked on this, but that might've changed in the meantime! Worth checking.

clang/include/clang/Basic/CodeGenOptions.h
292 ↗(On Diff #182603)

You're right, SmallVector doesn't actually help here. Sorry!

melver updated this revision to Diff 183140.Jan 23 2019, 11:37 AM
  • Revert use of SmallVector

Thanks!

I'm not sure what the current state of plugins on windows is. They were broken and disabled last time I worked on this, but that might've changed in the meantime! Worth checking.

Right, though I feel this is related to DynamicLibrary, and orthogonal to this patch. I don't have access to a Windows machine right now, but I can try to check if you feel it's urgent to land this. What do you think?

melver marked an inline comment as done.Jan 24 2019, 3:20 PM
philip.pfaffe accepted this revision.Jan 25 2019, 12:57 AM

It would be good to check, since the bots won't! Otherwise this looks good.

This revision is now accepted and ready to land.Jan 25 2019, 12:57 AM
melver updated this revision to Diff 183715.Jan 26 2019, 11:28 AM
  • Improve error reporting. While testing on Windows, noticed that Clang wants the error to be checked otherwise crashed quite verbosely.

It would be good to check, since the bots won't! Otherwise this looks good.

Thanks!

I attempted to test on Windows, but noticed that for a shared library CMake already reports "Loadable modules not supported on this platform", and couldn't create a real plugin. In any case, I attempted to load an arbitrary DLL to see if errors are reported correctly and subsequently improved that. If a library can't be loaded, it will not fail silently now.

If you're happy with this version, I would need you to land this for me (I do not have commit access). Many thanks!

Gentle ping. Need someone to land this on my behalf, as I do not have commit access. Many thanks!

This revision was automatically updated to reflect the committed changes.

Landed it for you in r352972. Thanks!

Landed it for you in r352972. Thanks!

Great, thanks!