Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang] Match -isysroot behaviour with system compiler on Darwin #80524

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

drodriguez
Copy link
Contributor

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 3197357 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.

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
3197357 because it brokes two tests.
The code was further changed in llvm#70817 to add a third option.

The changes in this commit try to match Apple Clang, and incorporate the
changes in llvm#70817, as well as fixing the tests that broke when D157283
was applied.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Feb 3, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 3, 2024

@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 3197357 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 --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 <install>/include/c++/v1
-    // 2. Alongside the compiler in <clang-executable-folder>/../include/c++/v1
-    // 3. In a SDK (or a custom sysroot) in <sysroot>/usr/include/c++/v1
+    // 1. In a SDK (or a custom sysroot) in <sysroot>/usr/include/c++/v1
+    // 2. Alongside the compiler in <install>/include/c++/v1
+    // 3. Alongside the compiler in <clang-executable-folder>/../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 '<install>/bin' to '<install>/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:     --target=x86_64-apple-darwin \
@@ -35,8 +35,8 @@
 // RUN:               -DSYSROOT=%S/Inputs/basic_darwin_sdk_no_libcxx \
 // RUN:               --check-prefix=CHECK-LIBCXX-TOOLCHAIN-2 %s
 // CHECK-LIBCXX-TOOLCHAIN-2: "-cc1"
-// CHECK-LIBCXX-TOOLCHAIN-2: "-internal-isystem" "[[TOOLCHAIN]]/usr/bin/../include/c++/v1"
 // CHECK-LIBCXX-TOOLCHAIN-2-NOT: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
+// CHECK-LIBCXX-TOOLCHAIN-2: "-internal-isystem" "[[TOOLCHAIN]]/usr/bin/../include/c++/v1"
 
 // Check with only headers in the sysroot (those should be used).
 //
@@ -49,11 +49,11 @@
 // RUN:               -DTOOLCHAIN=%S/Inputs/basic_darwin_toolchain_no_libcxx \
 // RUN:               --check-prefix=CHECK-LIBCXX-SYSROOT-1 %s
 // CHECK-LIBCXX-SYSROOT-1: "-cc1"
-// CHECK-LIBCXX-SYSROOT-1: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
 // CHECK-LIBCXX-SYSROOT-1-NOT: "-internal-isystem" "[[TOOLCHAIN]]/usr/bin/../include/c++/v1"
+// CHECK-LIBCXX-SYSROOT-1: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
 
 // Check with both headers in the sysroot and headers alongside the installation
-// (the headers in the toolchain should be preferred over the <sysroot> headers).
+// (the headers in the <sysroot> headers should be preferred over the toolchain).
 // Ensure that both -isysroot and --sysroot work, and that isysroot has precedence
 // over --sysroot.
 //
@@ -89,8 +89,8 @@
 // RUN:               --check-prefix=CHECK-LIBCXX-SYSROOT_AND_TOOLCHAIN-1 %s
 //
 // CHECK-LIBCXX-SYSROOT_AND_TOOLCHAIN-1: "-cc1"
-// CHECK-LIBCXX-SYSROOT_AND_TOOLCHAIN-1: "-internal-isystem" "[[TOOLCHAIN]]/usr/bin/../include/c++/v1"
-// CHECK-LIBCXX-SYSROOT_AND_TOOLCHAIN-1-NOT: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
+// CHECK-LIBCXX-SYSROOT_AND_TOOLCHAIN-1-NOT: "-internal-isystem" "[[TOOLCHAIN]]/usr/bin/../include/c++/v1"
+// CHECK-LIBCXX-SYSROOT_AND_TOOLCHAIN-1: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
 
 // Make sure that using -nostdinc, -nostdinc++ or -nostdlib will drop both the toolchain
 // C++ include path and the sysroot one.
@@ -157,8 +157,8 @@
 // RUN:   | FileCheck -DSYSROOT=%S/Inputs/basic_darwin_sdk_no_libcxx \
 // RUN:               -DTOOLCHAIN=%S/Inputs/basic_darwin_toolchain_no_libcxx \
 // RUN:               --check-prefix=CHECK-LIBCXX-MISSING-BOTH %s
