Page MenuHomePhabricator

Adjust Registry interface to not require plugins to export a registry
ClosedPublic

Authored by john.brawn on Jun 15 2016, 9:03 AM.

Details

Summary

Currently the Registry class contains the vestiges of a previous attempt to allow plugins to be used on Windows without using BUILD_SHARED_LIBS, where a plugin would have its own copy of a registry and export it to be imported by the tool that's loading the plugin. This only works if the plugin is entirely self-contained with the only interface between the plugin and tool being the registry, and in particular this conflicts with how IR pass plugins work.

This patch changes things so that instead the add_node function of the registry is exported by the tool and then imported by the plugin, which solves this problem and also means that instead of every plugin having to export every registry they use instead LLVM only has to export the add_node functions. This allows plugins that use a registry to work on Windows if LLVM_EXPORT_SYMBOLS_FOR_PLUGINS is used.

Diff Detail

Repository
rL LLVM

Event Timeline

john.brawn updated this revision to Diff 60844.Jun 15 2016, 9:03 AM
john.brawn retitled this revision from to Adjust Registry interface to not require plugins to export a registry.
john.brawn updated this object.
john.brawn added reviewers: ehsan, reames, chapuni.
john.brawn set the repository for this revision to rL LLVM.
john.brawn added subscribers: llvm-commits, cfe-commits.
Ilod added a subscriber: Ilod.Jun 30 2016, 3:51 PM

Hello,

When I tried your patch and added LLVM_EXPORT_SYMBOLS_FOR_PLUGINS=ON, I had multiple errors on your python scripts, like:

  File "utils/extract_symbols.py", line 399, in <module>
    calling_convention_decoration = is_32bit_windows(libs[0])
  File "utils/extract_symbols.py", line 88, in dumpbin_is_32bit_windows
    match = re.match('.+machine \((\S+)\)', line)
  File "C:\Program Files\Python35\lib\re.py", line 163, in match
    return _compile(pattern, flags).match(string)
TypeError: cannot use a string pattern on a bytes-like object

And after I fixed them by passing bytes-like object literals, I had the following error:

  File "utils/extract_symbols.py", line 488, in <module>
    print >>outfile, str(k)
TypeError: unsupported operand type(s) for >>: 'builtin_function_or_method' and '_io.TextIOWrapper'

I gave up after this. I have Pyhton 3.5 64 bits, and was building clang 32-bits on Windows with Visual Studio 2015 Update 2.

Looking at the patch:

If I understand correctly, the pros/cons between your approach and the previous attempt are:

  • Your method allows to export the various symbols of the tool from the plugin, allowing mainly to access/modify global variables of it, instead of having another instance of it in the DLL (thus probably causing many bugs)
  • Previous attempt allowed to make a plugin which could be loaded by any tool, while yours need the plugin to be specific to a tool

If that's all, I suppose it's better to have your limitation to plugin support rather than a full support which will bug if you call a function that accesses a static member.

(By the way, the previous attempt works fine, it was reverted because of a mingw crash which can be fixed by the patch D18847)

However, couldn't we enable by default LLVM_EXPORT_SYMBOLS_FOR_PLUGINS on the platforms/compilers which doesn't currently support the plugins? Having plugins only work on custom builds of clang seems to be against the main use of plugins. (If testing is the only problem, I would gladly test it on Windows with mingw, and maybe Mac if needed)

Also, could you add in the documentation (cfe/trunk/docs/ClangPlugins.rst) what are the limitations/setup needed to make an out-of-tree plugin?

In D21385#471807, @Ilod wrote:

I gave up after this. I have Pyhton 3.5 64 bits, and was building clang 32-bits on Windows with Visual Studio 2015 Update 2.

python 3 isn't supported when building llvm/clang, according to llvm's CMakeLists.txt.

If that's all, I suppose it's better to have your limitation to plugin support rather than a full support which will bug if you call a function that accesses a static member.

(By the way, the previous attempt works fine, it was reverted because of a mingw crash which can be fixed by the patch D18847)

However, couldn't we enable by default LLVM_EXPORT_SYMBOLS_FOR_PLUGINS on the platforms/compilers which doesn't currently support the plugins? Having plugins only work on custom builds of clang seems to be against the main use of plugins. (If testing is the only problem, I would gladly test it on Windows with mingw, and maybe Mac if needed)

After this patch goes in I plan to email llvm-dev/cfe-dev to see if people think that's a good idea.

Also, could you add in the documentation (cfe/trunk/docs/ClangPlugins.rst) what are the limitations/setup needed to make an out-of-tree plugin?

Will do.

Ilod added a comment.Jul 1 2016, 5:29 AM
In D21385#471807, @Ilod wrote:

I gave up after this. I have Pyhton 3.5 64 bits, and was building clang 32-bits on Windows with Visual Studio 2015 Update 2.

python 3 isn't supported when building llvm/clang, according to llvm's CMakeLists.txt.

Oops, will try again tonight with Python 2.
http://clang.llvm.org/get_started.html doesn't specify a version of Python, the LLVM webpage only say >= 2.7, and I ran the tests without any problem with Python 3 (but maybe some tests were automatically skipped?). Also, Python is also marked as only necessary to build the tests, which is no longer true with your script.

