Page MenuHomePhabricator

Add support for importing and exporting Registry objects on Windows
ClosedPublic

Authored by ehsan on Jan 31 2016, 3:57 PM.

Details

Summary

On Windows, the DLL containing the registry will get its own global head
and tail variables, so the entries registered in the DLL will be
invisible to the consumer.

In order to solve this, we need to export a getter function from the
plugin DLL per registry and copy over the data inside it. This patch
adds support for this. This will be used to support clang plugins on
Windows.

Turn on LLVM_ENABLE_PLUGINS by default on Windows

Diff Detail

Repository
rL LLVM

Event Timeline

ehsan updated this revision to Diff 46498.Jan 31 2016, 3:57 PM
ehsan retitled this revision from to Add support for importing and exporting Registry objects on Windows.
ehsan updated this object.
ehsan added a reviewer: rnk.
ehsan added a subscriber: llvm-commits.
rnk added inline comments.Feb 2 2016, 11:26 AM
include/llvm/Support/Registry.h
133 ↗(On Diff #46498)

This should have a comment. Basically, it's copying the entire registry linked list from another DLL into the current one.

163 ↗(On Diff #46498)

I think this should be LLVM_ON_WIN32. That's actually false for Cygwin and true for mingw. I'm not sure if these dllexport annotations will do the right thing with mingw, but let's find out.

172 ↗(On Diff #46498)

Do you think we should have an #else block here that defines these macros to nothing on Unix, so that clients don't have to do the platform ifdefs?

rnk edited edge metadata.Feb 2 2016, 11:28 AM

How do you deal with importing symbols from the clang exectuable? For Chromium, we statically link our plugins into the clang binary and let the registry do its thing that way.

In D16760#342008, @rnk wrote:

How do you deal with importing symbols from the clang exectuable? For Chromium, we statically link our plugins into the clang binary and let the registry do its thing that way.

Importing symbols on Windows is a hopeless effort. We'd either need to spray everything in clang with a CLANG_API macro that resolves to dllimport/dllexport depending on what you're building, or maintain a .def file with mangled names of everything we export, both of which are terrible solutions IMO. This setup is intended to be used with static linking:

  • You'll get static .lib libraries from LLVM and clang.
  • To build your plugin, you pass llvm-config --cxxflags to the compiler (and similarly llvm-config --ldflags --system-libs --libs whateveryouuse to the linker) which will cause you to link against clang statically.
  • The plugin DLL will create its own registry which gets imported into the main clang registry per this patch.
  • clang calls into the function from the DLL (discovered through the registry) so the plugin would be able to look at clang's data structures without the need to import symbols.

This should work for code symbols, but probably not for data symbols. The Head and Tail variables here are an obvious example. As we run into such issues, we'd need to add support for them on a case by case basis.

That being said, it looks like this setup is enough for using an existing plugin which examines the AST. I haven't tried it on anything more sophisticated yet...

include/llvm/Support/Registry.h
133 ↗(On Diff #46498)

Yep, will do.

163 ↗(On Diff #46498)

mingw supports __declspec(dllexport) IIRC. Whether or not this specific code will work for it is of course something we'll find out. I'll fix things if it breaks mingw...

172 ↗(On Diff #46498)

That sounds like a good idea. (FWIW I'm really unhappy that we needed to change the client side API for this, but sadly there is no way to avoid that.)

rnk added a comment.Feb 2 2016, 2:22 PM

Importing symbols on Windows is a hopeless effort. We'd either need to spray everything in clang with a CLANG_API macro that resolves to dllimport/dllexport depending on what you're building, or maintain a .def file with mangled names of everything we export, both of which are terrible solutions IMO. This setup is intended to be used with static linking:

CMake has some new auto-export functionality that I want to look into at some point. It didn't work immediately out of the box, so I had to put it aside for now, but eventually, I would like to stop statically linking clang.exe.

include/llvm/Support/Registry.h
172 ↗(On Diff #46498)

What if we kept the export side (the non-client side) the same and changed the Registry constructor to do dynamic lookup in all loaded DLLs for the exported symbol? Then we'd only have to change clang and opt.

In D16760#342227, @rnk wrote:

Importing symbols on Windows is a hopeless effort. We'd either need to spray everything in clang with a CLANG_API macro that resolves to dllimport/dllexport depending on what you're building, or maintain a .def file with mangled names of everything we export, both of which are terrible solutions IMO. This setup is intended to be used with static linking:

CMake has some new auto-export functionality that I want to look into at some point. It didn't work immediately out of the box, so I had to put it aside for now, but eventually, I would like to stop statically linking clang.exe.

Nice! I didn't know about that.

include/llvm/Support/Registry.h
172 ↗(On Diff #46498)

Do you mean the Registry constructor running in the plugin to look at all loaded modules for a AddNodeToRegistry function and call that instead of add_node() in this patch to add its registrations to the Registry in clang.exe? (I'm not sure how opt enters the picture here.)

ehsan added inline comments.Feb 5 2016, 2:43 PM
include/llvm/Support/Registry.h
172 ↗(On Diff #46498)

I tried the following:

  • Export a LLVMRegistery_${REGISTRY_NAME}_AddNode function from clang.exe.
  • Get Registry::Add() to look up the function above from all of the loaded modules using DynamicLibrary to find the correct version of the function no matter whether the call is being made from clang.exe or the plugin DLL.

The problem is that in order to look up the correct function name, we need to have the name of the Registry as a string at runtime. This means that given a client doing something like FrontendPluginRegistry::Add<MyAction>() needs to somehow be passing "FrontendPluginRegistry" as a string but that's not possible without changing the client code. And if we're going to change the client code, I prefer to keep using the current approach as it's more similar to the normal way of doing things on Windows.

ehsan updated this revision to Diff 47054.Feb 5 2016, 2:50 PM
ehsan edited edge metadata.

Addressed the review comments.

rnk accepted this revision.Feb 8 2016, 3:14 PM
rnk edited edge metadata.

lgtm

This revision is now accepted and ready to land.Feb 8 2016, 3:14 PM
This revision was automatically updated to reflect the committed changes.