[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
ldionne wrote: > > So the intent of the current ordering was to allow creating a toolchain by > > placing the libc++ headers alongside Clang, as is typically done (and > > exceptionally not done on Apple platforms). On Apple platforms, you > > basically always specify a sysroot, either explicitly or implicitly (via > > env vars). You have to, since the SDK is where you find everything you need > > to compile even the most basic program (like our libc headers). > > @ldionne thank you for explaining the reasoning behind the decision. > Unfortunately, it makes the life of a non-system compilers and clang-tools > much more complicated on Apple platforms. They usually don’t have system > headers shipped with them so they rely on one provided in SDK. My interest in > this issue started from clang-tidy that was not able to find cxxabi.h in the > path specified with -isysroot due to the new order introduced in > https://reviews.llvm.org/D89001. If you ship libc++ headers in the toolchain directory as part of your non-system compiler, you should also be shipping the lib++abi headers in the toolchain directory. Libc++ and libc++abi always go hand in hand, that's a common source of confusion. > But it seems my use case will be broken anyway if SDK stops providing libc++ > headers. Is it the eventual future? No. The libc++ headers are in the SDK right now and they will remain there in the future. > If it is the case, checking -sysroot first will be only a temporary solution. > I still think that it makes sense for the time being but also we need to > think about a long term solution for non-system compilers and clang-tools. > Perhaps it should be configured at build time if you are building the system > default compiler or additional tools shipped separately. The current setup is explicitly intended to make the life of non-system compilers easier. You pass `-isysroot ` for the system headers, and if you happen to want to build libc++ from source and ship it with your system compiler, you place the headers in the toolchain directory (as usually done in upstream LLVM) and everything will work. If you want to rely on the SDK-provided libc++ instead, you simply don't put any libc++ headers in the toolchain directory and Clang will use the ones it finds in `-isysroot`. I feel like either I am fundamentally misunderstanding something or we're talking past each other. I don't see a problem with the current logic as done upstream, since it makes it easiest to build a non-system toolchain (from my point of view at least). https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
dmpolukhin wrote: > So the intent of the current ordering was to allow creating a toolchain by > placing the libc++ headers alongside Clang, as is typically done (and > exceptionally not done on Apple platforms). On Apple platforms, you basically > always specify a sysroot, either explicitly or implicitly (via env vars). You > have to, since the SDK is where you find everything you need to compile even > the most basic program (like our libc headers). @ldionne thank you for explaining the reasoning behind the decision. Unfortunately, it makes the life of a non-system compilers and clang-tools much more complicated on Apple platforms. They usually don’t have system headers shipped with them so they rely on one provided in SDK. My interest in this issue started from clang-tidy that was not able to find cxxabi.h in the path specified with -isysroot due to the new order introduced in https://reviews.llvm.org/D89001. But it seems my use case will be broken anyway if SDK stops providing libc++ headers. Is it the eventual future? If it is the case, checking -sysroot first will be only a temporary solution. I still think that it makes sense for the time being but also we need to think about a long term solution for non-system compilers and clang-tools. Perhaps it should be configured at build time if you are building the system default compiler or additional tools shipped separately. https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
ldionne wrote: > Yes. What I think Dmitry and I are trying to explain is that it is very weird > that providing an explicit `-isysroot` in the command line is ignored for the > C++ headers (and only for them). One can start using `-nostdinc` and friends, > but that mean rebuilding the header search paths manually, which is not great. I understand now, thanks for explaining again. So the intent of the current ordering was to allow creating a toolchain by placing the libc++ headers alongside Clang, as is typically done (and exceptionally not done on Apple platforms). On Apple platforms, you basically always specify a sysroot, either explicitly or implicitly (via env vars). You *have* to, since the SDK is where you find everything you need to compile even the most basic program (like our libc headers). If we instead "listened" to the `-isysroot` argument that is always passed by the user on Apple platforms for finding libc++ headers, there wouldn't be a way to create a toolchain like I explained above. https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
ilg-ul wrote: Yes, that's why I said that your patch must be elaborated and check for the presence of `-isysroot`, since the internal variable may come from other settings. https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
drodriguez wrote: The other place that a default sysroot might come is the CMake option `DEFAULT_SYSROOT`. In my builds it is empty (the default), but it might be pointing to something in those xpack builds: https://github.com/xpack/xpack-build-box/blob/64488ebdfefd96e5eec45ab31bc170aa028fed4e/helper/common-apps-functions-source.sh#L9135 I think this is were that value is coming from. In all my testing that value is empty, so it is never a fallback, but it might have an actual value in your case, which makes `-isysroot` magically appear even if no value has been provided in the command line. https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
ilg-ul wrote: If, for any reason, you need the SDK path to be the first in the search list, I suggest you add an extra condition, the explicit presence of the `-isysroot`, since it looks like the `Sysroot` variable may come from other configuration definitions. https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
ilg-ul wrote: > I am suspicious of SDKROOT maybe being in the environment. Can you check if > your environment is providing that value under the hood? I checked and there is no SDKROOT in the environment. https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
drodriguez wrote: @ilg-ul: It seems that the driver does not receive any `-isysroot`, but it seems that `cc1` receives `-isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk`, so I am suspicious of `SDKROOT` maybe being in the environment. Can you check if your environment is providing that value under the hood? https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
ilg-ul wrote: I did a test with this patch, and, as expected, the resulting behaviour is wrong. The verbose build of a hello-world test resulted in: ``` [/Users/ilg/Work/xpack-dev-tools-build/clang-18.1.0-1/darwin-x64/application/bin/clang++ simple-hello.cpp -o simple-hello-cpp-one -v -v] xPack x86_64 clang version 18.1.0rc Target: x86_64-apple-darwin23.2.0 Thread model: posix InstalledDir: /Users/ilg/Work/xpack-dev-tools-build/clang-18.1.0-1/darwin-x64/application/bin "/Users/ilg/Work/xpack-dev-tools-build/clang-18.1.0-1/darwin-x64/application/bin/clang-18" -cc1 -triple x86_64-apple-macosx10.13.0 -Wundef-prefix=TARGET_OS_ -Werror=undef-prefix -Wdeprecated-objc-isa-usage -Werror=deprecated-objc-isa-usage -emit-obj -mrelax-all -dumpdir simple-hello-cpp-one- -disable-free -clear-ast-before-backend -disable-llvm-verifier -discard-value-names -main-file-name simple-hello.cpp -mrelocation-model pic -pic-level 2 -mframe-pointer=all -ffp-contract=on -fno-rounding-math -funwind-tables=2 -fcompatibility-qualified-id-block-type-checking -fvisibility-inlines-hidden-static-local-var -fbuiltin-headers-in-system-modules -fdefine-target-os-macros -target-cpu penryn -tune-cpu generic -debugger-tuning=lldb -fdebug-compilation-dir=/Users/ilg/Work/xpack-dev-tools-build/clang-18.1.0-1/darwin-x64/x86_64-apple-darwin23.2.0/tests/clang/c-cpp -target-linker-version 1022.1 -v -v -fcoverage-compilation-dir=/Users/ilg/Work/xpack-dev-tools-build/clang-18.1.0-1/darwin-x64/x86_64-apple-darwin23.2.0/tests/clang/c-cpp -resource-dir /Users/ilg/Work/xpack-dev-tools-build/clang-18.1.0-1/darwin-x64/application/lib/clang/18 -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk -internal-isystem /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1 -internal-isystem /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/local/include -internal-isystem /Users/ilg/Work/xpack-dev-tools-build/clang-18.1.0-1/darwin-x64/application/lib/clang/18/include -internal-externc-isystem /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include -fdeprecated-macro -ferror-limit 19 -stack-protector 1 -fblocks -fencode-extended-block-signature -fregister-global-dtors-with-atexit -fgnuc-version=4.2.1 -fskip-odr-check-in-gmf -fcxx-exceptions -fexceptions -fmax-type-align=16 -D__GCC_HAVE_DWARF2_CFI_ASM=1 -o /var/folders/gr/13tt3vcd7m1gnbhwtkmf5cnwgn/T/simple-hello-6b5d6d.o -x c++ simple-hello.cpp clang -cc1 version 18.1.0rc based upon LLVM 18.1.0rc default target x86_64-apple-darwin23.2.0 ignoring nonexistent directory "/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/local/include" ignoring nonexistent directory "/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/Library/Frameworks" #include "..." search starts here: #include <...> search starts here: /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1 <--- INCORRECT! /Users/ilg/Work/xpack-dev-tools-build/clang-18.1.0-1/darwin-x64/application/lib/clang/18/include /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks (framework directory) End of search list. "/usr/bin/ld" -demangle -lto_library /Users/ilg/Work/xpack-dev-tools-build/clang-18.1.0-1/darwin-x64/application/lib/libLTO.dylib -no_deduplicate -dynamic -arch x86_64 -platform_version macos 10.13.0 10.13.0 -syslibroot /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk -mllvm -enable-linkonceodr-outlining -o simple-hello-cpp-one /var/folders/gr/13tt3vcd7m1gnbhwtkmf5cnwgn/T/simple-hello-6b5d6d.o -lc++ -lSystem /Users/ilg/Work/xpack-dev-tools-build/clang-18.1.0-1/darwin-x64/application/lib/clang/18/lib/darwin/libclang_rt.osx.a ``` The same run with the un-patched source: ``` [/Users/ilg/Work/xpack-dev-tools-build/clang-18.1.0-1/darwin-x64/application/bin/clang++ simple-hello.cpp -o simple-hello-cpp-one -v -v] xPack x86_64 clang version 18.1.0rc Target: x86_64-apple-darwin23.2.0 Thread model: posix InstalledDir: /Users/ilg/Work/xpack-dev-tools-build/clang-18.1.0-1/darwin-x64/application/bin "/Users/ilg/Work/xpack-dev-tools-build/clang-18.1.0-1/darwin-x64/application/bin/clang-18" -cc1 -triple x86_64-apple-macosx10.13.0 -Wundef-prefix=TARGET_OS_ -Werror=undef-prefix -Wdeprecated-objc-isa-usage -Werror=deprecated-objc-isa-usage -emit-obj -mrelax-all -dumpdir simple-hello-cpp-one- -disable-free -clear-ast-before-backend -disable-llvm-verifier -discard-value-names -main-file-name simple-hello.cpp -mrelocation-model pic -pic-level 2 -mframe-pointer=all -ffp-contract=on -fno-rounding-math -funwind-tables=2 -fcompatibility-qualified-id-block-type-checking -fvisibility-inlines-hidden-static-local-var -fbuiltin-headers-in-system-modules -fdefine-target-os-macros -target-cpu penryn -tune-cpu generic -debugger-tuning=lldb -fdebug-compilation-dir=/Users/ilg/Work/xpack-dev-tools-build/clang-18.1.0-1/darwin-x64/x86_64-apple-darwin23.2.0/tests/cla
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
drodriguez wrote: > With that being said, can you clarify what you mean by "ignoring the > command-line option", and can you expand on why the current state of upstream > Clang is broken in your opinion? But for productivity's sake, let's take for > granted that the canonical state is upstream Clang and let's pretend that > AppleClang also behaves the same (which is not quite true yet). I speak only for myself, but I find very confusing that `/bar/bin/clang -isysroot /foo` will ignore the headers provided in `/foo/usr/include/c++/v1` when they exist just because `/bar/include/c++/v1` exists. An additional problem is that there is no flag to say `-ignore-the-toolchain-c++-headers-and-use-sysroot-I-mean-it` so someone can have a toolchain with C++ headers, but still prefer some external headers in the SDK. In a particular case, when developing, one can end up with a build directory that has created an empty `/include/c++/v1` (because libc++ has been configured, but not built), and `/bin/clang` will only look at that directory instead of the one provided in `SDKROOT` or `-isysroot`, failing every time that C++ headers are referenced (because they are not actually there). https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
drodriguez wrote: > But after taking a quick look, my impression is that this change reverses the > bug fixed in #70817. > > To recap, that PR preferred the C++ headers available in the toolchain > distribution over the SDK headers, when the compiler was launched via a > symbolic link to the executable (a behaviour common to the npm/xpm ecosystem). > > If I understand this proposal right (please correct me if I'm wrong), it will > move the SDK headers to the top of the list. > > If this will happen only when an explicit `-isysroot` is passed on the > compiler command line, it might be ok, but if it will happen for all cases, > this will simply bring us back to the pre 70817 case, when builds on old > macOS-es (like 10.13) with a new clang (like 17.x) will fail, due to the out > of sync old headers from the SDK with the new library from the toolchain. I > wasted quite a lot of time to diagnose this subtle issue by that time. Yes. What I think Dmitry and I are trying to explain is that it is very weird that providing an explicit `-isysroot` in the command line is ignored for the C++ headers (and only for them). One can start using `-nostdinc` and friends, but that mean rebuilding the header search paths manually, which is not great. At least in my testing preferring the sysroot only happens when providing an explicit `-isysroot` in the command line, but also might be affected by the presence of `SDKROOT` in the environment (tools like `xcrun` set this value). If one is careful and avoid those two, the headers by the `clang` binary should be preferred. https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
ldionne wrote: > @ldionne if downstream is catching upstream, I would love to discuss > rationale behind ignoring command line option. I asked this question several > times and haven't got answer. Original > [diff](https://reviews.llvm.org/D89001) that introduced this behavior also > didn't explain why new behavior is "intended" and better than old one. > Instead of resolving case of two headers it actually prevents finding header > that exists and path is specified in command line. I think I don't understand your question, but I'm happy to discuss the rationale for the current approach once I do. Let me try to shed some light. Five-ish years ago, Apple shipped libc++ in the toolchain (roughly in `$(which clang++)/../include/c++/v1`), and did not ship any libc++ headers in the SDK. We wanted to transition over to shipping libc++ only in the SDK, and not in the toolchain anymore. This was motivated by numerous factors, in particular our desire to decouple the shipping of libc++ and Clang to give more flexibility to libc++. To perform that transition, we elaborated the following plan: 1. We started shipping libc++ headers in the SDK (in addition to those in the toolchain) 2. We changed the Clang driver to prefer headers found in the SDK if there were any, and to fallback the toolchain headers otherwise. This behavior only ever made sense for AppleClang during the transition period, as a way for us to support mixing set ups where the headers could come either from the toolchain or from the SDK, while preferring the SDK ones. 3. We then removed the toolchain headers. Now AppleClang would always find only the headers in the SDK. 4. Finally we flipped back the default to what it should always have been, i.e. the behavior where we look in the toolchain and use these headers if they exist, and otherwise we fall back on the SDK headers. If my memory serves me right, upstream never saw the change (2) since it only made sense for Apple's own toolchain (or at least that was our reasoning back then). The current problem you're seeing is simply that the "switch back" in step (4) is taking a long time to hit a release (due to release-related internal difficulties), and so naturally that makes you think that upstream is broken somehow. With that being said, can you clarify what you mean by "ignoring the command-line option", and can you expand on why the current state of upstream Clang is broken in your opinion? But for productivity's sake, let's take for granted that the canonical state is upstream Clang and let's pretend that AppleClang also behaves the same (which is not quite true yet). https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
ilg-ul wrote: Sorry, the discussion went too specific for me :-( But after taking a quick look, my impression is that this change reverses the bug fixed in https://github.com/llvm/llvm-project/pull/70817. To recap, that PR preferred the C++ headers available in the toolchain distribution over the SDK headers, when the compiler was launched via a symbolic link to the executable (a behaviour common to the npm/xpm ecosystem). If I understand this proposal right (please correct me if I'm wrong), it will move the SDK headers to the top of the list. If this will happen only when an explicit `-isysroot` is passed on the compiler command line, it might be ok, but if it will happen for all cases, this will simply bring us back to the pre 70817 case, when builds on old macOS-es (like 10.13) with a new clang (like 17.x) will fail, due to the out of sync old headers from the SDK with the new library from the toolchain. I wasted quite a lot of time to diagnose this subtle issue by that time. https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
dmpolukhin wrote: @ldionne if downstream is catching upstream, I would love to discuss rationale behind ignoring command line option. I asked this question several times and haven't got answer. Original [diff](https://reviews.llvm.org/D89001) that introduced this behavior also didn't explain why new behavior is "intended" and better than old one. Instead of resolving case of two headers it actually prevents finding header that exists and path is specified in command line. https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
ldionne wrote: If we do that, we’ll just create churn. It’s a moving target. You will « fix » upstream Clang to match « the system compiler » temporarily, but by doing so you’re causing the downstream Clang to ingest that change too via auto-merging and that means you’ll flip-flop the state of downstream. That is, unless we manually undo the change downstream but then upstream Clang is the one that will eventually need to play catch up with downstream. IMO that’s just churn. It’s a lot easier and better to let upstream be the canonical version and let downstream play catch up like it normally does. It just sucks that it took so long for this change to make it into downstream clang but that’s a separate story. https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
drodriguez wrote: I agree with Dmitry in that it would be preferred to have the system compiler behaviour until the system compiler actually changes. I have tried again with Xcode 14.3.1 (released almost 1 year ago), which includes `include/c++/v1` side by side with the compiler, and for it, passing `-isysroot` still overrides the side by side path. https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
dmpolukhin wrote: @ldionne what about fixing upstream behavior to match actual system compiler behavior now and when/as soon as we have new behavior in Apple compiler fix it again in newer clang versions? Moreover as I pointed out Apple compiler behavior is problematic for clang-tools and clangd because it basically ignores option passed in command line. https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
ldionne wrote: Woah, I just checked Xcode 15.2 and you are totally right, it contains `clang-1500.1.0.2.5` which still has the behavior reversed from upstream. Sorry for the confusion, I assumed it was out in recent Xcodes cause the change was made a while back, but it looks like this change still hasn't made it into a version of Xcode. However, being the author of the change and the one who synced upstream and downstream, I can guarantee you that the upstream behavior is the correct and desired one, I made sure to make it that way. It's just annoying that the downstream hasn't caught up with this change yet, but it's only a matter of time. So IMO, there's unfortunately nothing to do here except wait. https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
drodriguez wrote: The `Xcode.app` I was using in the previous comment was 15.1. I tried again the same steps with the last version I have (Xcode 15.3b1, 15E5178i) and I obtain the same behaviour. https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
dmpolukhin wrote: @ldionne I also haven't see Apple compiler that matches upstream behavior. In https://reviews.llvm.org/D157283#4648242, you mentioned that "Xcode 15 RC" should be such compiler and checked it and couple version after that and none of them matched upstream so I decided to wait and forgot about the issue. @drodriguez thank you for rising it again! https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
drodriguez wrote: > @drodriguez I'm not certain why you think AppleClang prefers `isysroot` over > toolchain headers -- it might be because we simply don't ship libc++ > toolchain headers anymore and we only ship libc++ SDK headers. But if you try > installing libc++ headers in the toolchain alongside a reasonably recent > AppleClang, it should prefer them over the SDK headers (which is consistent > with the upstream behavior). This is the tests I have done to verify: ``` $ mkdir -p /tmp/test-toolchains/ $ cd /tmp/test-toolchains/ $ cp -R /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain pristine $ cp -R pristine modified $ mkdir -p modified/usr/include/c++/v1 $ echo "#error Boom" > modified/usr/include/c++/v1/cxxabi.h $ echo "#include " > test.cpp # Using pristine, which does not have any headers beside it $ pristine/usr/bin/clang -c test.cpp -o test.pristine.o test.cpp:1:10: fatal error: 'cxxabi.h' file not found #include ^~ 1 error generated. # Using modified, with the headers beside it modified/usr/bin/clang -c test.cpp -o test.modified.o In file included from test.cpp:1: /tmp/test-toolchain/modified/usr/bin/../include/c++/v1/cxxabi.h:1:2: error: Boom #error Boom ^ 1 error generated. # Using modified, but passing a isysroot (no errors) $ modified/usr/bin/clang -c test.cpp -o test.sysroot.o -isysroot (xcrun --show-sdk-path) $ ls test.sysroot.o test.sysroot.o ``` Unless I am understanding the behaviour incorrectly, this seems to be the opposite of the upstream behaviour. https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
ldionne wrote: @dmpolukhin Replying to https://reviews.llvm.org/D157283#4648445: > @ldionne thank you for the reply. Unfortunately current behaviour makes > problems for clang-tools like clang-tidy and clangd that read CDBs from > compile_commands.json. They start looking headers relative to compiler path > specified in CDB but it is better to use path specified in -isysroot (it is > what user expects when they specify the option). What is the rationale behind > ignoring -isysroot specified in command line? If user would like to use > search path relative to compiler, they could just remove -isysroot and get > this behaviour. The intent was simply that if someone builds a Clang-based toolchain on Apple platforms and they include libc++ in that toolchain, the toolchain libc++ should be preferred over any system-provided libc++ we have in the SDK. So even if you use `-isysroot ` (which you need for everything like the C stdlib and other system headers), we want your custom-built toolchain libc++ headers to be preferred over the system-provided libc++ headers (in the SDK). But by default, we don't ship toolchain headers with AppleClang so we check in the toolchain, find nothing, and then fall back to the SDK. https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
ldionne wrote: In https://reviews.llvm.org/D157283#4648242, I explained that the current upstream code actually matches exactly what we have downstream. I made sure to upstream everything to avoid differences after we started shipping libc++ in the SDK and were done with our transition. @drodriguez I'm not certain why you think AppleClang prefers `isysroot` over toolchain headers -- it might be because we simply don't ship libc++ toolchain headers anymore and we only ship libc++ SDK headers. But if you try installing libc++ headers in the toolchain alongside a reasonably recent AppleClang, it should prefer them over the SDK headers (which is consistent with the upstream behavior). https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
drodriguez wrote: > I need to take a closer look, since at first reading I can't evaluate the > consequences, especially if this does change the behaviour when -isysroot is > **not** provided. Small detail that I think you are aware: the `-isysroot` can be provided, but it needs to have the `include/c++/v1` directory to be considered. > And I do not know exactly the use case you are considering. My use case was > relatively straightforward, multiple versions of the toolchain are installed > in versioned custom folders in user home, and different projects requiring > different toolchain versions have symbolic links from the project folder to > one of the clang executable. > > If your change does not affect the above use case and also adds more > consistency with Apple clang, it should be fine. I hope my modifications have not changed the behaviour of those versioned custom folders and symlinked `clang`. The tests were modified in a couple of places to remove a sysroot containing C++ headers in those checks. Before it did not matter which sysroot was being used, since it was the last to be chosen, but it was now interfering with what the test seemed to try to test. https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
ilg-ul wrote: > they change the behaviour you introduced in > https://github.com/llvm/llvm-project/pull/70817 when -isysroot is provided. I need to take a closer look, since at first reading I can't evaluate the consequences, especially if this does change the behaviour when -isysroot is **not** provided. And I do not know exactly the use case you are considering. My use case was relatively straightforward, multiple versions of the toolchain are installed in versioned custom folders in user home, and different projects requiring different toolchain versions have symbolic links from the project folder to one of the clang executable. If your change does not affect the above use case and also adds more consistency with Apple clang, it should be fine. https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
drodriguez wrote: @ilg-ul: you might be interested in reviewing this changes, since they change the behaviour you introduced in #70817 when `-isysroot` is provided. If some folks at Apple reads this: it would be preferable to have the actual code instead of trying to guess which is the internal behaviour of Apple Clang. I will be happy to close this PR if the original code (or a "censored" version of it) can be upstreamed. Thanks! https://github.com/llvm/llvm-project/pull/80524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
llvmbot wrote:
@llvm/pr-subscribers-clang-driver
@llvm/pr-subscribers-clang
Author: Daniel Rodríguez Troitiño (drodriguez)
Changes
The current Apple Clang behaviour is to prefer `-isysroot` vs libc++ headers
side-by-side the compiler. This has been like that for several Xcode versions,
at least since Xcode 14.
The code was originally written in D89001 chosing the order that was correct at
the time for Apple Clang. In 2023 D157283 tried to flip the order to match the
current Apple Clang, but was reverted in
3197357b7e39a58bc7eb0600eb337ac2a1c8c225 because it brokes two tests. The code
was further changed in #70817 to add a third option.
The changes in this commit try to match Apple Clang, and incorporate the
changes in #70817, as well as fixing the tests that broke when D157283
was applied.
---
Full diff: https://github.com/llvm/llvm-project/pull/80524.diff
4 Files Affected:
- (modified) clang/lib/Driver/ToolChains/Darwin.cpp (+16-16)
- (modified) clang/test/Driver/darwin-header-search-libcxx.cpp (+21-20)
- (modified) clang/test/Tooling/clang-check-mac-libcxx-abspath.cpp (+2-1)
- (modified) clang/test/Tooling/clang-check-mac-libcxx-relpath.cpp (+2-1)
``diff
diff --git a/clang/lib/Driver/ToolChains/Darwin.cpp
b/clang/lib/Driver/ToolChains/Darwin.cpp
index fae8ad1a958ad..004a19284ee4a 100644
--- a/clang/lib/Driver/ToolChains/Darwin.cpp
+++ b/clang/lib/Driver/ToolChains/Darwin.cpp
@@ -2515,20 +2515,31 @@ void DarwinClang::AddClangCXXStdlibIncludeArgs(
switch (GetCXXStdlibType(DriverArgs)) {
case ToolChain::CST_Libcxx: {
// On Darwin, libc++ can be installed in one of the following places:
-// 1. Alongside the compiler in /include/c++/v1
-// 2. Alongside the compiler in /../include/c++/v1
-// 3. In a SDK (or a custom sysroot) in /usr/include/c++/v1
+// 1. In a SDK (or a custom sysroot) in /usr/include/c++/v1
+// 2. Alongside the compiler in /include/c++/v1
+// 3. Alongside the compiler in /../include/c++/v1
//
// The precedence of paths is as listed above, i.e. we take the first path
// that exists. Note that we never include libc++ twice -- we take the
first
// path that exists and don't send the other paths to CC1 (otherwise
// include_next could break).
//
-// Also note that in most cases, (1) and (2) are exactly the same path.
+// Also note that in most cases, (2) and (3) are exactly the same path.
// Those two paths will differ only when the `clang` program being run
// is actually a symlink to the real executable.
// Check for (1)
+llvm::SmallString<128> SysrootUsr = Sysroot;
+llvm::sys::path::append(SysrootUsr, "usr", "include", "c++", "v1");
+if (getVFS().exists(SysrootUsr)) {
+ addSystemInclude(DriverArgs, CC1Args, SysrootUsr);
+ return;
+} else if (DriverArgs.hasArg(options::OPT_v)) {
+ llvm::errs() << "ignoring nonexistent directory \"" << SysrootUsr
+ << "\"\n";
+}
+
+// Check for (2)
// Get from '/bin' to '/include/c++/v1'.
// Note that InstallBin can be relative, so we use '..' instead of
// parent_path.
@@ -2543,7 +2554,7 @@ void DarwinClang::AddClangCXXStdlibIncludeArgs(
<< "\"\n";
}
-// (2) Check for the folder where the executable is located, if different.
+// (3) Check for the folder where the executable is located, if different.
if (getDriver().getInstalledDir() != getDriver().Dir) {
InstallBin = llvm::StringRef(getDriver().Dir);
llvm::sys::path::append(InstallBin, "..", "include", "c++", "v1");
@@ -2556,17 +2567,6 @@ void DarwinClang::AddClangCXXStdlibIncludeArgs(
}
}
-// Otherwise, check for (3)
-llvm::SmallString<128> SysrootUsr = Sysroot;
-llvm::sys::path::append(SysrootUsr, "usr", "include", "c++", "v1");
-if (getVFS().exists(SysrootUsr)) {
- addSystemInclude(DriverArgs, CC1Args, SysrootUsr);
- return;
-} else if (DriverArgs.hasArg(options::OPT_v)) {
- llvm::errs() << "ignoring nonexistent directory \"" << SysrootUsr
- << "\"\n";
-}
-
// Otherwise, don't add any path.
break;
}
diff --git a/clang/test/Driver/darwin-header-search-libcxx.cpp
b/clang/test/Driver/darwin-header-search-libcxx.cpp
index 70cc06090a993..82f2616a87dd1 100644
--- a/clang/test/Driver/darwin-header-search-libcxx.cpp
+++ b/clang/test/Driver/darwin-header-search-libcxx.cpp
@@ -23,8 +23,8 @@
// RUN: | FileCheck -DTOOLCHAIN=%S/Inputs/basic_darwin_toolchain \
// RUN: --check-prefix=CHECK-LIBCXX-TOOLCHAIN-1 %s
// CHECK-LIBCXX-TOOLCHAIN-1: "-cc1"
-// CHECK-LIBCXX-TOOLCHAIN-1: "-internal-isystem"
"[[TOOLCHAIN]]/usr/bin/../include/c++/v1"
// CHECK-LIBCXX-TOOLCHAIN-1-NOT: "-internal-isystem" "/usr/include/c++/v1"
+// CHECK-LIBCXX-TOOLCHAIN-1: "-internal-isystem"
"[[TOOLCHAIN]]/usr/bin/../include/c++/v1"
//
// RUN: %clang -### %s -fsyntax-only 2>&1 \
// RUN: --targe
[clang] [clang] Match -isysroot behaviour with system compiler on Darwin (PR #80524)
https://github.com/drodriguez created
https://github.com/llvm/llvm-project/pull/80524
The current Apple Clang behaviour is to prefer `-isysroot` vs libc++ headers
side-by-side the compiler. This has been like that for several Xcode versions,
at least since Xcode 14.
The code was originally written in D89001 chosing the order that was correct at
the time for Apple Clang. In 2023 D157283 tried to flip the order to match the
current Apple Clang, but was reverted in
3197357b7e39a58bc7eb0600eb337ac2a1c8c225 because it brokes two tests. The code
was further changed in #70817 to add a third option.
The changes in this commit try to match Apple Clang, and incorporate the
changes in #70817, as well as fixing the tests that broke when D157283 was
applied.
From 32310bb312432bc215bec823c26f6194b0dcd447 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Rodri=CC=81guez=20Troitin=CC=83o?=
Date: Fri, 2 Feb 2024 17:26:39 -0800
Subject: [PATCH] [clang] Match -isysroot behaviour with system compiler on
Darwin
The current Apple Clang behaviour is to prefer `-isysroot` vs libc++
headers side-by-side the compiler. This has been like that for several
Xcode versions, at least since Xcode 14.
The code was originally written in D89001 chosing the order that was
correct at the time for Apple Clang. In 2023 D157283 tried to flip the
order to match the current Apple Clang, but was reverted in
3197357b7e39a58bc7eb0600eb337ac2a1c8c225 because it brokes two tests.
The code was further changed in #70817 to add a third option.
The changes in this commit try to match Apple Clang, and incorporate the
changes in #70817, as well as fixing the tests that broke when D157283
was applied.
---
clang/lib/Driver/ToolChains/Darwin.cpp| 32 +++
.../Driver/darwin-header-search-libcxx.cpp| 41 ++-
.../clang-check-mac-libcxx-abspath.cpp| 3 +-
.../clang-check-mac-libcxx-relpath.cpp| 3 +-
4 files changed, 41 insertions(+), 38 deletions(-)
diff --git a/clang/lib/Driver/ToolChains/Darwin.cpp
b/clang/lib/Driver/ToolChains/Darwin.cpp
index fae8ad1a958ad..004a19284ee4a 100644
--- a/clang/lib/Driver/ToolChains/Darwin.cpp
+++ b/clang/lib/Driver/ToolChains/Darwin.cpp
@@ -2515,20 +2515,31 @@ void DarwinClang::AddClangCXXStdlibIncludeArgs(
switch (GetCXXStdlibType(DriverArgs)) {
case ToolChain::CST_Libcxx: {
// On Darwin, libc++ can be installed in one of the following places:
-// 1. Alongside the compiler in /include/c++/v1
-// 2. Alongside the compiler in /../include/c++/v1
-// 3. In a SDK (or a custom sysroot) in /usr/include/c++/v1
+// 1. In a SDK (or a custom sysroot) in /usr/include/c++/v1
+// 2. Alongside the compiler in /include/c++/v1
+// 3. Alongside the compiler in /../include/c++/v1
//
// The precedence of paths is as listed above, i.e. we take the first path
// that exists. Note that we never include libc++ twice -- we take the
first
// path that exists and don't send the other paths to CC1 (otherwise
// include_next could break).
//
-// Also note that in most cases, (1) and (2) are exactly the same path.
+// Also note that in most cases, (2) and (3) are exactly the same path.
// Those two paths will differ only when the `clang` program being run
// is actually a symlink to the real executable.
// Check for (1)
+llvm::SmallString<128> SysrootUsr = Sysroot;
+llvm::sys::path::append(SysrootUsr, "usr", "include", "c++", "v1");
+if (getVFS().exists(SysrootUsr)) {
+ addSystemInclude(DriverArgs, CC1Args, SysrootUsr);
+ return;
+} else if (DriverArgs.hasArg(options::OPT_v)) {
+ llvm::errs() << "ignoring nonexistent directory \"" << SysrootUsr
+ << "\"\n";
+}
+
+// Check for (2)
// Get from '/bin' to '/include/c++/v1'.
// Note that InstallBin can be relative, so we use '..' instead of
// parent_path.
@@ -2543,7 +2554,7 @@ void DarwinClang::AddClangCXXStdlibIncludeArgs(
<< "\"\n";
}
-// (2) Check for the folder where the executable is located, if different.
+// (3) Check for the folder where the executable is located, if different.
if (getDriver().getInstalledDir() != getDriver().Dir) {
InstallBin = llvm::StringRef(getDriver().Dir);
llvm::sys::path::append(InstallBin, "..", "include", "c++", "v1");
@@ -2556,17 +2567,6 @@ void DarwinClang::AddClangCXXStdlibIncludeArgs(
}
}
-// Otherwise, check for (3)
-llvm::SmallString<128> SysrootUsr = Sysroot;
-llvm::sys::path::append(SysrootUsr, "usr", "include", "c++", "v1");
-if (getVFS().exists(SysrootUsr)) {
- addSystemInclude(DriverArgs, CC1Args, SysrootUsr);
- return;
-} else if (DriverArgs.hasArg(options::OPT_v)) {
- llvm::errs() << "ignoring nonexistent directory \"" << SysrootUsr
- << "\"\n";
-}
-
// Otherwise, don't add any path.
break;
}
d
