[test] Disable the Passes/PluginsTest cases on windows

[mstorsjo]

Summary

The plugin expects to have undefined references to symbols exported by the loading process, which isn't supported by shared libraries on windows.

Just disable the whole directory unconditionally on Windows, as the test essentially is a no-op there anyway.

[rnk]

llvm/unittests/Passes/CMakeLists.txt

4–5

It seems there is some prior reason this is done this way, so that reconfiguring a build directory from BUILD_SHARED_LIBS=ON to OFF and back rebuilds the executable. Otherwise lit test discovery will find it.

To this point, I think we should simply disable this whole directory on Windows. (if (WIN32)) One cannot switch from win32 to non-win32 in the same directory and expect things to work.

[mstorsjo]

Simplified the check to if (WIN32)

[thakis]

Maybe only disable this in shared library builds?

Also, having a return() in a cmake file looks kind of gross to me. (It's a local statement but has a file-global effect, and it's easy to miss. In C, returns are super common, but in cmake they very much aren't [1]) Maybe wrap the rest of the file in the if instead?

1: git ls-files '*CMake*' | xargs rg -l 'return\('-- 1 occurrence in clang, 3 in llvm

[mstorsjo]

In D74042#1864714, @thakis wrote:

Maybe only disable this in shared library builds?

That's what I did originally, but then @rnk suggested disabling it altogether, as there seemed to be existing efforts not to leave stale test executables if switching shared libs on/off

Also, having a return() in a cmake file looks kind of gross to me. (It's a local statement but has a file-global effect, and it's easy to miss. In C, returns are super common, but in cmake they very much aren't [1]) Maybe wrap the rest of the file in the if instead?

1: git ls-files '*CMake*' | xargs rg -l 'return\('-- 1 occurrence in clang, 3 in llvm

Sure, I could do that

[mstorsjo]

Don't use return() on the file level

[thakis]

In D74042#1865426, @mstorsjo wrote:

In D74042#1864714, @thakis wrote:

Maybe only disable this in shared library builds?

That's what I did originally, but then @rnk suggested disabling it altogether, as there seemed to be existing efforts not to leave stale test executables if switching shared libs on/off

Hm, I'm aware of one patch fixing something like that (by rnk), but I'm also aware of all the target unittest binaries being wrong on arch changes (rg stale llvm/utils/gn/ finds one comment about being careful about getting it right, and one FIXME on where we don't). So I'm not sure I fully buy that :)

Anyhoo, if Reid likes it more this way, then let's do it.

But now that I look at the gn build files again, one of them is for this very directory, and it already does what the cmake file does after your patch. So maybe it used to be this way when I wrote the gn build file and then someone undid it?

Looks like I made the plugin link in Support in https://reviews.llvm.org/D47082 , then https://reviews.llvm.org/rL342150 (unreviewed?) undid that and broke Windows again.

Anyhoo, lgtm I suppose.

llvm/unittests/Passes/CMakeLists.txt

 # Needed by LLVM's CMake checks because this file defines multiple targets.
 set(LLVM_OPTIONAL_SOURCES PluginsTest.cpp TestPlugin.cpp)
 
 # 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()
 
+# The plugin expects to not link against the Support and Core libraries
+# (otherwise there are multiple definitions of the same command line option
+# in the binary), but expects them to exist in the process loading the plugin.
+# This doesn't work with DLLs on Windows (where a shared library can't have
+# undefined references), so just skip this testcase in such configurations.
+if (WIN32 AND BUILD_SHARED_LIBS)
+  return()
+endif()
+
 set(LLVM_LINK_COMPONENTS Support Passes Core)
 add_llvm_unittest(PluginsTests
   PluginsTest.cpp
   )
 export_executable_symbols(PluginsTests)
 target_link_libraries(PluginsTests PRIVATE LLVMTestingSupport)
 
 set(LLVM_LINK_COMPONENTS)