[libcxx-commits] [PATCH] D141882: [libc++] Remove #pragma GCC system_header

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

In D141882#4060754 <https://reviews.llvm.org/D141882#4060754>, @philnik wrote:

> In D141882#4060005 <https://reviews.llvm.org/D141882#4060005>, @Mordante wrote:
>
>> It seems the CI disagrees and they are needed for some headers since they use implementation reserved things, for example
>> `_LIBCPP_HIDE_FROM_ABI constexpr chrono::hours operator""h(unsigned long long __h)`
>
> The problem is that libc++abi doesn't use `-isystem` when including libc++ headers. I wasn't able to find out how libc++abi actually includes the libc++ headers though. Maybe Louis can help me out there.

This is where this happens: https://github.com/llvm/llvm-project/blob/main/libcxxabi/src/CMakeLists.txt#L171 and https://github.com/llvm/llvm-project/blob/main/libcxxabi/src/CMakeLists.txt#L253.

If you wanted to use `-isystem`, you'd have to change to `target_include_directories(... SYSTEM)` here: https://github.com/llvm/llvm-project/blob/main/libcxx/include/CMakeLists.txt#L903. This would propagate to the targets that use it. That being said, I don't think that's what we want -- I think it makes sense to build libc++ and libc++abi both using `-I` and get all the warnings, but we should probably sync the warnings between libc++ and libc++abi so that we don't have new warnings in libc++abi.

>> I think if we want to do this we should mention this change to vendors.

Yes, I agree. That will most definitely cause some breakage since a lot of people are using `-I` to include libc++, even though they really shouldn't. I see a lot of those internally and I am certainly not the only one. Basically anyone with a slightly funky setup (usually embedded stuff) ends up including the library manually (i.e. not letting the compiler figuring it out for them), and usually those are really naive attempts at making things work. Most people don't even realize that `-isystem` is a thing and what the difference is. So this is a bigger deal than it may seem at first. That being said, I still support the change, I think it's definitely the direction we want to move in.

Suggestion per the discussion: first, start by syncing up the libc++ and libc++abi warnings. I suggest moving the warning function to `libcxx/cmake/RuntimesWarnings.cmake` or something like that.

Then, when this patch is ready, we will have to ping the vendors and take this patch for a run. I think this may lead to a lot of (easy to fix) breakage, and we'll have to see how bad it is to know whether that is reasonable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141882/new/

https://reviews.llvm.org/D141882