[libcxx] [test] Link against msvcprt as C++ ABI library in tests

[mstorsjo]

Summary

This library provides some parts of the C++ ABI bits needed when running on top of vcruntime.

This matches what we link the library itself against (set in CMakeLists.txt). When testing a static library version of libc++, this is needed for essentially every test due to libc++ object files requiring it.

Also with libc++ built as a DLL, some tests directly call functions that are provided by msvcprt (such as std::set_new_handler), thus this fixes a number of tests in that configuration too.

[Mordante]

In the description the first sentence "This matches what". what -> when?

I'm not a Windows expert, so won't approve. However I don't see a real issue with the change.

libcxx/utils/libcxx/test/config.py

464

Is msvcprt required here?

[mstorsjo]

In D99263#2648618, @Mordante wrote:

In the description the first sentence "This matches what". what -> when?

No, the sentence is the way I intended it to - "this matches what we do elsewhere"

libcxx/utils/libcxx/test/config.py

464

Not sure - I'm not entirely sure of the full actual setup where this would be exercised. It might not be relevant here actually.

[mstorsjo]

Ping - this makes the list of libraries linked here match the ones set in the main CMakeLists.txt, cf https://github.com/llvm/llvm-project/blob/d92b4956d6db8904f6a21c0e037dd5161b843b87/libcxx/CMakeLists.txt#L799-L802.

[curdeius]

LGTM.
At the first sight, I thought that the whole story of linking another C++ standard library to build/test another was strange, but finally, that makes sense (for the ABI), and it's not MSVC specific.

libcxx/utils/libcxx/test/config.py

433

Could you add a comment (somewhere) mentioning the corresponding flags in CMakeLists.txt, please?
They should probably be always in sync.

[mstorsjo]

libcxx/utils/libcxx/test/config.py

433

I could add a comment for the MSVC specific case that I'm editing. For the rest of this function, the whole concept of "add flags for linking ABI library" is handled quite differently between the main cmake build and the tests - for other ABI libraries, the necessary options are set in cmake/Modules/HandleLibCXXABI.cmake, while these MSVC specific ones are set in the toplevel CMakeLists.txt within an LIBCXX_TARGETING_MSVC case.

It's a bit iffy because part of these (ucrt, msvcrt) correspond to adding -lc and -lgcc (in toplevel CMakeLists.txt, to counter the fact that we're linking with -nodefaultlibs or -nostdlib), while others of them (msvcprt, maybe vcruntime) correspond to the C++ ABI library. Not entirely sure at what level it's "supported"/working to build for MSVC targets with a abi other than vcruntime.

There are people who build for the "windows-itanium" target, with an almost-msvc-like environment but with libcxxabi. I think some of such cases do it with some out-of-tree patches to libcxx though. Or maybe building libcxx with that configuration, with LIBCXX_CXX_ABI=libcxxabi works out of the box right now and we're just needlessly linking msvcprt (without actually ending up using anything from it?) in those cases, who knows. In any case, sorting out those bits is a rabbit hole I'd refrain from diving into at this point :-)

TL;DR, can add a comment for the MSVC specific case, but I'd refrain from making it too generic about things being more in sync than they really are.

[mstorsjo]

libcxx/utils/libcxx/test/config.py

433

Also, the ABI layering parts of "vcruntime" isn't exactly as clearcut and clean as one could want. If linking against DLL versions of that runtime, c++.dll ends up importing functions like` __std_terminate` and _CxxThrowException from vcruntime140.dll but things like __ExceptionPtrDestroy from msvcp140.dll (which is their full C++ lib).

So in one sense I guess the situation is like using libstdc++ as ABI library - as it's a full C++ library, but we only end up using the ABI bits from it.

[mstorsjo]

Ping for second approval

[mstorsjo]

Ping for second approval

[Mordante]

Since the changes only affect Windows and looks it looks like a general improvement. LGTM.

