This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Remove #pragma GCC system_header
AbandonedPublic

Authored by philnik on Jan 16 2023, 6:08 PM.

Details

Reviewers
ldionne
Mordante
var-const
huixie90
Group Reviewers
Restricted Project
Summary

Having this pragma does effectively nothing. libc++ headers are normally included with -isystem, so the compiler automatically marks them as system headers. The only case where they aren't automatically marked as system headers is when explicitly including them and not using -isystem for that. This should almost never be the case, and even for the people who explicitly include them, it's just a matter of replacing -I with -isystem to get the correct behaviour. While having them isn't super bad, it's another bit of boilerplate. Given that having them or not only affects a very small group of people, and the fix being very simple, I think it makes sense to remove the pragmas.

Diff Detail

Event Timeline

philnik created this revision.Jan 16 2023, 6:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2023, 6:08 PM
philnik requested review of this revision.Jan 16 2023, 6:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2023, 6:08 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

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)

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

Since we're quite close to the branching of LLVM 16 I would prefer to postpone this patch by a week, unless there are pressing reasons to land this before branching.

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.

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

Since we're quite close to the branching of LLVM 16 I would prefer to postpone this patch by a week, unless there are pressing reasons to land this before branching.

I'm fine with that.

ldionne requested changes to this revision.Feb 16 2023, 9:48 AM

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.

This revision now requires changes to proceed.Feb 16 2023, 9:48 AM

it's just a matter of replacing -I with -isystem to get the correct behaviour

-I is ahead of -isystem in the header search path list, so just swapping the flags might not work as expected, since the libc++ headers need to be ordered in a particular way with respect to the resource directory and C library headers (and potentially other platform-specific headers, such as libandroid_support for Android). I'm fine with this change (we'll need to clean up some tech debt, but this is good motivation to get that done); I just wanted to point out that it's not always just as simple as "use -isystem instead of -I".

philnik updated this revision to Diff 505807.Mar 16 2023, 7:56 AM

Address comments

philnik updated this revision to Diff 505823.Mar 16 2023, 8:46 AM

Generate files

ldionne added a subscriber: Restricted Project.Mar 23 2023, 10:25 AM

Adding libc++ vendors for awareness. Once the CI is green, I will take this for a spin and I suggest at least Google does it as well because I suspect this may lead to a lot of small and easy to fix issues. But I expect the number to be quite large. Depending on what we find out, we will be able to tell whether this is reasonable or not.

philnik updated this revision to Diff 507909.Mar 23 2023, 3:51 PM

Try to fix CI

hans added a subscriber: hans.Mar 24 2023, 7:11 AM

it's just a matter of replacing -I with -isystem to get the correct behaviour

-I is ahead of -isystem in the header search path list, so just swapping the flags might not work as expected, since the libc++ headers need to be ordered in a particular way with respect to the resource directory and C library headers (and potentially other platform-specific headers, such as libandroid_support for Android). I'm fine with this change (we'll need to clean up some tech debt, but this is good motivation to get that done); I just wanted to point out that it's not always just as simple as "use -isystem instead of -I".

This (flag type affecting header search order) broke Chromium's Windows builds when we tried to include libc++ with -imsvc (clang-cl's -isystem equivalent) last year (crbug.com/1338539). We were never able to find a solution, so if this change lands it will cause us pain.

These pragmas have been in libc++'s header since the initial import. Does anyone know what was the original motivation, and whether or not it still applies?

philnik updated this revision to Diff 508548.Mar 27 2023, 2:41 AM

Generate files

These pragmas have been in libc++'s header since the initial import. Does anyone know what was the original motivation, and whether or not it still applies?

I do not know the original motivation, but I imagine it might have been to work around users incorrectly using -I to include headers, or maybe even for Howard himself when he first bootstrapped the library. I don't know for sure though.

I'll take this for a spin internally to see how much breakage this causes. Can you do the same with Chromium to try and quantify how much pain this causes?

It would sure be nice to shake the tree a bit and "encourage" users that include libc++ incorrectly to do it properly, while staying within the realm of what's reasonable of course.

hans added a comment.Mar 31 2023, 12:19 PM

I'll take this for a spin internally to see how much breakage this causes. Can you do the same with Chromium to try and quantify how much pain this causes?

For Chromium, we're fine except for Windows. As I mentioned above, we ran into trouble when we tried to switch to clang-cl's equivalent of -isystem because it affects the include path search order, which needs to be exactly right to make libc++ and msvc's headers work together.

We could probably figure it out with some more work (it's hard to say how much, I think I spent 1-2 days on it last time without succeeding), but I worry that others might run into similar problems.

It would sure be nice to shake the tree a bit and "encourage" users that include libc++ incorrectly to do it properly, while staying within the realm of what's reasonable of course.

It seems like this has potential for a lot of breakage with not much benefit, but that's just my two cents.

philnik abandoned this revision.Sep 23 2023, 1:58 AM