[clang][driver] Add \<executable\>/../include/c++/v1 to include path on Darwin #70817
Conversation
On macOS, when clang is invoked via a symlink, since the InstalledDir is where the link is located, the C++ headers are not identified and the default system headers are used. This fix adds a second check using the folder where the executable is located.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-driver Author: Liviu Ionescu (ilg-ul) ChangesOn macOS, when clang is invoked via a symlink, since the InstalledDir is where the link is located, the C++ headers are not identified and the default system headers are used. % ln -s /Users/ilg/Library/xPacks/@<!-- -->xpack-dev-tools/clang/15.0.7-3.1/.content/bin/clang++ ~/tmp/clang++
% ~/tmp/clang++ -v hello.cpp -stdlib=libc++
xPack x86_64 clang version 15.0.7 (https://github.com/xpack-dev-tools/clang-xpack 9b1ff65945b1aaddfe7c0c4d99001ebca5d67b03)
Target: x86_64-apple-darwin21.6.0
Thread model: posix
InstalledDir: /Users/ilg/tmp/. <--- !!!
ignoring nonexistent directory "/Users/ilg/tmp/./../include/c++/v1" <--- !!!
"/Users/ilg/Library/xPacks/@<!-- -->xpack-dev-tools/clang/15.0.7-3.1/.content/bin/clang-15" -cc1 -triple x86_64-apple-macosx12.0.0 -Wundef-prefix=TARGET_OS_ -Werror=undef-prefix -Wdeprecated-objc-isa-usage -Werror=deprecated-objc-isa-usage -emit-obj -mrelax-all --mrelax-relocations -disable-free -clear-ast-before-backend -disable-llvm-verifier -discard-value-names -main-file-name 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 -target-cpu penryn -tune-cpu generic -mllvm -treat-scalable-fixed-error-as-warning -debugger-tuning=lldb -target-linker-version 409.12 -v -fcoverage-compilation-dir=/Users/ilg/tmp -resource-dir /Users/ilg/Library/xPacks/@<!-- -->xpack-dev-tools/clang/15.0.7-3.1/.content/lib/clang/15.0.7 -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk -stdlib=libc++ -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/Library/xPacks/@<!-- -->xpack-dev-tools/clang/15.0.7-3.1/.content/lib/clang/15.0.7/include -internal-externc-isystem /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include -fdeprecated-macro -fdebug-compilation-dir=/Users/ilg/tmp -ferror-limit 19 -stack-protector 1 -fblocks -fencode-extended-block-signature -fregister-global-dtors-with-atexit -fgnuc-version=4.2.1 -fcxx-exceptions -fexceptions -fmax-type-align=16 -fcolor-diagnostics -D__GCC_HAVE_DWARF2_CFI_ASM=1 -o /var/folders/gr/13tt3vcd7m1gnbhwtkmf5cnw0000gn/T/hello-a87934.o -x c++ hello.cpp
clang -cc1 version 15.0.7 based upon LLVM 15.0.7 default target x86_64-apple-darwin21.6.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 <--- Wrong!
/Users/ilg/Library/xPacks/@<!-- -->xpack-dev-tools/clang/15.0.7-3.1/.content/lib/clang/15.0.7/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/Library/xPacks/@<!-- -->xpack-dev-tools/clang/15.0.7-3.1/.content/lib/libLTO.dylib -no_deduplicate -dynamic -arch x86_64 -macosx_version_min 12.0.0 -syslibroot /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk -o a.out /var/folders/gr/13tt3vcd7m1gnbhwtkmf5cnw0000gn/T/hello-a87934.o -lc++ -lSystem /Users/ilg/Library/xPacks/@<!-- -->xpack-dev-tools/clang/15.0.7-3.1/.content/lib/clang/15.0.7/lib/darwin/libclang_rt.osx.aUsing the system headers instead of the toolchain headers may have very subtle consequences, sometimes leading to compile errors which are hard to diagnose. This fix adds a second check using the folder where the executable is located. Full diff: https://github.com/llvm/llvm-project/pull/70817.diff 1 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/Darwin.cpp b/clang/lib/Driver/ToolChains/Darwin.cpp
index f28e08d81bf29b4..de55307385966cf 100644
--- a/clang/lib/Driver/ToolChains/Darwin.cpp
+++ b/clang/lib/Driver/ToolChains/Darwin.cpp
@@ -2494,6 +2494,19 @@ void DarwinClang::AddClangCXXStdlibIncludeArgs(
<< "\"\n";
}
+ // Check for the folder where the executable is located, if different.
+ if (getDriver().getInstalledDir() != getDriver().Dir) {
+ InstallBin = llvm::StringRef(getDriver().Dir.c_str());
+ llvm::sys::path::append(InstallBin, "..", "include", "c++", "v1");
+ if (getVFS().exists(InstallBin)) {
+ addSystemInclude(DriverArgs, CC1Args, InstallBin);
+ return;
+ } else if (DriverArgs.hasArg(options::OPT_v)) {
+ llvm::errs() << "ignoring nonexistent directory \"" << InstallBin
+ << "\"\n";
+ }
+ }
+
// Otherwise, check for (2)
llvm::SmallString<128> SysrootUsr = Sysroot;
llvm::sys::path::append(SysrootUsr, "usr", "include", "c++", "v1");
|
|
@Endilll, @AaronBallman, @bjope, @mstorsjo, this is a cleanup of a previous PR (#68091) where you contributed. Could you take a second look? |
|
I am unfamiliar with Darwin and @jansvoboda11 @ldionne can give a more authoritative answer, but I can share some feelings.
IMHO this patch does not move toward the ideal situation as |
The current behaviour is:
In most cases, when invoked via a link, there is no The proposed behaviour is:
Only one of them is used. |
|
Is there any reason why we don't just use |
That's a good question. There was a long discussion in #68091, and my understanding is that, for compatibility reasons, the current behaviour must be preserved.
In my use case, and probably in most use cases, the location where the link comes from is a plain folder, not a hierarchy specific to a toolchain. However, for testing purposes, including several clang tests, such a hierarchy is created, with other headers, and a link is placed there, with the expectation that the compiler will use that environment and not the one where the toolchain is actually installed. There are many places in the source code where something is searched for first in the This patch uses exactly the same strategy for identifying the C++ headers on Darwin. Without the patch, when using links the compiler chooses the SDK headers, which is plainly wrong and damaging. |
I would have expected the opposite. Most non-windows clang toolchains that I know of typically ship As an aside, there is also driver mode selection logic that depends heavily on the specific names of these symlinks, e.g. for C vs C++ mode, clang-cl mode, and/or cross-compiling to a specific target when prefixed with the target triple e.g. Don't consider this an objection, but: for your use case, can you stick symlinks in to preserve that toolchain folder structure relative to the |
I'm not sure I understand your proposal, but I'm afraid changing the structure of the symlinks is not possible, it is done automatically by a tool. The example above was just to demonstrate the bug. In the actual use case, which is inspired by the npm use case, in the project there is a folder This project in particular has binary dependencies only to clang, but other projects can also have dependencies to cmake, ninja, etc, and all binaries are linked in the With the proposed change, clang correctly idenifies the proper |
To be fair: tools can be changed, and in fact you're proposing to change one ;) But that said, now I see what you're up against. |
|
Can you clearly state why you are against this PR? You don't consider the current behaviour of silently using the wrong headers a bug, or the proposed solution is not correct? |
I am not against it; I just wanted to talk through all the options. |
|
Then can this bug fix be accepted? Are there any other solutions? |
|
Can someone review this patch? Exactly the same strategy (check if the desired subfolder is below |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
After several more attempts I identified the reason for the failure: the folder structure in the build location does not include the After creating this include folder, the Linux test finally passed. I don't know if creating this folder is acceptable for the testing infrastructure, but if we want this test together with the bug fix, we probably have no other choice. Louis @ldionne, could you review the test code? |
|
I pushed a new test that copies The Linux build passed. Any other comments? |
|
@ldionne, do you agree with the current changes? Since your pending request for changes currently blocks this PR. |
| @@ -172,3 +172,32 @@ | |||
| // RUN: --check-prefix=CHECK-LIBCXX-STDLIB-UNSPECIFIED %s | |||
| // CHECK-LIBCXX-STDLIB-UNSPECIFIED: "-cc1" | |||
| // CHECK-LIBCXX-STDLIB-UNSPECIFIED: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1" | |||
|
|
|||
| // ---------------------------------------------------------------------------- | |||
There was a problem hiding this comment.
Can you try to integrate your test to the existing ones a bit more closely? They are not really specific to xpm/npm, so it makes sense to support them in a platform agnostic way.
For example, we need to add a test where we set up a symlinked Clang and then run with a toolchain that has headers in it, and confirm that this takes precedence over the ones in the symlink directory. In other words, we now have (1), (2) and (3) places where we look for headers in. Previously, we pretty exhaustively checked the combinations for the two locations we checked -- now we should at least try to check:
- Headers in (1) and in (2) -> (1) is preferred over (2)
- Headers in (2) and in (3) -> (2) is preferred over (3) -- you already check that with your current test
- Headers in (2) and nowhere else -> (2) is used
Also, we should make the comments in the tests platform-agnostic. We also probably want to insert the tests in the right place in the file and I suspect that using (1) (2) and (3) terminology to explain what we're testing would help.
There was a problem hiding this comment.
BTW this looks good to me apart from these test improvements.
It passed on my Mac, let's hope that the CI will pass too. But, not related to this PR, on my Mac, I always had several tests failing: Is this expected? |
|
That's a question for the Clang regulars -- I generally don't run those tests since I primarily work on libc++, not Clang. |
|
The CI passed. Do we need a second review, or the PR can be merged? |
|
Thank you, Louis @ldionne, I highly appreciate your help! |
|
The ppc64 build failed with: How can I fix the test now? |
|
I opened #72924 to fix this bug. |
… on AIX (#72924) The PR #70817 introduced a small bug, the tests failed on ppc64-aix with: ``` RUN: at line 186: mkdir -pv /home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/tools/clang/test/Driver/Output/darwin-header-search-libcxx.cpp.tmp/install/bin + mkdir -pv /home/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/tools/clang/test/Driver/Output/darwin-header-search-libcxx.cpp.tmp/install/bin mkdir: illegal option -- v Usage: mkdir [-p] [-e] [-m mode] Directory ... ``` This PR removes the verbose flag from both `mkdir` and `ln -s`.
I am more convinced that we should unify I believe that Created #80527 |
On macOS, when clang is invoked via a symlink, since the InstalledDir is where the link is located, the C++ headers are not identified and the default system headers are used.
Using the system headers instead of the toolchain headers may have very subtle consequences, sometimes leading to compile errors which are hard to diagnose.
This fix adds a second check using the folder where the executable is located.