[libcxx] [test] Deduplicate setting parameters for clangcl test configs. NFC.

[mstorsjo]

Summary

This patch is written on top of D155561 and D155562, but it can be
rebased and applied first, with the other two coming in on top of
this as well.

I'm undecided about whether this is a good thing or not. The duplicated
logic is clearly not great. But this on the other hand includes a lot
of very obscure variable setting that only makes sense in the context
of the *-clangcl.cfg.in files.

[Mordante]

Can you rebase to see whether that fixes the CI. There seems to be an issue in the modular build, which I expect is unrelated.

[mstorsjo]

In D156573#4544303, @Mordante wrote:

Can you rebase to see whether that fixes the CI. There seems to be an issue in the modular build, which I expect is unrelated.

Ok, will do - I’ll probably have to rebase the two dependencies to get the right result.

[mstorsjo]

Rebased

[Mordante]

No objections from my perspective. Please wait for @andrewng's approval before landing.

[mstorsjo]

In D156573#4544760, @Mordante wrote:

No objections from my perspective. Please wait for @andrewng's approval before landing.

Any preference on landing (and adapting) this to go in before the two dependencies (D155561 and D155562), or keeping it where it is now afterwards? I can adapt it so this goes in first too. The second one of those dependencies isn’t approved yet (and depends on the requirement being raised to clang 16, which I guess we can do now that the branch is made - but holding off of that for a while might ease backporting).

[andrewng]

LGTM and is definitely an improvement. However, there's still quite a bit of duplicated content. Perhaps the templates could be merged and the other differences put into variables? Although that can be a separate patch.

Any preference on landing (and adapting) this to go in before the two dependencies (D155561 and D155562), or keeping it where it is now afterwards? I can adapt it so this goes in first too. The second one of those dependencies isn’t approved yet (and depends on the requirement being raised to clang 16, which I guess we can do now that the branch is made - but holding off of that for a while might ease backporting).

I think it would be nice to get this in before D155562, just to avoid the duplication within the patch itself. But I don't think it's a requirement.

By the way, thanks for breaking up my patch and getting most of it submitted. I wouldn't have been able to find the time to do it for quite a while!

[mstorsjo]

In D156573#4547076, @andrewng wrote:

LGTM and is definitely an improvement. However, there's still quite a bit of duplicated content. Perhaps the templates could be merged and the other differences put into variables? Although that can be a separate patch.

