Page MenuHomePhabricator

[libc++] Include <__config_site> from <__config>
ClosedPublic

Authored by phosek on Feb 26 2021, 12:27 PM.

Details

Reviewers
ldionne
jdoerfert
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Commits
rGc06a8f9caa51: [libc++] Include <__config_site> from <__config>
Summary

Prior to this patch, we would generate a fancy <config> header by
concatenating <
config_site> and <__config>. This complexifies the
build system and also increases the difference between what's tested
and what's actually installed.

This patch removes that complexity and instead simply installs <config_site>
alongside the libc++ headers. <
config_site> is then included by <config>,
which is much simpler. Doing this also opens the door to having different
<
config_site> headers depending on the target, which was impossible before.

It does change the workflow for testing header-only changes to libc++.
Previously, we would run lit against the headers in libcxx/include.
After this patch, we run it against a fake installation root of the
headers (containing a proper <__config_site> header). This makes use
closer to testing what we actually install, which is good, however it
does mean that we have to update that root before testing header changes.
Thus, we now need to run ninja check-cxx-deps before running lit by
hand.

This commit was originally applied in 5d796645d and reverted in 48e4b0fd3
because it broke several bots. The issues should have been addressed now.

Diff Detail

Event Timeline

phosek created this revision.Feb 26 2021, 12:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 26 2021, 12:27 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
phosek requested review of this revision.Feb 26 2021, 12:27 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added subscribers: Restricted Project, sstefan1. · View Herald Transcript

@ldionne this is an attempt at reviving D89041, I tested this locally and it seems to be working, at least for the Fuchsia runtimes build. It might break out some parts of this patch to make it more manageable, like the s/-isystem/-cxx-isystem/ in compiler-rt.

I think it would be great to try it out again. To be honest, I've been meaning to try it out again (I had a similar patch locally), but I've been too scared to do so. Last time the world just exploded and I spent like a week trying to make heads or tail of the situation.

I'm entirely favorable to doing this, but can we try splitting out changes as much as possible? Ideally, we'd land everything before, and then at the end we'd land the libc++ changes, and only those.

Also, it would be awesome to set up a true runtimes build on our pre-commit CI infrastructure. The GCE instances we use could probably handle the load, they're very beefy. Would you be willing to help set that up?

curdeius added inline comments.
libcxx/cmake/Modules/HandleLibCXXABI.cmake
54–55

If it's always true, then the condition should be removed, no?

ldionne added inline comments.Mar 1 2021, 8:03 AM
libcxx/cmake/Modules/HandleLibCXXABI.cmake
54–55

I think at the time it was an attempt to make the diff as small as possible, since that change was already very risky. If we manage to split this out into smaller patches, then yeah, I think we could probably just remove this if and unconditionally execute its body.

phosek updated this revision to Diff 329712.Mar 10 2021, 10:55 AM

Minor drive-by comments. I'm extremely unlikely to voice either approval or disapproval in an official capacity. :)

libcxx/CMakeLists.txt
407

This comment is scary but uninformative. What "other projects" do you mean? By "these dependencies" do you mean the dependency from those other projects to the specific string "c++/v1" (in which case surely you should say They not We)? or by "these dependencies" do you mean one or more of lines 408-424?
Do you mean They should use LIBCXX_GENERATED_INCLUDE_DIR instead of hard-coding include/c++/v1?

libcxx/docs/TestingLibcxx.rst
44

Is running [ninja|make] cxx specifically not enough? (I'm guessing the answer is "yes, not enough.") That answer should be called out explicitly in the text.

libcxxabi/CMakeLists.txt
173 ↗(On Diff #329712)

Lines 170 and 172 are identical except for /I versus -I. I know Microsoft's command-line compiler driver supports -I as a synonym for /I. So could we combine these five lines into one line using -I in both cases? Or is the slash character required because it's magic to CMake, or something like that?

phosek updated this revision to Diff 332838.Mar 23 2021, 6:00 PM
phosek marked an inline comment as done.
phosek marked an inline comment as done.
phosek added inline comments.
libcxx/docs/TestingLibcxx.rst
44

This section specifically is about using llvm-lit directly, if you're using ninja or make you can just run check-cxx.

phosek updated this revision to Diff 332839.Mar 23 2021, 6:05 PM
phosek marked an inline comment as done.

@ldionne I've already landed all changes that could be meaningfully carved out, would you be OK if we tried to land this again?

phosek updated this revision to Diff 332879.Mar 24 2021, 12:37 AM

Looks like presubmit checks are passing now.

Looks like presubmit checks are passing now.

I'm pretty sure we'll need the cxx-headers change in libcxxabi (D98367) to make this change, no? Otherwise, we might break some people building and testing libc++abi in funky ways. I'd also wait until https://reviews.llvm.org/D97888 has landed so that the pre-commit tests include the Runtimes build. But except for that, I think this is good and I'm excited/scared to try this again.

phosek updated this revision to Diff 334222.Mar 30 2021, 10:39 AM

Looks like presubmit checks are passing now.

I'm pretty sure we'll need the cxx-headers change in libcxxabi (D98367) to make this change, no? Otherwise, we might break some people building and testing libc++abi in funky ways. I'd also wait until https://reviews.llvm.org/D97888 has landed so that the pre-commit tests include the Runtimes build. But except for that, I think this is good and I'm excited/scared to try this again.

This should be ready to land now.

ldionne accepted this revision.Mar 30 2021, 12:39 PM

LGTM with nitpick (and assuming CI comes back green).

I still think we should watch out for any unintended consequence of this change. Basically, anyone including libcxx/include instead of the properly installed headers will now run into issues after this change (since __config_site will not have been included and thus won't be found). That's why this change has so much possibility for breakage. However, once that's fixed, we'll be in a much better place knowing that all users are using the correctly installed (and hence configured) libc++ headers.

libcxx/docs/TestingLibcxx.rst
42

This target name has been renamed since the original patch. It's now cxx-test-depends.

phosek updated this revision to Diff 334267.Mar 30 2021, 2:05 PM
phosek marked an inline comment as done.
This revision was not accepted when it landed; it landed in state Needs Review.Mar 30 2021, 2:06 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

We're seeing two test failures on macOS in bootstrap builds after this: https://bugs.chromium.org/p/chromium/issues/detail?id=1194745

We're seeing two test failures on macOS in bootstrap builds after this: https://bugs.chromium.org/p/chromium/issues/detail?id=1194745

D99706 should hopefully address this.

muiez added a subscriber: muiez.Mon, Apr 26, 7:32 AM
muiez added inline comments.
libcxx/cmake/Modules/HandleLibCXXABI.cmake
57

Wouldn't this copy the libc++abi headers to include/c++/v1/include/c++/v1/ (on z/OS for example)? I ask because LIBCXX_GENERATED_INCLUDE_DIR is already set to ${LLVM_BINARY_DIR}/include/c++/v1 above. Nonetheless, the old change copies them to include/c++/v1 instead. In other words, was this intended?

phosek marked an inline comment as done.Tue, Apr 27, 11:32 PM
phosek added inline comments.
libcxx/cmake/Modules/HandleLibCXXABI.cmake
57

This was unintended, thanks for spotting it, I fixed it in rGeea5cbc8583d0857e0a9e429c61f7e87122b4dd6.