[libcxx] [ci] Add a Windows CI configuration for a statically linked libc++

[mstorsjo]

Summary

On Windows, static vs DLL linking affects details in quite a few
cases, so it's good to have coverage for both cases.

Testing with static linking also increases coverage for a number of
cases and individual checks that have had to be waived for the DLL
case, and allows testing libc++experimental, increasing the number
of test cases actually executed by 180 (176 new tests from
libc++experimental and 4 ones that are XFAIL windows-dll).

[curdeius]

libcxx/utils/ci/buildkite-pipeline.yml

320

?

320–332

Didn't you intend to do it this way?

libcxx/utils/ci/run-buildbot

508

I'm not a big fan of having this if inside case. I'd rather go for factoring out common flags to a function.
The current way is harder to reason about IMO.

[mstorsjo]

libcxx/utils/ci/buildkite-pipeline.yml

320

Oh, sorry, yes you're right - I've missed in resolving git conflicts here after removing the bash path recently.

libcxx/utils/ci/run-buildbot

508

Ok, I can try to make it into a function.

[mstorsjo]

Kicking off a new CI run with the job names fixed, before attempting refactoring common settings to a function.

[mstorsjo]

Factorized the core of the cmake configuration to a function, removed the misplaced "generic-" prefix from the config names (these ones aren't "generic" in the same sense as the ones for e.g. generic posix configurations).

[curdeius]

LGTM. Nice!

[ldionne]

Adding this job LGTM, I'd like it written a bit differently (if my suggestions make sense).

libcxx/utils/ci/buildkite-pipeline.yml

321

Please use windows-dll instead of win-dll. Same for -static.

libcxx/utils/ci/run-buildbot

97–108

Could we instead do this to reuse as much between the Windows and non-Windows cases? I'm a bit worried that the two invocations are going to diverge with time.

generate-cmake -DCMAKE_CXX_FLAGS="-D_LIBCPP_HAS_NO_INT128" \
                   -DLIBCXX_TEST_COMPILER_FLAGS="-D_LIBCPP_HAS_NO_INT128" \
                   -DLIBCXX_ENABLE_FILESYSTEM=YES

Then, from below:

export CC=clang-cl
export CXX=clang-cl
generate-cmake-libcxx-windows -DOPTIONS

[ldionne]

Requesting changes for the queue.

[mstorsjo]

libcxx/utils/ci/buildkite-pipeline.yml

321

Sure, yes that's definitely better.

libcxx/utils/ci/run-buildbot

97–108

Yeah we could do something like that.

I'd have to split up the existing generate-cmake into a base configuration usable for the windows target though - using the current generate-cmake as such doesn't work for the clang-cl targets for a number of reasons.

The current generate-cmake configures cmake pointing at the llvm subdir. It's possible to build libcxx for windows that way, but it requires adding the option LLVM_FORCE_BUILD_RUNTIME (see llvm/projects/CMakeLists.txt). Additionally the current generate-cmake adds libunwind and libcxxabi to the build; libunwind fails to cmake configure for clang-cl targets, and adding libcxxabi also isn't buildable as such for clang-cl targets. Finally generate-cmake also adds LIBCXX_CXX_ABI=libcxxabi which isn't right for our configuration.

(As an aside - would it be good to have a CI configuration where libcxx is built on top of another ABI library like libsupc++?)

I kinda also prefer using LIBCXX_CXX_COMPILER=clang-cl instead of export CXX=clang-cl even though both would work.

[mstorsjo]

Rewrote generate-cmake-libcxx-win to share the basic cmake setup for the tests with the generate-cmake bash function. Changed the CI job names to spell out "windows-dll" and "windows-static".

[curdeius]

Still LGTM.

libcxx/utils/ci/run-buildbot

97–108

I kinda also prefer using LIBCXX_CXX_COMPILER=clang-cl instead of export CXX=clang-cl even though both would work.

+1.
Also, it has a minor (because doesn't apply to CI) advantage that CMake won't let you change the compiler by mistake in an already created build dir when using -DCMAKE_C_COMPILER, but will happily discard a change in CC/CXX env. vars.

[ldionne]

libcxx/utils/ci/run-buildbot

97–108

Yeah we could do something like that.

I'd have to split up the existing generate-cmake into a base configuration usable for the windows target though - using the current generate-cmake as such doesn't work for the clang-cl targets for a number of reasons.

I see.

The current generate-cmake configures cmake pointing at the llvm subdir. It's possible to build libcxx for windows that way, but it requires adding the option LLVM_FORCE_BUILD_RUNTIME (see llvm/projects/CMakeLists.txt). Additionally the current generate-cmake adds libunwind and libcxxabi to the build; libunwind fails to cmake configure for clang-cl targets, and adding libcxxabi also isn't buildable as such for clang-cl targets.

It's kind of lame that our Windows support is *that* bad.

Finally generate-cmake also adds LIBCXX_CXX_ABI=libcxxabi which isn't right for our configuration.

Indeed.

(As an aside - would it be good to have a CI configuration where libcxx is built on top of another ABI library like libsupc++?)

Yes, I think it would be useful since we claim it's supported.

I kinda also prefer using LIBCXX_CXX_COMPILER=clang-cl instead of export CXX=clang-cl even though both would work.

I think we should do this throughout the script then. I'll do it separately for other jobs.

[mstorsjo]

libcxx/utils/ci/run-buildbot

97–108

The current generate-cmake configures cmake pointing at the llvm subdir. It's possible to build libcxx for windows that way, but it requires adding the option LLVM_FORCE_BUILD_RUNTIME (see llvm/projects/CMakeLists.txt). Additionally the current generate-cmake adds libunwind and libcxxabi to the build; libunwind fails to cmake configure for clang-cl targets, and adding libcxxabi also isn't buildable as such for clang-cl targets.

It's kind of lame that our Windows support is *that* bad.

JFTR, it's not actually that our Windows support is bad.

Those two projects together implement the itanium C++ ABI, but MSVC configurations don't use the itanium ABI. MinGW environments use the itanium ABI on Windows, and there, libunwind and libcxxabi work just fine. So it's not that they're bad at what they're supposed to do, but what they do just doesn't make sense in an MSVC ABI environment. (That said, I guess one could want them to compile successfully in that environment regardless of whether they're usable.)

libcxx/test/libcxx/experimental/memory/memory.resource.adaptor/memory.resource.adaptor.mem/db_deallocate.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
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03
 
 // <experimental/memory_resource>
 
 // template <class T> class polymorphic_allocator
 
 // T* polymorphic_allocator<T>::deallocate(T*, size_t size)
 
+// XFAIL: LIBCXX-WINDOWS-FIXME
+
 int AssertCount = 0;
 
 #define _LIBCPP_ASSERT(x, m) ((x) ? (void)0 : (void)::AssertCount++)
 #define _LIBCPP_DEBUG 0
 #include <experimental/memory_resource>
 #include <type_traits>
 #include <cassert>
 

libcxx/utils/ci/buildkite-pipeline.yml

artifact_paths:
       - "**/test-results.xml"
     agents:
       queue: "libcxx-builders"
     retry:
       automatic:
         - exit_status: -1  # Agent was lost
           limit: 2
 
-  - label: "Windows"
-    command: "bash libcxx/utils/ci/run-buildbot generic-win"
+  - label: "Windows (DLL)"
+    command: "bash libcxx/utils/ci/run-buildbot win-dll"
+    artifact_paths:
+      - "**/test-results.xml"
+    agents:
+      queue: "windows"
+    retry:
+      automatic:
+        - exit_status: -1  # Agent was lost
+          limit: 2
+
+  - label: "Windows (Static)"
+    command: "bash libcxx/utils/ci/run-buildbot win-static"
     artifact_paths:
       - "**/test-results.xml"
     agents:
       queue: "windows"
     retry:
       automatic:
         - exit_status: -1  # Agent was lost
           limit: 2

libcxx/utils/ci/run-buildbot

cmake -S "${MONOREPO_ROOT}/llvm" \
           -DCMAKE_BUILD_TYPE=RelWithDebInfo \
           -DCMAKE_INSTALL_PREFIX="${INSTALL_DIR}" \
           -DLLVM_ENABLE_PROJECTS="libcxx;libunwind;libcxxabi" \
           -DLLVM_LIT_ARGS="-sv --show-unsupported --xunit-xml-output test-results.xml" \
           -DLIBCXX_CXX_ABI=libcxxabi \
           "${@}"
 }
 