-// CHECK-LIBCXX-MISSING-BOTH: ignoring nonexistent directory "[[TOOLCHAIN]]/usr/bin/../include/c++/v1"
 // CHECK-LIBCXX-MISSING-BOTH: ignoring nonexistent directory "[[SYSROOT]]/usr/include/c++/v1"
+// CHECK-LIBCXX-MISSING-BOTH: ignoring nonexistent directory "[[TOOLCHAIN]]/usr/bin/../include/c++/v1"
 
 // Make sure that on Darwin, we use libc++ header search paths by default even when
 // -stdlib= isn't passed.
@@ -175,9 +175,9 @@
 
 // ----------------------------------------------------------------------------
 // On Darwin, libc++ can be installed in one of the following places:
-// 1. Alongside the compiler in <install>/include/c++/v1
-// 2. Alongside the compiler in <clang-executable-folder>/../include/c++/v1
-// 3. In a SDK (or a custom sysroot) in <sysroot>/usr/include/c++/v1
+// 1. In a SDK (or a custom sysroot) in <sysroot>/usr/include/c++/v1
+// 2. Alongside the compiler in <install>/include/c++/v1
+// 3. Alongside the compiler in <clang-executable-folder>/../include/c++/v1
 
 // The build folders do not have an `include/c++/v1`; create a new
 // local folder hierarchy that meets this requirement.
@@ -187,7 +187,7 @@
 // RUN: cp %clang %t/install/bin/clang
 // RUN: mkdir -p %t/install/include/c++/v1
 
-// Headers in (1) and in (2) -> (1) is preferred over (2)
+// Headers in (2) and in (3) -> (2) is preferred over (3)
 // RUN: rm -rf %t/symlinked1
 // RUN: mkdir -p %t/symlinked1/bin
 // RUN: ln -sf %t/install/bin/clang %t/symlinked1/bin/clang
@@ -196,16 +196,16 @@
 // RUN: %t/symlinked1/bin/clang -### %s -fsyntax-only 2>&1 \
 // RUN:     --target=x86_64-apple-darwin \
 // RUN:     -stdlib=libc++ \
-// RUN:     -isysroot %S/Inputs/basic_darwin_sdk_usr_cxx_v1 \
+// RUN:     -isysroot %S/Inputs/basic_darwin_sdk_no_libcxx \
 // RUN:   | FileCheck -DSYMLINKED=%t/symlinked1 \
 // RUN:               -DTOOLCHAIN=%t/install \
-// RUN:               -DSYSROOT=%S/Inputs/basic_darwin_sdk_usr_cxx_v1 \
+// RUN:               -DSYSROOT=%S/Inputs/basic_darwin_sdk_no_libcxx \
 // RUN:               --check-prefix=CHECK-SYMLINKED-INCLUDE-CXX-V1 %s
-// CHECK-SYMLINKED-INCLUDE-CXX-V1: "-internal-isystem" "[[SYMLINKED]]/bin/../include/c++/v1"
 // CHECK-SYMLINKED-INCLUDE-CXX-V1-NOT: "-internal-isystem" "[[TOOLCHAIN]]/bin/../include/c++/v1"
 // CHECK-SYMLINKED-INCLUDE-CXX-V1-NOT: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
+// CHECK-SYMLINKED-INCLUDE-CXX-V1: "-internal-isystem" "[[SYMLINKED]]/bin/../include/c++/v1"
 
-// Headers in (2) and in (3) -> (2) is preferred over (3)
+// Headers in (1) and in (3) -> (1) is preferred over (3)
 // RUN: rm -rf %t/symlinked2
 // RUN: mkdir -p %t/symlinked2/bin
 // RUN: ln -sf %t/install/bin/clang %t/symlinked2/bin/clang
