This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by mstorsjo on Apr 29 2021, 1:04 PM.

Details

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).

Diff Detail

Event Timeline

mstorsjo created this revision.Apr 29 2021, 1:04 PM
mstorsjo requested review of this revision.Apr 29 2021, 1:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2021, 1:04 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
curdeius added inline comments.
libcxx/utils/ci/buildkite-pipeline.yml
332

?

332–344

Didn't you intend to do it this way?

libcxx/utils/ci/run-buildbot
522

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 added inline comments.Apr 29 2021, 1:56 PM
libcxx/utils/ci/buildkite-pipeline.yml
332

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
522

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

mstorsjo updated this revision to Diff 341633.Apr 29 2021, 1:58 PM

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

mstorsjo updated this revision to Diff 341805.Apr 30 2021, 1:46 AM

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 accepted this revision as: curdeius.Apr 30 2021, 4:38 AM

LGTM. Nice!

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

libcxx/utils/ci/buildkite-pipeline.yml
333

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

libcxx/utils/ci/run-buildbot
106–117

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 requested changes to this revision.Apr 30 2021, 2:01 PM

Requesting changes for the queue.

This revision now requires changes to proceed.Apr 30 2021, 2:01 PM
mstorsjo added inline comments.May 2 2021, 12:38 PM
libcxx/utils/ci/buildkite-pipeline.yml
333

Sure, yes that's definitely better.

libcxx/utils/ci/run-buildbot
106–117

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 updated this revision to Diff 342258.May 2 2021, 12:39 PM

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".

Still LGTM.

libcxx/utils/ci/run-buildbot
106–117

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 accepted this revision.May 5 2021, 10:34 AM
ldionne added inline comments.
libcxx/utils/ci/run-buildbot
106–117

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.

This revision is now accepted and ready to land.May 5 2021, 10:34 AM
mstorsjo added inline comments.May 5 2021, 12:22 PM
libcxx/utils/ci/run-buildbot
106–117

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.)