Fix build warning compiling TestPlugin on Windows and disable Passes plugin stuff on Windows since it fundamentally can't work

[thakis]

Summary

Aaron Ballman reported that TestPlugin warned about it using exception handling without /EHsc flag, and that llvmGetPassInfo() had conflicting export attributes (dllimport in the header, dllexport in the source file).

/EHsc is because TestPlugin didn't use the llvm_ cmake functions, so llvm_update_compile_flags didn't get called for the target (llvm_update_compile_flags explicitly passes /Ehs-c-, which fixes the warning). Use add_llvm_loadable_module instead of add_library(... MODULE) to fix this. This also has the side effect of not building the plugin on Windows. That's not a big problem, since before the plugin was built on Windows, but the test didn't attempt to load it, due to -DLLVM_ENABLE_PLUGIN not being passed to PluginsTests.cpp during compilation on Windows. This makes the plugin behavior consistent with e.g. lib/Transforms/Hello/CMakeLists.txt. (This also automatically sets LTDL_SHLIB_EXT correctly.)

The dllimport/dllexport warning is more serious: Since LLVM doesn't generally use export annotations for its code, the only way the plugin could link was by linking in some LLVM libraries both into the test and the dll, so the plugin would call the llvm code in the dll instead of the copy in the main executable. This means globals weren't shared, and things generally can't work. (I think there's a build config where you can build a LLVM.dll which might work, but that wasn't how the test was configured. If that config is used, the dll should still be built, but I haven't checked).

Now that add_llvm_loadable_module is used, LLVM_LINK_COMPONENTS got linked into both executable and plugin on posix too, so unset it after the executable so that the plugin doesn't end up with a 2nd copy of things on posix.

[philip.pfaffe]

I'm likely not the right person to review this, I know next to nothing about windows dev.

On the other hand, legacy pass plugins are still enabled currently, and have been re-enabled only recently IIRC. Are those similarly fundamentally broken as well? Or is there anything different there that makes them work?

[aaron.ballman]

Adding some more Win32 folks. These changes look reasonable to me, but I'd like some confirmation.

Have plugins ever worked on Windows when building with MSVC? I seem to recall them being disabled there specifically because we don't export and import interfaces from DLLs properly, but I could be remembering wrong.

[philip.pfaffe]

There is this ancient commit here D18826 that introduced auto-export of all symbols. Shouldn't this work here as well?

[aaron.ballman]

In D47082#1105042, @philip.pfaffe wrote:

There is this ancient commit here D18826 that introduced auto-export of all symbols. Shouldn't this work here as well?

Blech, yes, I believe it could, but I agree with @rnk's original comments on that thread -- we should consider getting serious about properly annotating our API boundaries. Obviously, that is not in scope for this patch. ;-)

[philip.pfaffe]

I agree with @rnk, too, the question is do we want to enable this in the meantime?

[rnk]

lgtm

Thanks for resolving this!

In D47082#1104998, @aaron.ballman wrote:

Have plugins ever worked on Windows when building with MSVC? I seem to recall them being disabled there specifically because we don't export and import interfaces from DLLs properly, but I could be remembering wrong.

I don't think so. I think over the years people have done crazy stuff with dumpbin and LLVM_ENABLE_DYLIB (or whatever it's called) to make things kind of work, but it's never worked in any of the common build modes.

[philip.pfaffe]

Thanks for the clarification @rnk!

[aaron.ballman]

In D47082#1105085, @rnk wrote:

lgtm

Thanks for resolving this!

In D47082#1104998, @aaron.ballman wrote:

Have plugins ever worked on Windows when building with MSVC? I seem to recall them being disabled there specifically because we don't export and import interfaces from DLLs properly, but I could be remembering wrong.

I don't think so. I think over the years people have done crazy stuff with dumpbin and LLVM_ENABLE_DYLIB (or whatever it's called) to make things kind of work, but it's never worked in any of the common build modes.

Okay, that's what I thought. Thank you for the confirmation -- patch LGTM as well.

[thakis]

r332796, thanks!