+function generate-cmake-libcxx-win() {
+    # TODO: Clang-cl in MSVC configurations don't have access to compiler_rt
+    # builtins helpers for int128 division. See
+    # https://reviews.llvm.org/D91139#2429595 for a comment about longterm
+    # intent for handling the issue. In the meantime, define
+    # -D_LIBCPP_HAS_NO_INT128 (both when building the library itself and
+    # when building tests) to allow enabling filesystem for running tests,
+    # even if it uses a non-permanent ABI.
+
+    echo "--- Generating CMake"
+    cmake -S "${MONOREPO_ROOT}/libcxx" \
+          -B "${BUILD_DIR}" \
+          -GNinja -DCMAKE_MAKE_PROGRAM="${NINJA}" \
+          -DCMAKE_BUILD_TYPE=RelWithDebInfo \
+          -DCMAKE_C_COMPILER=clang-cl \
+          -DCMAKE_CXX_COMPILER=clang-cl \
+          -DLLVM_LIT_ARGS="-sv --show-unsupported --xunit-xml-output test-results.xml" \
+          -DLIBCXX_ENABLE_FILESYSTEM=YES \
+          -DCMAKE_CXX_FLAGS="-D_LIBCPP_HAS_NO_INT128" \
+          -DLIBCXX_TEST_COMPILER_FLAGS="-D_LIBCPP_HAS_NO_INT128" \
+          "${@}"
+}
+
 function check-cxx-cxxabi() {
     echo "--- Installing libc++ and libc++abi to a fake location"
     ${NINJA} -vC "${BUILD_DIR}" install-cxx install-cxxabi
 
     echo "+++ Running the libc++ tests"
     ${NINJA} -vC "${BUILD_DIR}" check-cxx
 
     echo "+++ Running the libc++abi tests"
armv7)
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Armv7Arm.cmake"
     check-cxx-cxxabi
 ;;
 armv7-noexceptions)
     clean
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Armv7Thumb-noexceptions.cmake"
     check-cxx-cxxabi
 ;;
