[PATCH] D16760: Add support for importing and exporting Registry objects on Windows

[Ehsan Akhgari via llvm-commits]

ehsan added a comment.

In http://reviews.llvm.org/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...


================
Comment at: include/llvm/Support/Registry.h:133
@@ +132,3 @@
+      if (Getter) {
+        typedef std::pair<const node *, const node *> Info;
+        Info *I = static_cast<Info *>(Getter());
----------------
rnk wrote:
> This should have a comment. Basically, it's copying the entire registry linked list from another DLL into the current one.
Yep, will do.

================
Comment at: include/llvm/Support/Registry.h:163
@@ -128,1 +162,3 @@
 
+#ifdef _MSC_VER
+#define LLVM_EXPORT_REGISTRY(REGISTRY_CLASS)                                   \
----------------
rnk wrote:
> 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.
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...

================
Comment at: include/llvm/Support/Registry.h:172
@@ +171,3 @@
+  REGISTRY_CLASS::import(DL, #REGISTRY_CLASS)
+#endif
+
----------------
rnk wrote:
> 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?
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.)


http://reviews.llvm.org/D16760