[libcxx][modules] protects users from relying on detail headers

[cjdb]

Summary

libc++ has started splicing standard library headers into much more
fine-grained content for maintainability. It's very likely that outdated
and naive tooling (some of which is outside of LLVM's scope) will
suggest users include things such as <__ranges/access.h> instead of
<ranges>, and Hyrum's law suggests that users will eventually begin to
rely on this without the help of tooling. As such, this commit
intends to protect users from themselves, by making it a hard error for
anyone outside of the standard library to include libc++ detail headers.

[zoecarver]

libcxx/include/__ranges/drop_view.h

14

Two suggestions:

1. could we make this a smaller macro? For example, could we make it so that in drop_view.h I only have to type out _LIBCPP_PRIVATE_HEADER(ranges, __ranges/drop_view.h)?

2. Could we somehow roll this into _LIBCPP_PUSH_MACROS and _LIBCPP_POP_MACROS so that we don't even need to update it? This would require some creative design work but I think it might be doable.

libcxx/test/libcxx/diagnostics/detail.headers/ranges/view_interface.header.verify.cpp

13

Could these tests be turned into a custom shell test that automates all this? I don't really want to have to do another thing every time I add a header file.

(You can use RUN: <whatever> like this.)

[ldionne]

Could this be implemented as a pragma? We have #pragma GCC system_header, perhaps we could have #pragma clang include_instead("<ranges>") or something along those lines? That would also be more generally useful outside of the standard library.

As it is, I find the benefit provided doesn't clearly surpass the amount of boilerplate needed to make it work. Also, if we want to make this worthwhile in any form, we'll have to automate the creation of those tests (or do that on the fly with a .sh.cpp test like Zoe suggests). We don't want to add a manual step like this every time we add a detail header -- we have many of those.

[cjdb]

libcxx/test/libcxx/diagnostics/detail.headers/ranges/view_interface.header.verify.cpp

13

The idea will be to use the same Python test for private modules. I did it by hand for this one for two reasons:

1. I wanted to showcase a small patch to get the discussion going. Your RFC has vindicated this first one.
2. I haven't yet worked out a way to safely automate code injection so it can be applied to all our headers at once.

But yes, automating this will happen by the time I look at <algorithm> (I am NOT writing ~120 of these by hand).

[cjdb]

Completely reworks patch to use #pragma clang include_instead(<header>). As with D105932, reviewers don't need to inspect individual files: reviewing the changes to utils/generate_private_header_tests.py should suffice.

I fully expect CI to fail for the time being, since D106394 hasn't been merged yet.

[cjdb]

Properly documents find_header_name.

[ldionne]

Overall LGTM, but I'd like to see this again before it's shipped. I'll request changes so it shows up correctly in the review queue, and I'll look again once the Clang patch has landed.

As it's been discussed, I think we'll want a solution to avoid having to generate N files every time we add a private header. We could look into .sh.cpp tests. But that's non-blocking.

libcxx/utils/generate_private_header_tests.py

89

Weird indentation?

97

Why are those not prefixed by their __?

[cjdb]

rebases to activate CI

[cjdb]

rebase to update CI

[cjdb]

rebases to activate CI with generated scripts corrected!

[cjdb]

rebases onto main

[cjdb]

rebases to activate CI

[cjdb]

Reviving this patch since we need this support

[EricWF]

I like this patch a lot.

There are already plenty of code at Google that has accidentally started including our new implementation headers. We should nip this in the bud right away.

Does this work when a user passes -Wsystem-headers?

[• Quuxplusone]

@EricWF obviously this should wait for @ldionne to return; it hasn't been touched since July 2021, so let's not jump the gun here.

Data point: We do now have a way to test for the existence of a line in every header, without generating a .verify.cpp test for every single detail header: see libcxx/test/libcxx/lint/lint_headers.sh.py. However, personally I'd be willing to do that part of the change myself (either Chris could eliminate all the new tests before landing, or I could do it myself with a big git rm after landing, I don't actually care which).

[EricWF]

In D106124#3346349, @Quuxplusone wrote:

@EricWF obviously this should wait for @ldionne to return; it hasn't been touched since July 2021, so let's not jump the gun here.

I don't see the need for that.

Data point: We do now have a way to test for the existence of a line in every header, without generating a .verify.cpp test for every single detail header: see libcxx/test/libcxx/lint/lint_headers.sh.py. However, personally I'd be willing to do that part of the change myself (either Chris could eliminate all the new tests before landing, or I could do it myself with a big git rm after landing, I don't actually care which).

We want to verify the actual diagnostic.

[cjdb]

rebases to ToT, which I'd forgotten to do before last push

[EricWF]

@cjdb You have my permission to land this when ready.

[cjdb]

adds clang-13, apple-clang-{11,12,13} as unsupported

[cjdb]

changes <threading_support.h> to "threading_support.h"

[cjdb]

Puts threading_support.h into config files

[cjdb]

adds no-locale and no-thread to list of unsupported

[thakis]

Why is this guarded by _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER (instead of, say, _LIBCPP_BUILDING_LIBRARY, or a dedicated define)? Seems like a fairly unrelated macro.

[• Quuxplusone]

In D106124#3347379, @thakis wrote:

Why is this guarded by _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER (instead of, say, _LIBCPP_BUILDING_LIBRARY, or a dedicated define)? Seems like a fairly unrelated macro.

No idea. Also, all the .verify.cpp tests are superfluously marked UNSUPPORTED: gcc-11 (verify tests run on Clang by definition), and ADDITIONAL_COMPILE_FLAGS: -U_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER ; both of those lines could go away. In fact, the huge huge majority of these tests could go away, and just roll the checks into lint_headers.sh.py; I have some ideas in that area.
But as long as mainline CI isn't actively failing, I think it makes the most sense to just wait for @ldionne to come back from vacation and then see what he wants to do with this patch.

