This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [ci] Add a test configuration with an incomplete sysroot
Needs ReviewPublic

Authored by mstorsjo on May 17 2023, 3:02 AM.

Details

Reviewers
phosek
Group Reviewers
Restricted Project
Summary

When bringing up a new cross compiler from scratch, we build
libunwind/libcxx in a setup where the toolchain is incomplete and
unable to perform the normal linker checks; this requires a few
special cases in the CMake files.

We simulate that scenario by making a copy of the existing sysroot
and remove libunwind/libcxx from it, and point to this with the
--sysroot option passed via CMAKE_*_FLAGS_INIT.

We need to set CMAKE_CXX_COMPILER_WORKS since CMake fails to
probe the compiler. We need to set CMAKE_CXX_COMPILER_TARGET,
since LLVM's heuristics fail when CMake hasn't been able to
probe the environment properly. (This is normal; one has to
set those options when setting up such a toolchain from scratch.)

This adds CI coverage for these build scenarios, which otherwise
seldom are tested by some build flow (but are essential when setting
up a cross compiler from scratch).

Diff Detail

Event Timeline

mstorsjo created this revision.May 17 2023, 3:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2023, 3:02 AM
Herald added a subscriber: arichardson. · View Herald Transcript
mstorsjo requested review of this revision.May 17 2023, 3:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2023, 3:02 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
phosek added inline comments.May 17 2023, 10:39 AM
libcxx/utils/ci/run-buildbot
675–676

Can you use CMAKE_SYSROOT?

679

I'd set CMAKE_C_COMPILER_TARGET as well for consistency.

mstorsjo updated this revision to Diff 523136.May 17 2023, 12:29 PM

Use CMAKE_SYSROOT instead of CMAKE_*_FLAGS_INIT, set CMAKE_C_COMPILER_TARGET for consistency.

phosek accepted this revision.May 17 2023, 4:39 PM

LGTM

Ping @ldionne - I believe this should add extra CI coverage for situations that we otherwise don't have tested, which certainly would be useful for @phosek 's pending refactorings.

ldionne added inline comments.Jul 10 2023, 1:41 PM
libcxx/utils/ci/run-buildbot
648

Is there a reason why we're running this on MinGW? It would make more sense IMO to do this in our Docker image on Linux. At the end of the day, we are trying to test a general property of our CMake build, i.e. that it can be run even if you don't have access to a fully working C++ toolchain. This isn't Windows-specific, right?

675

I would set this unconditionally in runtimes/CMakeLists.txt with an appropriate comment. I don't think it ever makes sense for us to rely on the default CMake compiler checking.

681–683

Additionally, the sysroot flag from CMAKE_SYSROOT doesn't end up propagated into the tests

This shouldn't matter, since we're using -nostdlib++ and -nostdinc++ to avoid depending on any existing libc++ in the toolchain.

mstorsjo added inline comments.Jul 10 2023, 2:02 PM
libcxx/utils/ci/run-buildbot
648

Indeed it isn't Windows specific per se.

But in most cases on e.g. Linux, you'd already have a working toolchain with libgcc provided by the system. For the case of something that mostly behaves as a cross compiler (i.e. there's no such thing as a system provided libgcc or similar), it's easier to produce a stripped down sysroot and reach such a state.

The same goes for the actual bringup of a toolchain, where you'd have such an incomplete stage; when building a non-cross toolchain, you're usually already operating on top of a working environment, so you usually don't pass through that incomplete state.

(There's of course small nuances that do differ between systems wrt how they behave in an incomplete state, e.g. ELF usually is more tolerant against undefined symbols than what PE/COFF is.)

If configuring a toolchain where the compiler uses libunwind by default, and libunwind doesn't exist yet, then you'd have the same situation on Linux too. The main thing that is relevant is that you'd have a clang setup for a target where just invoking clang without any flags tries to link libunwind by default.

Regarding Docker - if I understood correctly, the current setup is that we're running Docker containers, which then run the buildkite agent within them, serving multiple build jobs within the same persistent Docker container. If the setup would be the reverse - each buildkite job spawning a new individual Docker container, then this setup would be easier to more properly simulate an incomplete situation. Instead of setting up and stripping down a sysroot, the actual system toolchain could be stripped in a matching fashion.

In other words: I was easily able to set this up, and this gives test coverage of the bringup phase of my toolchain. If soomeone else wants to try to set up a similar test scenario within the Linux test configs, feel free to, but for me, that'd be significantly more effort to set up.

The case that this covers is literally something that every one of @phosek's refactorings to runtimes/CMakeFiles.txt hits and needs to test.

675

Hmm, I wonder if it's possible (and generally advisable?) to do that. WDYT @phosek?

681–683

It's not so much about libc++ as it is about libunwind and all that. If the toolchain default sysroot is fully functional with a working libunwind, and the intentionally stripped down sysroot lacks libunwind, then testing without the --sysroot parameter loses that distinction, even if -nostdlib++ avoids using the default C++ library.

But in any case, this configuration is mostly interesting for compiling the runtimes, once built it should mostly behave as any other build.

phosek added inline comments.Jul 10 2023, 11:38 PM
libcxx/utils/ci/run-buildbot
675

We already set these in the bootstrapping build, see https://github.com/llvm/llvm-project/blob/e53da3eab42e6efd448c1c60c14668e1eb3d907c/llvm/runtimes/CMakeLists.txt#L239, but I'm not sure if setting this in runtimes/CMakeFiles.txt is the right approach since these should be only needed when building runtimes with an incomplete toolchain, and I'm worried if this could be a potential footgun.

So, FWIW, my view is that this patch would add valuable test coverage as-is.

It should be theoretically possible to set up something similar on linux as well, but it's not necessarily as straightforward (as it'd require setting up an environment that hard defaults to libunwind etc, to get the same test coverage as this gets). If it's a hard requirement for adding this kind of test scenario to have it running on Linux and not Windows, then I'd defer it until some time later when I have time to try to set it up. But as is, this patch gives lots of extra valuable test coverage.