http://clang.llvm.org/get_started.html doesn't specify a version of Python, the LLVM webpage only say >= 2.7, and I ran the tests without any problem with Python 3 (but maybe some tests were automatically skipped?). Also, Python is also marked as only necessary to build the tests, which is no longer true with your script.

Hmm, it looks like this webpage (and http://llvm.org/docs/GettingStarted.html#requirements) has been out of date since 2011 (commits r143742 and r143793), when python started being necessary always as it's used to run llvm-build. Python 3 not being supported appears to come from commit r184732 which just says "All of LLVM's Python scripts only support Python 2 for widely understood reasons", though I must confess I don't know what those reasons are. I'll look into making extract_symbols.py work with python 3, though I haven't touched python 3 before now so I don't know how much luck I'll have.

Commit r274365 should make extract_symbols.py work with Python 3.

Ilod added a comment.Jul 2 2016, 9:51 AM

The PrintFunctionNames plugin works fine, but not the SampleAnalyzerPlugin? The checker-plugins test fails. Not sure, but I suppose it's because the regitration method for checkers is different (a C method called clang_registerCheckers is retrieved in the DLL from the main program, then called with a CheckerRegistry - which is not related with the Registry class - on which a templated method addChecker is called).

FAIL: Clang :: Analysis/checker-plugins.c (156 of 9493)
******************** TEST 'Clang :: Analysis/checker-plugins.c' FAILED ********************
Script:
--
Debug/bin/clang.EXE -cc1 -internal-isystem Debug\bin\..\lib\clang\3.9.0\include -nostdsysteminc -load Debug/bin/SampleAnalyzerPlugin.dll -analyze -analyzer-checker='example.MainCallChecker' -verify tools\clang\test\Analysis\checker-plugins.c
--
Exit Code: 1

Command Output (stdout):
--
$ "Debug/bin/clang.EXE" "-cc1" "-internal-isystem" "Debug\bin\..\lib\clang\3.9.0\include" "-nostdsysteminc" "-load" "Debug/bin/SampleAnalyzerPlugin.dll" "-analyze" "-analyzer-checker=example.MainCallChecker" "-verify" "tools\clang\test\Analysis\checker-plugins.c"
# command stderr:
CUSTOMBUILD : error : 'error' diagnostics seen but not expected:

  (frontend): no analyzer checkers are associated with 'example.MainCallChecker'

CUSTOMBUILD : error : 'warning' diagnostics expected but not seen:

  File tools\clang\test\Analysis\checker-plugins.c Line 9: call to main

CUSTOMBUILD : error : 'note' diagnostics seen but not expected:

    (frontend): use -analyzer-disable-all-checks to disable all static analyzer checkers

  3 errors generated.


CUSTOMBUILD : error : command failed with exit status: 1
In D21385#473163, @Ilod wrote:

The PrintFunctionNames plugin works fine, but not the SampleAnalyzerPlugin? The checker-plugins test fails.

Argh, I hadn't noticed that the plugin tests depend on the 'plugins' feature, and lit.cfg sets that based only based on BUILD_SHARED_LIBS, so I've accidentally been failing to run the tests. Thanks for noticing this.

Created D21971 to make the sample analyzer plugin work.

john.brawn updated this revision to Diff 62684.Jul 4 2016, 7:08 AM

Updated to not touch the sample analyzer plugin.

Ilod added a comment.Jul 8 2016, 5:23 AM

LGTM

Plugins tests are still disabled by lit.cfg I think, as it only checks for enable_shared, which is disabled. I suppose we could wait for both this and the SampleAnalyzerPlugin to be patched before enabling plugins tests if we have LLVM_EXPORT_SYMBOLS_FOR_PLUGINS, as every plugin test should work after this.

Plugins tests are still disabled by lit.cfg I think, as it only checks for enable_shared, which is disabled. I suppose we could wait for both this and the SampleAnalyzerPlugin to be patched before enabling plugins tests if we have LLVM_EXPORT_SYMBOLS_FOR_PLUGINS, as every plugin test should work after this.

D22221 for enabling the tests.

Also: you said LGTM, but didn't mark the review as accepted. OK to commit?

Ilod added a comment.Jul 12 2016, 2:56 AM

I don't have write access to SVN, so IIUC LLVM policy says I can't approve a patch.

klimek added a reviewer: rnk.Jul 26 2016, 8:12 AM

+Reid for a windows reviewer

rnk accepted this revision.Jul 26 2016, 12:59 PM
rnk edited edge metadata.

lgtm

This revision is now accepted and ready to land.Jul 26 2016, 12:59 PM
This revision was automatically updated to reflect the committed changes.
reames edited edge metadata.Aug 30 2016, 7:29 PM

This seems to have landed a couple of days ago without problem, but if anyone sees any weird effects in shared builds for Linux, this change is probably the culprit. The last time I tried to do something like this, I had to back out my change due to linker errors I never had the time to understand on some of the build bots.

llvm/trunk/include/llvm/Support/Registry.h
132

Minor style comment: Extracting the body of add_node into an add_node_impl which is still static and having the macro just pound out a wrapper would be a bit cleaner.