include/llvm/Demangle/Compiler.h

 #define LLVM_ENABLE_EXCEPTIONS 1
 #endif
 
-/// \macro LLVM_PLUGIN_IMPORT
-/// Used to import the well-known entry point for registering loaded pass
-/// plugins
-#ifdef WIN32
-#define LLVM_PLUGIN_IMPORT __declspec(dllimport)
-#else
-#define LLVM_PLUGIN_IMPORT
 #endif
-
-/// \macro LLVM_PLUGIN_EXPORT
-/// Used to export the well-known entry point for registering loaded pass
-/// plugins
-#ifdef WIN32
-#define LLVM_PLUGIN_EXPORT __declspec(dllexport)
-#else
-#define LLVM_PLUGIN_EXPORT
-#endif
-
-#endif

include/llvm/Passes/PassPlugin.h

 ///
 /// ```
 /// extern "C" ::llvm::PassPluginLibraryInfo LLVM_ATTRIBUTE_WEAK
-/// LLVM_PLUGIN_EXPORT llvmGetPassPluginInfo() {
+/// llvmGetPassPluginInfo() {
 ///   return {
 ///     LLVM_PLUGIN_API_VERSION, "MyPlugin", "v0.1", [](PassBuilder &PB) { ... }
 ///   };
 /// }
 /// ```
-extern "C" ::llvm::PassPluginLibraryInfo LLVM_ATTRIBUTE_WEAK LLVM_PLUGIN_IMPORT
+extern "C" ::llvm::PassPluginLibraryInfo LLVM_ATTRIBUTE_WEAK
 llvmGetPassPluginInfo();
 
 #endif /* LLVM_PASSES_PASSPLUGIN_H */

unittests/Passes/CMakeLists.txt

 # Needed by LLVM's CMake checks because this file defines multiple targets.
-set(LLVM_OPTIONAL_SOURCES TestPlugin.cpp)
+set(LLVM_OPTIONAL_SOURCES PluginsTest.cpp TestPlugin.cpp)
 
-set(LLVM_LINK_COMPONENTS Support Passes Core)
-
 # If plugins are disabled, this test will disable itself at runtime. Otherwise,
 # reconfiguring with plugins disabled will leave behind a stale executable.
 if (LLVM_ENABLE_PLUGINS)

   add_definitions(-DLLVM_ENABLE_PLUGINS)
 endif()
 
+set(LLVM_LINK_COMPONENTS Support Passes Core)
 add_llvm_unittest(PluginsTests
   PluginsTest.cpp
   )
 export_executable_symbols(PluginsTests)
 
-add_library(TestPlugin MODULE
+set(LLVM_LINK_COMPONENTS)
+add_llvm_loadable_module(TestPlugin
   TestPlugin.cpp
   )
 
+# Put plugin next to the unit test executable.
 set_output_directory(TestPlugin
   BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}
   LIBRARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}
   )
-
-set_target_properties(TestPlugin
-  PROPERTIES PREFIX ""
-  SUFFIX ${LTDL_SHLIB_EXT}
-  )
 set_target_properties(TestPlugin PROPERTIES FOLDER "Tests")
 
-if (WIN32 OR CYGWIN OR LLVM_EXPORT_SYMBOLS_FOR_PLUGINS)
-  llvm_map_components_to_libnames(LLVM_DEPS ${LLVM_LINK_COMPONENTS})
-  target_link_libraries(TestPlugin ${LLVM_DEPS})
-endif()
-
 add_dependencies(TestPlugin intrinsics_gen)
 add_dependencies(PluginsTests TestPlugin)

unittests/Passes/TestPlugin.cpp

       });
 }
 
-extern "C" ::llvm::PassPluginLibraryInfo LLVM_ATTRIBUTE_WEAK LLVM_PLUGIN_EXPORT
+extern "C" ::llvm::PassPluginLibraryInfo LLVM_ATTRIBUTE_WEAK
 llvmGetPassPluginInfo() {
   return {LLVM_PLUGIN_API_VERSION, TEST_PLUGIN_NAME, TEST_PLUGIN_VERSION,
           registerCallbacks};