[libcxx] [test] Fix running tests with Clang-cl in Debug mode

[mstorsjo]

Summary

When building in debug mode, the debug version of the MSVC CRT
gets linked in. (This is the default in CMake in general. In the case
of libcxx, we manually link the CRT though - and in debug mode,
we pick the debug version of the CRT.) When building the tests,
we need to use the same version of the CRT as was used for building
the library.

Additionally; the debug CRT defaults to pop up a dialog box when
asserts fail, which blocks running tests. By including the
set_windows_crt_report_mode.h helper header, we change the assert
behaviour back to that of release mode - printing a message and
exiting immediately.

This was supported by the old libcxx test system, where support for
it was added in 7e3ee09ad24cbca3ea7687c50b53be5269127fb1. When porting
over to the newer test setup, this mechanism wasn't brought over (and the
old test infrastructure was removed in
a48f018bb7d8fadc67c08e71409c31713daa0071).

Thus: In debug mode, link against the debug versions of msvcrt and
msvcprt, define _DEBUG (enabling CRT debug mode code patterns),
and include the set_windows_crt_report_mode.h header.

Based on a patch by Andrew Ng.

Linking of the debug version of the CRT can also be done by using
the new -fms-runtime-lib= Clang option. However that option was
added in Clang 16, and libcxx only requires Clang 15 for now;
therefore doing the CRT linking entirely manually for now (just as
before).

Additionally, adjust set_windows_crt_report_mode.h to avoid including
the body of the file when building in C mode or in C++03 mode.
This fixes the following two tests:

libcxx/include_as_c.sh.cpp
libcxx/selftest/dsl/dsl.sh.py

The former test is built in C mode. The latter tries compiling things
as C++03. Some of the vcruntime headers that we include break in
C++03 mode when MS CRT debug mode is enabled.

[Mordante]

In general LGTM, but one question.

libcxx/test/support/set_windows_crt_report_mode.h

13

Why do we need to test for C+98? If needed can you use our normal TEST_STD_VER macro?

[mstorsjo]

libcxx/test/support/set_windows_crt_report_mode.h

13

One testcase in libcxx/selftest/dsl/dsl.sh.py tests compiling in C++03 mode. And for some reason, some of the MSVC headers that get included from <crtdbg.h> break if compiled in C++03 mode.

I can look into changing this to include test_macros.h and checking TEST_STD_VER >= 11 instead. But since this header is force-included in every test compilation, we should ideally keep it as minimal as possible, so including test_macros.h might not be ideal (even if most tests themselves do include it). And I presume test_macros.h fail if included in C mode, so we possibly need to check against defined(__cplusplus) anyway?

[Mordante]

LGTM modulo some nits.

libcxx/test/support/set_windows_crt_report_mode.h

13

Thanks for the explanation. Can you add this information as comment. I'm fine with not including test_macros.h

43

[mstorsjo]

libcxx/test/support/set_windows_crt_report_mode.h

13

Ok, I'll amend this with a comment.

43

Thanks, will fix.

[andrewng]

libcxx/test/support/set_windows_crt_report_mode.h

36–38

Does this work for you? I had to change this in my patch otherwise the pop-up for which version of Visual Studio to use would come up and stall testing, IIRC. Perhaps this depends on some specific Windows environment/configuration?

[mstorsjo]

libcxx/test/support/set_windows_crt_report_mode.h

36–38

Yeah, it does seem to work for me as is. If it needs to be changed, we can do that in a separate change (ideally by figuring out why/when it’s needed), but this patch is enough for me (and the CI environment) for running the tests in debug mode.

I also tested specifically tests with failing asserts, and they get printed on the console as expected, without any blocking dialog.

[ldionne]

What would this patch look like if we supported Clang 16 and Clang 17 but not Clang 15? It would be a lot simpler right? I am asking because we're branching in less than a week, so it might be worth punting this change to after that.

Edit: Nevermind, I just saw D155562.

[mstorsjo]

In D155554#4516731, @ldionne wrote:

What would this patch look like if we supported Clang 16 and Clang 17 but not Clang 15? It would be a lot simpler right? I am asking because we're branching in less than a week, so it might be worth punting this change to after that.

Edit: Nevermind, I just saw D155562.

Yeah, indeed. Using the new Clang 16 option doesn't make a big difference (it actually makes for slightly more code changes), but it is structurally more of the right thing to do in the long run. So it doesn't really make any difference for this patch whether we have that allowed yet or not.

[andrewng]

libcxx/test/support/set_windows_crt_report_mode.h

36–38

According to the docs for _CrtSetReportMode, _CRTDBG_MODE_DEBUG means "Writes the message to the debugger's output window.". I'm guessing that what happens in this mode might depend on debugger settings. The code in my patch explicitly set _CrtSetReportMode to output to STDERR.

[mstorsjo]

libcxx/test/support/set_windows_crt_report_mode.h

36–38

I see, thanks. I can split out that bit and submit as a separate patch with that description then.

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 = ''
+lib_suffix = ''
+
+if '@uppercase_CMAKE_BUILD_TYPE@' == 'DEBUG':
+    dbg_include = ' -D_DEBUG -include set_windows_crt_report_mode.h'
+    lib_suffix = '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'
+    '-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
 ))
 config.substitutions.append(('%{link_flags}',
-    '-nostdlib -L %{lib} -lc++ -lmsvcrt -lmsvcprt -loldnames'
+    '-nostdlib -L %%{lib} -lc++ -lmsvcrt%s -lmsvcprt%s -loldnames' % (lib_suffix, lib_suffix)
 ))
 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 = ''