-generic-win)
+win-dll)
     clean
-    # TODO: Clang-cl in MSVC configurations don't have access to compiler_rt
-    # builtins helpers for int128 division. See
-    # https://reviews.llvm.org/D91139#2429595 for a comment about longterm
-    # intent for handling the issue. In the meantime, define
-    # -D_LIBCPP_HAS_NO_INT128 (both when building the library itself and
-    # when building tests) to allow enabling filesystem for running tests,
-    # even if it uses a non-permanent ABI.
-
     # TODO: Currently, building with the experimental library breaks running
     # tests (the test linking look for the c++experimental library with the
     # wrong name, and the statically linked c++experimental can't be linked
     # correctly when libc++ visibility attributes indicate dllimport linkage
     # anyway), thus just disable the experimental library. Remove this
     # setting when cmake and the test driver does the right thing automatically.
-
-    echo "--- Generating CMake"
-    cmake -S "${MONOREPO_ROOT}/libcxx" \
-          -B "${BUILD_DIR}" \
-          -GNinja -DCMAKE_MAKE_PROGRAM="${NINJA}" \
-          -DCMAKE_BUILD_TYPE=RelWithDebInfo \
-          -DCMAKE_C_COMPILER=clang-cl \
-          -DCMAKE_CXX_COMPILER=clang-cl \
-          -DLLVM_LIT_ARGS="-sv --show-unsupported --xunit-xml-output test-results.xml" \
-          -DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=NO \
-          -DLIBCXX_ENABLE_FILESYSTEM=YES \
-          -DCMAKE_CXX_FLAGS="-D_LIBCPP_HAS_NO_INT128" \
-          -DLIBCXX_TEST_COMPILER_FLAGS="-D_LIBCPP_HAS_NO_INT128"
+    generate-cmake-libcxx-win -DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=OFF
+    echo "+++ Running the libc++ tests"
+    ${NINJA} -vC "${BUILD_DIR}" check-cxx
+;;
+win-static)
+    clean
+    generate-cmake-libcxx-win -DLIBCXX_ENABLE_SHARED=OFF
     echo "+++ Running the libc++ tests"
     ${NINJA} -vC "${BUILD_DIR}" check-cxx
 ;;
 *)
     echo "${BUILDER} is not a known configuration"
     exit 1
 ;;
 esac