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
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
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.
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.
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".
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.
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?
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.
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.