libcxx/test/std/language.support/support.dynamic/alloc.errors/set.new.handler/set_new_handler.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 // test set_new_handler
 
-// XFAIL: LIBCXX-WINDOWS-FIXME
-
 #include <new>
 #include <cassert>
 
 #include "test_macros.h"
 
 void f1() {}
 void f2() {}
 
 int main(int, char**)
 {
     assert(std::set_new_handler(f1) == 0);
     assert(std::set_new_handler(f2) == f1);
 
   return 0;
 }

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t.pass.cpp

 // UNSUPPORTED: apple-clang-9, apple-clang-10
 // UNSUPPORTED: clang-5, clang-6, clang-7
 // XFAIL: with_system_cxx_lib=macosx10.13
 // XFAIL: with_system_cxx_lib=macosx10.12
 // XFAIL: with_system_cxx_lib=macosx10.11
 // XFAIL: with_system_cxx_lib=macosx10.10
 // XFAIL: with_system_cxx_lib=macosx10.9
 
-// On Windows libc++ doesn't provide its own definitions for new/delete
-// but instead depends on the ones in VCRuntime. However VCRuntime does not
-// yet provide aligned new/delete definitions so this test fails to link.
-// XFAIL: LIBCXX-WINDOWS-FIXME
-
 // test operator new
 
 #include <new>
 #include <cstddef>
 #include <cassert>
 #include <cstdint>
 #include <limits>
 

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_align_val_t_nothrow.pass.cpp

 // UNSUPPORTED: apple-clang-9, apple-clang-10
 // UNSUPPORTED: clang-5, clang-6, clang-7
 // XFAIL: with_system_cxx_lib=macosx10.13
 // XFAIL: with_system_cxx_lib=macosx10.12
 // XFAIL: with_system_cxx_lib=macosx10.11
 // XFAIL: with_system_cxx_lib=macosx10.10
 // XFAIL: with_system_cxx_lib=macosx10.9
 
-// On Windows libc++ doesn't provide its own definitions for new/delete
-// but instead depends on the ones in VCRuntime. However VCRuntime does not
-// yet provide aligned new/delete definitions so this test fails to compile/link.
-// XFAIL: LIBCXX-WINDOWS-FIXME
-
 // test operator new (nothrow)
 
 #include <new>
 #include <cstddef>
 #include <cstdint>
 #include <cassert>
 #include <limits>
 

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_array.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 // test operator new[]
 // NOTE: asan and msan will not call the new handler.
 // UNSUPPORTED: sanitizer-new-delete
-// XFAIL: LIBCXX-WINDOWS-FIXME
 
 
 #include <new>
 #include <cstddef>
 #include <cassert>
 #include <limits>
 
 #include "test_macros.h"

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_array_nothrow.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 // test operator new [] (nothrow)
 // NOTE: asan and msan will not call the new handler.
 // UNSUPPORTED: sanitizer-new-delete
-// XFAIL: LIBCXX-WINDOWS-FIXME
 
 
 #include <new>
 #include <cstddef>
 #include <cassert>
 #include <limits>
 
 #include "test_macros.h"

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 // test operator new
 
 // asan and msan will not call the new handler.
 // UNSUPPORTED: sanitizer-new-delete
-// XFAIL: LIBCXX-WINDOWS-FIXME
 
 #include <new>
 #include <cstddef>
 #include <cassert>
 #include <limits>
 
 #include "test_macros.h"
 

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t.pass.cpp

 // XFAIL: with_system_cxx_lib=macosx10.12
 // XFAIL: with_system_cxx_lib=macosx10.11
 // XFAIL: with_system_cxx_lib=macosx10.10
 // XFAIL: with_system_cxx_lib=macosx10.9
 
 // asan and msan will not call the new handler.
 // UNSUPPORTED: sanitizer-new-delete
 
-// On Windows libc++ doesn't provide its own definitions for new/delete
-// but instead depends on the ones in VCRuntime. However VCRuntime does not
-// yet provide aligned new/delete definitions so this test fails to compile/link.
-// XFAIL: LIBCXX-WINDOWS-FIXME
-
 // test operator new
 
 #include <new>
 #include <cstddef>
 #include <cassert>
 #include <cstdint>
 #include <limits>
 

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_align_val_t_nothrow.pass.cpp

 // XFAIL: with_system_cxx_lib=macosx10.12
 // XFAIL: with_system_cxx_lib=macosx10.11
 // XFAIL: with_system_cxx_lib=macosx10.10
 // XFAIL: with_system_cxx_lib=macosx10.9
 
 // asan and msan will not call the new handler.
 // UNSUPPORTED: sanitizer-new-delete
 
-// On Windows libc++ doesn't provide its own definitions for new/delete
-// but instead depends on the ones in VCRuntime. However VCRuntime does not
-// yet provide aligned new/delete definitions so this test fails to compile/link.
-// XFAIL: LIBCXX-WINDOWS-FIXME
-
 // test operator new (nothrow)
 
 #include <new>
 #include <cstddef>
 #include <cstdint>
 #include <cassert>
 #include <limits>
 

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_nothrow.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 // test operator new (nothrow)
 
 // asan and msan will not call the new handler.
 // UNSUPPORTED: sanitizer-new-delete
-// XFAIL: LIBCXX-WINDOWS-FIXME
 
 #include <new>
 #include <cstddef>
 #include <cassert>
 #include <limits>
 
 #include "test_macros.h"
 

libcxx/utils/libcxx/test/config.py

def configure_link_flags_cxx_library(self):
                 libname = self.make_static_lib_name('c++')
                 abs_path = os.path.join(self.cxx_library_root, libname)
                 assert os.path.exists(abs_path) and \
                        "static libc++ library does not exist"
                 self.cxx.link_flags += [abs_path]
             else:
                 self.cxx.link_flags += ['-lc++']
 
     def configure_link_flags_abi_library(self):
         cxx_abi = self.get_lit_conf('cxx_abi', 'libcxxabi')
         if cxx_abi == 'libstdc++':
             self.cxx.link_flags += ['-lstdc++']
         elif cxx_abi == 'libsupc++':
             self.cxx.link_flags += ['-lsupc++']
         elif cxx_abi == 'libcxxabi':
             # If the C++ library requires explicitly linking to libc++abi, or
             # if we're testing libc++abi itself (the test configs are shared),
def configure_link_flags_abi_library(self):
                         abs_path = os.path.join(self.abi_library_root, libname)
                         self.cxx.link_flags += [abs_path]
                     else:
                         self.cxx.link_flags += ['-lc++abi']
         elif cxx_abi == 'libcxxrt':
             self.cxx.link_flags += ['-lcxxrt']
         elif cxx_abi == 'vcruntime':
             debug_suffix = 'd' if self.debug_build else ''
+            # This matches the set of libraries linked in the toplevel
+            # libcxx CMakeLists.txt if building targeting msvc.
             self.cxx.link_flags += ['-l%s%s' % (lib, debug_suffix) for lib in
-                                    ['vcruntime', 'ucrt', 'msvcrt']]
+                                    ['vcruntime', 'ucrt', 'msvcrt', 'msvcprt']]
         elif cxx_abi == 'none' or cxx_abi == 'default':
             if self.target_info.is_windows():
                 debug_suffix = 'd' if self.debug_build else ''
                 self.cxx.link_flags += ['-lmsvcrt%s' % debug_suffix]
         else:
             self.lit_config.fatal(
                 'C++ ABI setting %s unsupported for tests' % cxx_abi)
 
     def configure_extra_library_flags(self):
         if self.get_lit_bool('cxx_ext_threads', default=False):
             self.cxx.link_flags += ['-lc++external_threads']
         self.target_info.add_cxx_link_flags(self.cxx.link_flags)