Yep, there is, but most of that is kinda static content (and doesn't interact with the cmake configuration options like this). OTOH I guess it could be possible to merge them into one and have most of the logic be controlled by cmake variables, but that's kinda bending the design of the current config file based test setup, so I'd prefer not to wander into that right now, but keeping the scope limited.

Any preference on landing (and adapting) this to go in before the two dependencies (D155561 and D155562), or keeping it where it is now afterwards? I can adapt it so this goes in first too. The second one of those dependencies isn’t approved yet (and depends on the requirement being raised to clang 16, which I guess we can do now that the branch is made - but holding off of that for a while might ease backporting).

I think it would be nice to get this in before D155562, just to avoid the duplication within the patch itself. But I don't think it's a requirement.

Ok, I'll rebase it that way.

By the way, thanks for breaking up my patch and getting most of it submitted. I wouldn't have been able to find the time to do it for quite a while!

No problem, I was interested in getting this fixed anyway - and it's kinda easy when having some idea of how I wanted to get it done.

[mstorsjo]

Rebased to place this commit first.

[mstorsjo]

Add the missed -D_DEBUG which was lost in the refactoring earlier

[mstorsjo]

Rebase, hoping for a successful CI run

libcxx/test/CMakeLists.txt

foreach(param IN LISTS LIBCXX_TEST_PARAMS)
   string(REGEX REPLACE "(.+)=(.+)" "\\2" value "${param}")
   serialize_lit_param("${name}" "\"${value}\"")
 endforeach()
 
 if (NOT DEFINED LIBCXX_TEST_DEPS)
   message(FATAL_ERROR "Expected LIBCXX_TEST_DEPS to be defined")
 endif()
 
+if (MSVC)
+  # Shared code for initializing some parameters used by all
+  # llvm-libc++-*-clangcl.cfg.in test configs.
+  set(dbg_include "")
+
+  if (NOT CMAKE_MSVC_RUNTIME_LIBRARY OR CMAKE_MSVC_RUNTIME_LIBRARY MATCHES "DLL$")
+    set(crt_lib "msvcrt")
+    set(cxx_lib "msvcprt")
+  else()
+    set(crt_lib "libcmt")
+    set(cxx_lib "libcpmt")
+  endif()
+
+  if (uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG")
+    set(dbg_include " -D_DEBUG -include set_windows_crt_report_mode.h")
+    set(crt_lib "${crt_lib}d")
+    set(cxx_lib "${cxx_lib}d")
+  endif()
+
+  serialize_lit_param(dbg_include "\"${dbg_include}\"")
+  serialize_lit_param(crt_lib "\"${crt_lib}\"")
+  serialize_lit_param(cxx_lib "\"${cxx_lib}\"")
+endif()
+
 if (LIBCXX_INCLUDE_TESTS)
   include(AddLLVM) # for configure_lit_site_cfg and add_lit_testsuite
 
   configure_file("${CMAKE_CURRENT_SOURCE_DIR}/configs/cmake-bridge.cfg.in"
                  "${CMAKE_CURRENT_BINARY_DIR}/cmake-bridge.cfg"
                  @ONLY)
 
   configure_lit_site_cfg(

libcxx/test/configs/llvm-libc++-shared-clangcl.cfg.in

 # This testing configuration handles running the test suite against LLVM's libc++
 # using a DLL, with Clang-cl on Windows.
 
 lit_config.load_config(config, '@CMAKE_CURRENT_BINARY_DIR@/cmake-bridge.cfg')
 
-dbg_include = ''
-runtime_library = '@CMAKE_MSVC_RUNTIME_LIBRARY@'
-
-if runtime_library == '' or runtime_library.endswith('DLL'):
-    crt_lib = 'msvcrt'
-    cxx_lib = 'msvcprt'
-else:
-    crt_lib = 'libcmt'
-    cxx_lib = 'libcpmt'
-
-if '@uppercase_CMAKE_BUILD_TYPE@' == 'DEBUG':
-    dbg_include = ' -D_DEBUG -include set_windows_crt_report_mode.h'
-    crt_lib += 'd'
-    cxx_lib += 'd'
-
 config.substitutions.append(('%{flags}', '--driver-mode=g++'))
 config.substitutions.append(('%{compile_flags}',
-    '-nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX' + dbg_include
+    '-nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX' + config.dbg_include
 ))
 config.substitutions.append(('%{link_flags}',
-    '-nostdlib -L %%{lib} -lc++ -l%s -l%s -loldnames' % (crt_lib, cxx_lib)
+    '-nostdlib -L %%{lib} -lc++ -l%s -l%s -loldnames' % (config.crt_lib, config.cxx_lib)
 ))
 config.substitutions.append(('%{exec}',
     '%{executor} --execdir %T --prepend_env PATH=%{lib} -- '
 ))
 
 import os, site
 site.addsitedir(os.path.join('@LIBCXX_SOURCE_DIR@', 'utils'))
 import libcxx.test.params, libcxx.test.config
 libcxx.test.config.configure(
     libcxx.test.params.DEFAULT_PARAMETERS,
     libcxx.test.features.DEFAULT_FEATURES,
     config,
     lit_config
 )

libcxx/test/configs/llvm-libc++-shared-no-vcruntime-clangcl.cfg.in

 # This testing configuration handles running the test suite against LLVM's libc++
 # using a DLL, with Clang-cl on Windows. This variant sets _HAS_EXCEPTIONS = 0
 # which removes exception class definitions from the vcruntime.
 
 lit_config.load_config(config, '@CMAKE_CURRENT_BINARY_DIR@/cmake-bridge.cfg')
 
-dbg_include = ''
-runtime_library = '@CMAKE_MSVC_RUNTIME_LIBRARY@'
-
-if runtime_library == '' or runtime_library.endswith('DLL'):
-    crt_lib = 'msvcrt'
-    cxx_lib = 'msvcprt'
-else:
-    crt_lib = 'libcmt'
-    cxx_lib = 'libcpmt'
-
-if '@uppercase_CMAKE_BUILD_TYPE@' == 'DEBUG':
-    dbg_include = ' -D_DEBUG -include set_windows_crt_report_mode.h'
-    crt_lib += 'd'
-    cxx_lib += 'd'
-
 config.substitutions.append(('%{flags}', '--driver-mode=g++'))
 config.substitutions.append(('%{compile_flags}',
-    '-nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX -D_HAS_EXCEPTIONS=0' + dbg_include
+    '-nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX -D_HAS_EXCEPTIONS=0' + config.dbg_include
 ))
 config.substitutions.append(('%{link_flags}',
-    '-nostdlib -L %%{lib} -lc++ -l%s -l%s -loldnames' % (crt_lib, cxx_lib)
+    '-nostdlib -L %%{lib} -lc++ -l%s -l%s -loldnames' % (config.crt_lib, config.cxx_lib)
 ))
 config.substitutions.append(('%{exec}',
     '%{executor} --execdir %T --prepend_env PATH=%{lib} -- '
 ))
 
 import os, site
 site.addsitedir(os.path.join('@LIBCXX_SOURCE_DIR@', 'utils'))
 import libcxx.test.params, libcxx.test.config
 libcxx.test.config.configure(
     libcxx.test.params.DEFAULT_PARAMETERS,
     libcxx.test.features.DEFAULT_FEATURES,
     config,
     lit_config
 )

libcxx/test/configs/llvm-libc++-static-clangcl.cfg.in

 # This testing configuration handles running the test suite against LLVM's libc++
 # using a static library, with Clang-cl on Windows.
 
 lit_config.load_config(config, '@CMAKE_CURRENT_BINARY_DIR@/cmake-bridge.cfg')
 
-dbg_include = ''
-runtime_library = '@CMAKE_MSVC_RUNTIME_LIBRARY@'
-
-if runtime_library == '' or runtime_library.endswith('DLL'):
-    crt_lib = 'msvcrt'
-    cxx_lib = 'msvcprt'
-else:
-    crt_lib = 'libcmt'
-    cxx_lib = 'libcpmt'
-
-if '@uppercase_CMAKE_BUILD_TYPE@' == 'DEBUG':
-    dbg_include = ' -D_DEBUG -include set_windows_crt_report_mode.h'
-    crt_lib += 'd'
-    cxx_lib += 'd'
-
 config.substitutions.append(('%{flags}', '--driver-mode=g++'))
 config.substitutions.append(('%{compile_flags}',
-    '-nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX' + dbg_include
+    '-nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX' + config.dbg_include
 ))
 config.substitutions.append(('%{link_flags}',
-    '-nostdlib -L %%{lib} -llibc++ -l%s -l%s -loldnames' % (crt_lib, cxx_lib)
+    '-nostdlib -L %%{lib} -llibc++ -l%s -l%s -loldnames' % (config.crt_lib, config.cxx_lib)
 ))
 config.substitutions.append(('%{exec}',
     '%{executor} --execdir %T -- '
 ))
 
 import os, site
 site.addsitedir(os.path.join('@LIBCXX_SOURCE_DIR@', 'utils'))
 import libcxx.test.params, libcxx.test.config
 libcxx.test.config.configure(
     libcxx.test.params.DEFAULT_PARAMETERS,
     libcxx.test.features.DEFAULT_FEATURES,
     config,
     lit_config
 )