+lib_suffix = ''
+
+if '@uppercase_CMAKE_BUILD_TYPE@' == 'DEBUG':
+    dbg_include = ' -D_DEBUG -include set_windows_crt_report_mode.h'
+    lib_suffix = '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'
+    '-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
 ))
 config.substitutions.append(('%{link_flags}',
-    '-nostdlib -L %{lib} -lc++ -lmsvcrt -lmsvcprt -loldnames'
+    '-nostdlib -L %%{lib} -lc++ -lmsvcrt%s -lmsvcprt%s -loldnames' % (lib_suffix, lib_suffix)
 ))
 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 = ''
+lib_suffix = ''
+
+if '@uppercase_CMAKE_BUILD_TYPE@' == 'DEBUG':
+    dbg_include = ' -D_DEBUG -include set_windows_crt_report_mode.h'
+    lib_suffix = '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'
+    '-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
 ))
 config.substitutions.append(('%{link_flags}',
-    '-nostdlib -L %{lib} -llibc++ -lmsvcrt -lmsvcprt -loldnames'
+    '-nostdlib -L %%{lib} -llibc++ -lmsvcrt%s -lmsvcprt%s -loldnames' % (lib_suffix, lib_suffix)
 ))
 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
 )

libcxx/test/support/set_windows_crt_report_mode.h

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef SUPPORT_SET_WINDOWS_CRT_REPORT_MODE_H
 #define SUPPORT_SET_WINDOWS_CRT_REPORT_MODE_H
 
+// A few tests are built in C mode or in C++03 mode. The initialization
+// of init_crt_anchor is a C++ feature, and <crtdbg.h> ends up including
+// MSVC header code which breaks in C++03 mode. Therefore, only expand
+// the body of this header when included in C++ >= 11 mode. As this file
+// is included in every single translation unit, we're intentionally not
+// including test_macros.h (for TEST_STD_VER) but try to keep it to the
+// bare minimum.
+#if defined(__cplusplus) && __cplusplus > 199711L
 #ifndef _DEBUG
 #error _DEBUG must be defined when using this header
 #endif
 
 #ifndef _WIN32
 #error This header can only be used when targeting Windows
 #endif
 
 #include <crtdbg.h>
 
 // On Windows in debug builds the default assertion handler opens a new dialog
 // window which must be dismissed manually by the user. This function overrides
 // that setting and instead changes the assertion handler to log to stderr
 // instead.
 inline int init_crt_report_mode() {
   _CrtSetReportMode(_CRT_WARN, _CRTDBG_MODE_DEBUG);
   _CrtSetReportMode(_CRT_ERROR, _CRTDBG_MODE_DEBUG);
   _CrtSetReportMode(_CRT_ASSERT, _CRTDBG_MODE_DEBUG);
   return 0;
 }
 
 static int init_crt_anchor = init_crt_report_mode();
+#endif // defined(__cplusplus) && __cplusplus > 199711L
 
 #endif // SUPPORT_SET_WINDOWS_CRT_REPORT_MODE_H

libcxx/utils/ci/buildkite-pipeline.yml

- label: "Clang-cl (no vcruntime exceptions)"
         - "**/*.abilist"
       agents:
         queue: "windows"
       retry:
         automatic:
           - exit_status: -1  # Agent was lost
             limit: 2
 
+    - label: "Clang-cl (Debug mode)"
+      command: "bash libcxx/utils/ci/run-buildbot clang-cl-debug"
+      artifact_paths:
+        - "**/test-results.xml"
+        - "**/*.abilist"
+      agents:
+        queue: "windows"
+      retry:
+        automatic:
+          - exit_status: -1  # Agent was lost
+            limit: 2
+      timeout_in_minutes: 120
+
     - label: "MinGW (DLL, x86_64)"
       command: "bash libcxx/utils/ci/run-buildbot mingw-dll"
       artifact_paths:
         - "**/test-results.xml"
         - "**/*.abilist"
       agents:
         queue: "windows"
       retry:

libcxx/utils/ci/run-buildbot

clang-cl-no-vcruntime)
     # tests with -D_HAS_EXCEPTIONS=0, which users might set in certain
     # translation units while using libc++, even if libc++ is built with
     # exceptions enabled.
     generate-cmake-libcxx-win -DLIBCXX_TEST_PARAMS="enable_experimental=False" \
                               -DLIBCXX_TEST_CONFIG="llvm-libc++-shared-no-vcruntime-clangcl.cfg.in"
     echo "+++ Running the libc++ tests"
     ${NINJA} -vC "${BUILD_DIR}" check-cxx
 ;;
+clang-cl-debug)
+    clean
+    generate-cmake-libcxx-win -DLIBCXX_TEST_PARAMS="enable_experimental=False" \
+                              -DCMAKE_BUILD_TYPE=Debug
+    echo "+++ Running the libc++ tests"
+    ${NINJA} -vC "${BUILD_DIR}" check-cxx
+;;
 mingw-dll)
     clean
     # Explicitly specify the compiler with a triple prefix. The CI
     # environment has got two installations of Clang; the default one
     # defaults to MSVC mode, while there's an installation of llvm-mingw
     # further back in PATH. By calling the compiler with an explicit
     # triple prefix, we use the one that is bundled with a mingw sysroot.
     generate-cmake \