[• Quuxplusone]

...Unless the diff in https://reviews.llvm.org/differential/changeset/?ref=3156531 is breaking something? Evaluating that change to HandleLibCXXABI.cmake is above my pay grade.

[thakis]

I don't know of anything broken by this, i was just curious :)

[• Quuxplusone]

arthur.j.odwyer added a reverting change: rGb6d75682f9fe: [libc++] [test] Check the presence of "pragma include_instead" in newly added….

That wasn't actually a reverting change; I just mentioned the magic phrase "revert D106124" in the middle of a sentence in the commit message.

[zibi]

In D106124#3347612, @thakis wrote:

I don't know of anything broken by this, i was just curious :)

The placement of threading_support.h in libcxxabi/include made difficult if not impossible to come up with POSIX(OFF) implementation in https://reviews.llvm.org/D120348. I ended up moving threading_support.h into libcxx/include.

Was there any reason why threading_support.h was created in libcxxabi/include and not libcxx/include together with existing implementation file __threading_support?

[ldionne]

This patch seems to break anyone using -I <include>/c++/v1 (as opposed to -isystem <include>/c++/v1, the more usual incantation). For example, I'm seeing an error when building libc++abi, which boils down to:

clang++ -nostdinc++ -I <build>/include/c++/v1 -std=c++2a libcxxabi/src/cxa_demangle.cpp -fsyntax-only

In file included from libcxxabi/src/cxa_demangle.cpp:13:
In file included from libcxxabi/src/demangle/ItaniumDemangle.h:25:
In file included from libcxxabi/src/demangle/Utility.h:20:
In file included from <build>/include/c++/v1/array:111:
In file included from <build>/include/c++/v1/__algorithm/equal.h:15:
In file included from <build>/include/c++/v1/__iterator/distance.h:14:
In file included from <build>/include/c++/v1/__iterator/concepts.h:14:
In file included from <build>/include/c++/v1/__iterator/incrementable_traits.h:14:
In file included from <build>/include/c++/v1/concepts:133:
In file included from <build>/include/c++/v1/__concepts/assignable.h:12:
In file included from <build>/include/c++/v1/__concepts/common_reference_with.h:12:
In file included from <build>/include/c++/v1/__concepts/convertible_to.h:14:
<build>/include/c++/v1/type_traits:4104:10: error: header '<__utility/forward.h>' is an implementation detail; #include '<utility>' instead
#include <__utility/forward.h>
         ^
<build>/include/c++/v1/type_traits:4105:10: error: header '<__utility/move.h>' is an implementation detail; #include '<utility>' instead
#include <__utility/move.h>
         ^

Reducing even more, it looks like the following breaks:

echo "#include <algorithm>" | clang++ -nostdinc++ -I <build>/include/c++/v1 -fsyntax-only -xc++ -

I'm not sure why CI seems happy with the patch, but I definitely see those issues locally. I'm still investigating.

In D106124#3346614, @EricWF wrote:

In D106124#3346349, @Quuxplusone wrote:

@EricWF obviously this should wait for @ldionne to return; it hasn't been touched since July 2021, so let's not jump the gun here.

I don't see the need for that.

I'm a bit disappointed this was landed hastily -- there are some issues with this patch. Except for what I reported above, those are not major issues, but it would have been easy to address them before landing. For example, we don't support some of the compilers that this patch contains UNSUPPORTED markup for. I also share other reviewer's questions about the introduction of a new threading_support.h header inside libc++abi.

Anyway, I'm not really sure where to go from here, given that it breaks a pretty basic use case. I'll investigate this a bit more, but we might have to revert this until we figure out these issues.

Other than that, I like this patch, I think it provides a nice guard against people relying on our implementation details.

[ldionne]

In D106124#3349467, @ldionne wrote:

This patch seems to break anyone using -I <include>/c++/v1 (as opposed to -isystem <include>/c++/v1, the more usual incantation). For example, I'm seeing an error when building libc++abi, which boils down to:

[...]

Reducing even more, it looks like the following breaks:

echo "#include <algorithm>" | clang++ -nostdinc++ -I <build>/include/c++/v1 -fsyntax-only -xc++ -

I'm not sure why CI seems happy with the patch, but I definitely see those issues locally. I'm still investigating.

Ah! I figured it out. It only happens with our downstream version of libc++ because our <type_traits> still include __utility/forward.h and __utility/move.h for backwards compatibility. Removing those includes (which was a TODO anyway) fixes the issue. The other points that were raised remain, but I think we can address them in subsequent commits without reverting. In the future, it would be good to wait before landing such patches, especially since it was not nearly time critical (it had been open for months anyway).

[ldionne]

Nevermind, the above was wrong too! It turns out to be much trickier than this. In fact, we pass -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER when running the tests with Clang, which means that we're excluding all the include_instead pragmas. So this is basically never tested, except in the generated tests where we explicitly undefine _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER.

Oh, and it's also "tested" on AppleClang because we don't define _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER with that compiler. I suspect that's why these issues showed up locally with a recent AppleClang but not on the Linux CI (nor on the Apple CI, which might be using an older AppleClang that doesn't support the pragma).

So... long story short, I think what we want is:

1. Pass _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER with AppleClang (easy to do and I'll handle that separately).
2. Use -I instead of -isystem in the test suite (D118616 does that).
3. Use pragma include_instead regardless of whether _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER is defined (I don't know why it's guarded by it anyways).

Until we get ourselves out of this mess a bit more, I think the only option is to revert this.

[ldionne]

In D106124#3349710, @ldionne wrote:

1. Pass _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER with AppleClang (easy to do and I'll handle that separately).

See D120684.