This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by cjdb on Jul 15 2021, 9:31 PM.

Details

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.

Diff Detail

Event Timeline

cjdb requested review of this revision.Jul 15 2021, 9:31 PM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2021, 9:31 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
zoecarver added inline comments.Jul 16 2021, 10:14 AM
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)?
  1. 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
14

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 requested changes to this revision.Jul 16 2021, 11:05 AM

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.

This revision now requires changes to proceed.Jul 16 2021, 11:05 AM
cjdb added inline comments.Jul 16 2021, 11:15 AM
libcxx/test/libcxx/diagnostics/detail.headers/ranges/view_interface.header.verify.cpp
14

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 updated this revision to Diff 361462.EditedJul 24 2021, 9:09 AM
cjdb edited the summary of this revision. (Show Details)

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 updated this revision to Diff 361463.Jul 24 2021, 9:26 AM

Properly documents find_header_name.

ldionne requested changes to this revision.Jul 26 2021, 6:32 AM

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
91

Weird indentation?

99

Why are those not prefixed by their __?

This revision now requires changes to proceed.Jul 26 2021, 6:32 AM
cjdb updated this revision to Diff 362017.Jul 27 2021, 6:56 AM

rebases to activate CI

Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2021, 6:56 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cjdb updated this revision to Diff 363068.Jul 30 2021, 6:54 AM

rebase to update CI

cjdb updated this revision to Diff 363110.Jul 30 2021, 8:45 AM
cjdb retitled this revision from [libcxx][modules] protects users from relying on ranges detail headers to [libcxx][modules] protects users from relying on detail headers.

rebases to activate CI with generated scripts corrected!

cjdb updated this revision to Diff 366529.Aug 15 2021, 4:22 PM
cjdb edited the summary of this revision. (Show Details)

rebases onto main

cjdb updated this revision to Diff 367239.Aug 18 2021, 10:08 AM

rebases to activate CI

cjdb abandoned this revision.Jan 18 2022, 11:54 AM
cjdb updated this revision to Diff 411485.Feb 25 2022, 11:47 AM

Reviving this patch since we need this support

EricWF accepted this revision.Feb 25 2022, 12:03 PM
EricWF added a subscriber: 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 requested changes to this revision.Feb 25 2022, 12:17 PM
Quuxplusone added a subscriber: 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).

This revision now requires changes to proceed.Feb 25 2022, 12:17 PM

@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 updated this revision to Diff 411536.Feb 25 2022, 3:36 PM

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

EricWF accepted this revision.Feb 25 2022, 3:49 PM

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

cjdb updated this revision to Diff 411560.Feb 25 2022, 5:31 PM

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

cjdb updated this revision to Diff 411574.Feb 25 2022, 9:20 PM

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

cjdb updated this revision to Diff 411579.Feb 25 2022, 10:19 PM

Puts threading_support.h into config files

Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2022, 10:19 PM
cjdb updated this revision to Diff 411583.Feb 25 2022, 11:34 PM

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

This revision was not accepted when it landed; it landed in state Needs Review.Feb 26 2022, 1:01 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Feb 26 2022, 7:24 AM

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.

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.

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

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

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 added a subscriber: zibi.Feb 27 2022, 3:20 PM

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?

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.

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

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

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.

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

See D120684.