@@ -216,16 +216,17 @@
 // RUN:     -isysroot %S/Inputs/basic_darwin_sdk_usr_cxx_v1 \
 // RUN:   | FileCheck -DTOOLCHAIN=%t/install \
 // RUN:               -DSYSROOT=%S/Inputs/basic_darwin_sdk_usr_cxx_v1 \
-// RUN:               --check-prefix=CHECK-TOOLCHAIN-INCLUDE-CXX-V1 %s
-// CHECK-TOOLCHAIN-INCLUDE-CXX-V1: "-internal-isystem" "[[TOOLCHAIN]]/bin/../include/c++/v1"
-// CHECK-TOOLCHAIN-INCLUDE-CXX-V1-NOT: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
+// RUN:               --check-prefix=CHECK-SYSROOT-INCLUDE-CXX-V1 %s
+// CHECK-SYSROOT-INCLUDE-CXX-V1-NOT: "-internal-isystem" "[[TOOLCHAIN]]/bin/../include/c++/v1"
+// CHECK-SYSROOT-INCLUDE-CXX-V1: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
 
-// Headers in (2) and nowhere else -> (2) is used
+// Headers in (3) and nowhere else -> (3) is used
 // RUN: %t/symlinked2/bin/clang -### %s -fsyntax-only 2>&1 \
 // RUN:     --target=x86_64-apple-darwin \
 // RUN:     -stdlib=libc++ \
-// RUN:     -isysroot %S/Inputs/basic_darwin_sdk_usr_cxx_v1 \
+// RUN:     -isysroot %S/Inputs/basic_darwin_sdk_no_libcxx \
 // RUN:   | FileCheck -DTOOLCHAIN=%t/install \
 // RUN:               -DSYSROOT=%S/Inputs/basic_darwin_sdk_no_libcxx \
 // RUN:               --check-prefix=CHECK-TOOLCHAIN-NO-SYSROOT %s
+// CHECK-TOOLCHAIN-NO-SYSROOT-NOT: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
 // CHECK-TOOLCHAIN-NO-SYSROOT: "-internal-isystem" "[[TOOLCHAIN]]/bin/../include/c++/v1"
diff --git a/clang/test/Tooling/clang-check-mac-libcxx-abspath.cpp b/clang/test/Tooling/clang-check-mac-libcxx-abspath.cpp
index 476ba3ce0849a..e2152e82e621c 100644
--- a/clang/test/Tooling/clang-check-mac-libcxx-abspath.cpp
+++ b/clang/test/Tooling/clang-check-mac-libcxx-abspath.cpp
@@ -3,12 +3,13 @@
 //
 // RUN: rm -rf %t
 // RUN: mkdir %t
+// RUN: mkdir %t/fake-sysroot
 //
 // Install the mock libc++ (simulates the libc++ directory structure).
 // RUN: cp -r %S/Inputs/mock-libcxx %t/
 //
 // Pretend clang is installed beside the mock library that we provided.
-// RUN: echo '[{"directory":"%t","command":"%t/mock-libcxx/bin/clang++ -stdlib=libc++ -target x86_64-apple-darwin -c test.cpp","file":"test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
+// RUN: echo '[{"directory":"%t","command":"%t/mock-libcxx/bin/clang++ -stdlib=libc++ -target x86_64-apple-darwin -isysroot %t/fake-sysroot -c test.cpp","file":"test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
 // RUN: cp "%s" "%t/test.cpp"
 // clang-check will produce an error code if the mock library is not found.
 // RUN: clang-check -p "%t" "%t/test.cpp"
diff --git a/clang/test/Tooling/clang-check-mac-libcxx-relpath.cpp b/clang/test/Tooling/clang-check-mac-libcxx-relpath.cpp
index 099be5ecd4984..37d9f9d89f152 100644
--- a/clang/test/Tooling/clang-check-mac-libcxx-relpath.cpp
+++ b/clang/test/Tooling/clang-check-mac-libcxx-relpath.cpp
@@ -3,12 +3,13 @@
 //
 // RUN: rm -rf %t
 // RUN: mkdir %t
+// RUN: mkdir -p %t/fake-sysroot
 //
 // Install the mock libc++ (simulates the libc++ directory structure).
 // RUN: cp -r %S/Inputs/mock-libcxx %t/
 //
 // Pretend clang is installed beside the mock library that we provided.
-// RUN: echo '[{"directory":"%t","command":"mock-libcxx/bin/clang++ -stdlib=libc++ -target x86_64-apple-darwin -c test.cpp","file":"test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
+// RUN: echo '[{"directory":"%t","command":"mock-libcxx/bin/clang++ -stdlib=libc++ -target x86_64-apple-darwin -isysroot %t/fake-sysroot -c test.cpp","file":"test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
 // RUN: cp "%s" "%t/test.cpp"
 // clang-check will produce an error code if the mock library is not found.
 // RUN: clang-check -p "%t" "%t/test.cpp"

@drodriguez
Copy link
Contributor Author

@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!

@ilg-ul
Copy link
Contributor

ilg-ul commented Feb 3, 2024

they change the behaviour you introduced in #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.

@drodriguez
Copy link
Contributor Author

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.

@ldionne
Copy link
Member

ldionne commented Feb 5, 2024

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).

@ldionne
Copy link
Member

ldionne commented Feb 5, 2024

@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 <SDK> (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.

@drodriguez
Copy link
Contributor Author

drodriguez commented Feb 5, 2024

@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).

These are 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 <cxxabi.h>" > 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 <cxxabi.h>
         ^~~~~~~~~~
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.

@dmpolukhin
Copy link
Contributor

dmpolukhin commented Feb 5, 2024

@ldionne I also haven't seen an 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 versions 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!

@drodriguez
Copy link
Contributor Author

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.

@ldionne
Copy link
Member

ldionne commented Feb 5, 2024

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.

@dmpolukhin
Copy link
Contributor

@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.

@drodriguez
Copy link
Contributor Author

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.

@ldionne
Copy link
Member

ldionne commented Feb 6, 2024

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.

@dmpolukhin
Copy link
Contributor

@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 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.

@ilg-ul
Copy link
Contributor

ilg-ul commented Feb 7, 2024

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 #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.

@ldionne
Copy link
Member

ldionne commented Feb 7, 2024

@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 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).

@drodriguez
Copy link
Contributor Author

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.

@drodriguez
Copy link
Contributor Author

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 <build dir>/include/c++/v1 (because libc++ has been configured, but not built), and <build dir>/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).

@ilg-ul
Copy link
Contributor

ilg-ul commented Feb 7, 2024

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/13tt3vcd7m1gnbhwtkmf5cnw0000gn/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/13tt3vcd7m1gnbhwtkmf5cnw0000gn/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/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 /Users/ilg/Work/xpack-dev-tools-build/clang-18.1.0-1/darwin-x64/application/bin/../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/13tt3vcd7m1gnbhwtkmf5cnw0000gn/T/simple-hello-c935cf.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:
 /Users/ilg/Work/xpack-dev-tools-build/clang-18.1.0-1/darwin-x64/application/bin/../include/c++/v1   <---  CORRECT!
 /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/13tt3vcd7m1gnbhwtkmf5cnw0000gn/T/simple-hello-c935cf.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

So, from my point of view, this patch is not acceptable.

@drodriguez
Copy link
Contributor Author

@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?

@ilg-ul
Copy link
Contributor

ilg-ul commented Feb 7, 2024

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.

@ilg-ul
Copy link
Contributor

ilg-ul commented Feb 7, 2024

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.

@drodriguez
Copy link
Contributor Author

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.

@ilg-ul
Copy link
Contributor

ilg-ul commented Feb 7, 2024

Yes, that's why I said that your patch must be more elaborated and check for the presence of -isysroot, since the internal variable may come from other settings. There is nothing magic there.

@ldionne
Copy link
Member

ldionne commented Feb 7, 2024

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.

@dmpolukhin
Copy link
Contributor

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.

@ldionne
Copy link
Member

ldionne commented Mar 5, 2024

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 <SDK> 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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants