[clang][Darwin] Try to guess the SDK root with xcrun when unspecified

[calebzulawski]

Summary

Picking up from https://reviews.llvm.org/D109460

[calebzulawski]

Change tests to specifically remove SDKROOT environment variable.

[tschuett]

clang/lib/Driver/ToolChains/Darwin.cpp

2120

Will there be an enum over the Apple variants to make this less error prone and future proof? I want a switch statement.

[calebzulawski]

Use switch when detecting OS. Limit sdkroot test to darwin platforms.

[calebzulawski]

clang/lib/Driver/ToolChains/Darwin.cpp

2120

It doesn't look like there is any enum available that _only_ contains Apple OS variants, but I did change this to a switch-case over the OS in general. Also, I noticed I missed DriverKit, so I added it.

[carlocab]

clang/lib/Driver/ToolChains/Darwin.cpp

2147

llvm_unreachable? Or assert, at least.

clang/test/Driver/darwin-sdkroot.c

1 ↗

(On Diff #469909)

I'm not sure this is the right move here, since there are a number of other test cases in this file that don't require system-darwin. We probably do want those tests to run on more buildbots to make sure problems with them are caught sooner rather than later.

Maybe we can check that /usr/bin/xcrun is executable instead?

[calebzulawski]

clang/lib/Driver/ToolChains/Darwin.cpp

2147

I'm not sure that's correct--what if you target something like x86_64-unknown-none? I figured it would be safest to fall back to the previous behavior of simply not providing a sysroot.

clang/test/Driver/darwin-sdkroot.c

1 ↗

(On Diff #469909)

That would work, but I'm not sure how to exclude a particular test if it's not found.

[calebzulawski]

Fix formatting, error on unrecognized OS, skip tests if xcrun isn't present.

[calebzulawski]

clang/lib/Driver/ToolChains/Darwin.cpp

2147

Realizing this is never hit for non-darwin targets anyway, added the llvm_unreachable.

[calebzulawski]

Fix test using RUN in CHECK prefix

[calebzulawski]

Try to fix windows test

[calebzulawski]

Move SDK detection to its own test.

[calebzulawski]

Is there any update to this? Or maybe someone I should add as a reviewer? Thanks!

[ldionne]

This LGTM.

clang/lib/Driver/ToolChains/Darwin.cpp

2084–2089

Instead of this:

On Apple platforms, C and C++ Standard Library headers are not provided with the base system.

I would say this, which is less specific to headers:

On Apple platforms, standard headers and libraries are not provided with the base system (e.g. in /usr/{include,lib}).

[calebzulawski]

Update comment

[ldionne]

You should make sure that the CI is passing before merging!

[calebzulawski]

Update diff for upstream changes

[hans]

Could we add a release note and/or update to the clang manual explaining the new behavior?

[calebzulawski]

Update user manual

[calebzulawski]

Looks like the failure was somehow in clangd? Trying to trigger another build.

[calebzulawski]

Happy new year all--
Is there anything left for me to do before this can be merged? Thanks!

[jryans]

@calebzulawski This looks ready to land to me! 🙂 Do you have commit access? If not, I can land it for you.

[calebzulawski]

@jryans I do not, I would appreciate that. Thanks!

[thakis]

This breaks oodles of tests for me: http://45.33.8.238/macm1/52994/step_7.txt

Is anyone else seeing this?

Also, independently of that, shouldn't this check that the _host_ os is macOS as well? Doesn't the current code try to run xcrun if I do clang --target=arm64-apple-macos -x c -c /dev/null on Linux? (Or am I misreading that?)

[calebzulawski]

Are those failures on Linux? I only developed this change on macOS, but the automated tests passed...

Admittedly I'm not sure what should happen when building for macOS from Linux (is there any use case where that works and does anything useful? I'm not sure). I was under the impression that the Darwin driver applied to Darwin hosts, not targets, but if that's not true, it would attempt to run xcrun. If xcrun isn't found, no error occurs but the sysroot remains unset.

[jryans]

@calebzulawski The toolchain is selected based on the target, see Driver::getToolChain.

Various groups do target macOS from Linux hosts. I'm not quite sure how the SDK is meant to be found in such a case though... @thakis, your log seems like it passes SDKROOT, so is that meant to be what's used for this host and target combo?

Let me know if I can assist by reverting the patch.

[thakis]

My bot is a Mac. It doesn't set any env vars (other than the ones that lit always sets). In theory it shouldn't matter since tests aren't supposed to depend on system headers. (Tangential: maybe there should be a flag to turn this off and %clang should expand to clang with that flag?)

For cross builds, I mostly build cc files that don't include any headers to test compiler behavior. But one could uses -isysroot like elsewhere.

I think it'd be good to revert this until it no longer tries to run xcrun on non-macs, independent of my test failures.

[thakis]

I just verified that that does indeed happen, and it does:

$ strace out/gn/bin/clang --target=x86_64-apple-macos -x c -c /dev/null 2>&1 | grep xcrun
access("/usr/bin/xcrun", F_OK)          = -1 ENOENT (No such file or directory)

[calebzulawski]

I don't think we necessarily need to revert due to the xcrun quirk since I think it's harmless, though I can provide a follow up change if necessary. I just checked Rust's source as a comparison and it appears that for macOS targets it will always attempt to invoke xcrun regardless of host as well. I could see a macOS cross-compiler environment providing an xcrun replacement, for example. The test failures are another issue but I still don't quite understand the cause.

[thakis]

In D136315#4065426, @calebzulawski wrote:

I don't think we necessarily need to revert due to the xcrun quirk since I think it's harmless, though I can provide a follow up change if necessary. I just checked Rust's source as a comparison and it appears that for macOS targets it will always attempt to invoke xcrun regardless of host as well.

Needlessly spawning additional processes doesn't seem "harmless" to me.

If you can fix quickly, sure, do that.

I could see a macOS cross-compiler environment providing an xcrun replacement, for example.

That's theoretically possible I suppose, but it doesn't exist. No mac cross compiling uses xcrun.

The test failures are another issue but I still don't quite understand the cause.

I can investigate a bit more. Which version of macOS are you running? Are you using system python3 or brew python3? I know that system python3 sets SDKROOT by default on macOS 13+ for some reason, so maybe that's related. (The bot runs macOS 13.)

[calebzulawski]

In D136315#4065481, @thakis wrote:

In D136315#4065426, @calebzulawski wrote:

I don't think we necessarily need to revert due to the xcrun quirk since I think it's harmless, though I can provide a follow up change if necessary. I just checked Rust's source as a comparison and it appears that for macOS targets it will always attempt to invoke xcrun regardless of host as well.

Needlessly spawning additional processes doesn't seem "harmless" to me.

If you can fix quickly, sure, do that.

I could see a macOS cross-compiler environment providing an xcrun replacement, for example.

That's theoretically possible I suppose, but it doesn't exist. No mac cross compiling uses xcrun.

Sorry, I agree that we shouldn't do things needlessly, but xcrun is only invoked if the compiler otherwise can't figure out the sysroot. I just tried using --target=x86_64-apple-macos on its own on Linux and sure enough the sysroot included /usr/include etc, which is surely also wrong. I suspect any existing cross-compiler environments either set SDKROOT or pass a sysroot flag, otherwise they wouldn't work (and this change doesn't affect them).

It seems osxcross does provide an xcrun replacement: https://github.com/tpoechtrager/osxcross/blob/master/wrapper/programs/xcrun.cpp

The test failures are another issue but I still don't quite understand the cause.

I can investigate a bit more. Which version of macOS are you running? Are you using system python3 or brew python3? I know that system python3 sets SDKROOT by default on macOS 13+ for some reason, so maybe that's related. (The bot runs macOS 13.)

I'm using 13.1 and I do have brew python installed. I see what you're describing, system python has SDKROOT set.

[steven_wu]

This breaks macOS bot too: https://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental/33839/

I think this changes might be more disruptive than expected. While it is encouraged to use -isysroot when building anything on Darwin platform, it is also perfectly "fine" to build without sysroot specified. Forcing a sysroot on users who are not specifying the isysroot will change the header/library search behavior.

[mysterymath]

This also appears to be breaking the Darwin build for the Fuchsia toolchain, with similar test failures to those seen by @thakis.
Given the scope of the breakages, can we revert this and reland later?

See: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-mac-x64/b8791526664823090241/overview

[calebzulawski]

One thing to throw into the mix: Apple's clang has a default sysroot configured, so with the default system compiler, there is no way to replicate this "build without a sysroot" scenario as far as I can tell. For the system compiler, I believe this behavior is a strict improvement.

Looking at all of the errors, it looks like the common thread is that when a sysroot is present, the version in the target triple (e.g. x86_64-apple-macos10) isn't respected. If this is true, I think this possibly uncovered an existing bug...

[jryans]

Since we seem to have several breakages here, it does seem best to revert for now so that a revised approach can be investigated. I will aim to revert tomorrow when back at my desk.

[calebzulawski]

@jryans thanks.

I've confirmed that this is specifically a bug with darwin targets (not macos) not respecting platform versions when an SDK is present, I can even reproduce this with clang provided with Xcode.

The following patch, for example, completely resolves all test errors for me (by disabling SDK detection for darwin targets):

diff
diff --git a/clang/lib/Driver/ToolChains/Darwin.cpp b/clang/lib/Driver/ToolChains/Darwin.cpp
index 03c28c14a0ec..a177a1d289b4 100644
--- a/clang/lib/Driver/ToolChains/Darwin.cpp
+++ b/clang/lib/Driver/ToolChains/Darwin.cpp
@@ -2141,13 +2141,14 @@ void Darwin::AddDeploymentTarget(DerivedArgList &Args) const {
       else
         SDKName = "iphoneos";
       break;
-    case llvm::Triple::OSType::Darwin:
     case llvm::Triple::OSType::MacOSX:
       SDKName = "macosx";
       break;
     case llvm::Triple::OSType::DriverKit:
       SDKName = "driverkit";
       break;
+    case llvm::Triple::OSType::Darwin:
+      break;
     default:
       llvm_unreachable("unknown kind of Darwin platform");
     }
diff --git a/clang/test/Driver/darwin-ld-platform-version-macos.c b/clang/test/Driver/darwin-ld-platform-version-macos.c
index 355df8dfc1bc..5e50d84df6da 100644
--- a/clang/test/Driver/darwin-ld-platform-version-macos.c
+++ b/clang/test/Driver/darwin-ld-platform-version-macos.c
@@ -41,9 +41,6 @@
 // ARM64_NEW_1: "-platform_version" "macos" "11.1.0" "10.14"
 // ARM64_OLD: "-macosx_version_min" "11.0.0"

-// RUN: %clang -target x86_64-apple-macos10.13 -mlinker-version=520 \
-// RUN:   -### %t.o 2>&1 \
-// RUN:   | FileCheck --check-prefix=NOSDK %s
 // RUN: %clang -target x86_64-apple-darwin17 -mlinker-version=520 \
 // RUN:   -### %t.o 2>&1 \
 // RUN:   | FileCheck --check-prefix=NOSDK %s
diff --git a/clang/test/Driver/darwin-sdk-detect.c b/clang/test/Driver/darwin-sdk-detect.c
index dff4def0568a..c5b270c526bd 100644
--- a/clang/test/Driver/darwin-sdk-detect.c
+++ b/clang/test/Driver/darwin-sdk-detect.c
@@ -3,7 +3,7 @@
 // Check that we default to running `xcrun --show-sdk-path` if there is no
 // SDKROOT defined in the environment.
 //
-// RUN: env -u SDKROOT %clang -target x86_64-apple-darwin -c %s -### 2> %t.log
+// RUN: env -u SDKROOT %clang -target x86_64-apple-macos -c %s -### 2> %t.log
 // RUN: FileCheck --check-prefix=CHECK-XC < %t.log %s
 //
 // CHECK-XC: clang

[thakis]

I've confirmed that this breaks a pretty vanilla build setup.

/Applications/CMake.app/Contents/bin/cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_ENABLE_PROJECTS='clang;compiler-rt;clang-tools-extra;lld' -DLLVM_APPEND_VC_REV=NO -DLLVM_TARGETS_TO_BUILD='X86' -DCMAKE_C_COMPILER=$PWD/../llvm-project/out/gn/bin/clang -DCMAKE_CXX_COMPILER=$PWD/../llvm-project/out/gn/bin/clang++ -DCMAKE_OSX_SYSROOT=$HOME/llvm-project/sysroot/MacOSX.sdk -DDARWIN_macosx_CACHED_SYSROOT=$HOME/src/llvm-project/sysroot/MacOSX.sdk -DDARWIN_iphoneos_CACHED_SYSROOT=$HOME/src/llvm-project/sysroot/iPhoneOS.sdk -DDARWIN_iphonesimulator_CACHED_SYSROOT=$HOME/src/llvm-project/sysroot/iPhoneSimulator.sdk ../llvm-project/llvm

% time caffeinate ninja check-clang
...
  Clang :: Driver/arc.c
  Clang :: Driver/clang-g-opts.c
  Clang :: Driver/clang-translation.c
  Clang :: Driver/darwin-debug-flags.c
  Clang :: Driver/darwin-header-search-system.cpp
  Clang :: Driver/darwin-ld-platform-version-macos.c
  Clang :: Driver/darwin-ld.c
  Clang :: Driver/darwin-multiarch-arm.c
  Clang :: Driver/darwin-objc-options.m
  Clang :: Driver/darwin-version.c
  Clang :: Driver/debug-options.c
  Clang :: Driver/fsanitize.c
  Clang :: Driver/macos-apple-silicon-slice-link-libs.cpp
  Clang :: Driver/target-triple-deployment.c

This is on macOS 13.1.

(sysroot dir created via llvm/utils/sysroot.py make-fake --out-dir sysroot)

[thakis]

Reverted in a5f446bc4bb1ac78d6852cc8e251a1229899b783 for now.

[calebzulawski]

@thakis thanks. I have an updated revision that can be reviewed properly.

[calebzulawski]

This revision is basically the same as before, but with two changes:

• darwin targets (e.g. x86_64-apple-darwin) do not automatically detect the SDK. There is another bug where these targets don't seem to properly pass the target version (e.g. x86_64-apple-darwin9) whenever an SDK is provided.
• A new flag --no-detect-xcode disables this detection.

Fingers crossed, this passes all tests for me locally.

[steven_wu]

In D136315#4067205, @calebzulawski wrote:

One thing to throw into the mix: Apple's clang has a default sysroot configured, so with the default system compiler, there is no way to replicate this "build without a sysroot" scenario as far as I can tell. For the system compiler, I believe this behavior is a strict improvement.

No, apple's clang doesn't have a default sysroot. When you run /usr/bin/clang, which is a shim that calls xcrun, it finds the clang in your toolchain and invokes it with SDKROOT environment variable. That is the behavior of the shim, not the behavior of clang.

[calebzulawski]

That makes more sense, I thought perhaps it was using DEFAULT_SYSROOT. The shim isn't smart enough to choose the sysroot from the target unfortunately.

It looks like the only error is unrelated to this change, something with concepts in libc++.

[steven_wu]

In D136315#4069582, @calebzulawski wrote:

That makes more sense, I thought perhaps it was using DEFAULT_SYSROOT. The shim isn't smart enough to choose the sysroot from the target unfortunately.

It looks like the only error is unrelated to this change, something with concepts in libc++.

The shim only works if you invoke /usr/bin/clang, and that shim is for building host OS. I am not sure it is a desirable behavior for clang to call into xcrun because it can be very slow on first invocation and you will get error if you don't have Xcode or Commandlinetools installed (on a basic macOS install basically).

I wonder if the better solution is print a very actionable diagnostics when building c++ but standard c++ library cannot be found.

[calebzulawski]

If SDKROOT is set or -isysroot is passed, xcrun won't be called so there is no regression for existing use cases like CMake. Additionally, if xcrun isn't found, no error is emitted. I also added the --no-detect-xcode flag based on similar feedback that it may not always be desirable.

Better diagnostics would certainly be helpful, but for my particular use case, a diagnostic wouldn't be sufficient. I am creating a set of default configuration files for cross compilation, e.g. aarch64-android-linux-clang++.cfg contains -isysroot with a known fixed path. This allows me to pass clang++ --target=aarch64-android-linux with no additional flags to cross compile. I would like to do the same for Apple targets (not with the system compiler), but since the path is not fixed I cannot embed -isysroot. If there is enough opposition to this being default behavior, I'd be open to it being opt-in with something like --detect-xcode.

[steven_wu]

This is not an apple platform problem. This is a cross compile problem where you have a SDK that is installed for cross compile target. In those cases, you just can't hard code the path to isysroot, and figuring out the sysroot is generally considered a problem for the build system, not the problem for compiler. If you have a config file, you can definitely setup SDKROOT in your configuration system after running xcrun. A similar but not identical problem is if you just install clang package but didn't install dev packages on a linux platform where you just can't find any headers. Same error, just different causes.

To be clear, I am not against adding features in clang to make user experience better, but I think this needs some polish to addresses problem mentioned above transparently.

clang/test/Driver/darwin-sdk-detect.c

2

This test won't work in all conditions. Like I said, you requires to have a Xcode/CommandLineTools installation to make xcrun work.

Maybe you can make the argument that you are compiling clang, so you must have one of those installed, but CommandLineTools definitely doesn't have iOS SDK for the second test.

[calebzulawski]

In my situation, at least, I am the vendor of the toolchain and my configuration file contains --sysroot=<CFGDIR>/../path/to/sysroot with a known relative path to where the sysroot is distributed. Apple is unique in this situation, since I am not distributing the sysroot, so there is no equivalent to using the <CFGDIR> variable. There is no way to invoke xcrun or set SDKROOT from config files, only pass flags.

It is true that the build system could do it, but I think it's reasonable for the driver to make a guess when not provided. I don't think it's all that different than detection of libstdc++ from GCC installs.

[calebzulawski]

clang/test/Driver/darwin-sdk-detect.c

2

True, maybe we can just remove the iOS test if you think that's sufficient. Otherwise, is there any way to make the have the test check that either the -isysroot flag is present with that pattern, or not present at all? I'm not too familiar with llvm-lit, I just browsed a few other examples before writing this.

[steven_wu]

In D136315#4069849, @calebzulawski wrote:

In my situation, at least, I am the vendor of the toolchain and my configuration file contains --sysroot=<CFGDIR>/../path/to/sysroot with a known relative path to where the sysroot is distributed. Apple is unique in this situation, since I am not distributing the sysroot, so there is no equivalent to using the <CFGDIR> variable. There is no way to invoke xcrun or set SDKROOT from config files, only pass flags.

It is true that the build system could do it, but I think it's reasonable for the driver to make a guess when not provided. I don't think it's all that different than detection of libstdc++ from GCC installs.

I definitely feel like if to fix your problem, a reverse option like -infer-sdkroot-from-xcrun might be better/safer but it doesn't help the general case where people doesn't know /usr/bin/clang is different from a regular clang that a regular clang needs to pass -isysroot.

@arphaman Any suggestions?

clang/test/Driver/darwin-sdk-detect.c

2

I don't think you can write a FileCheck with that. The best way to do this test might be in clang/test/Driver/lit.local.cfg write checks to make sure:

• Darwin platform
• xcrun exists
• xcrun can resolve all the platforms you want to test

Then add a new feature which you can REQUIRES in this test case.

[calebzulawski]

In D136315#4070121, @steven_wu wrote:

I definitely feel like if to fix your problem, a reverse option like -infer-sdkroot-from-xcrun might be better/safer but it doesn't help the general case where people doesn't know /usr/bin/clang is different from a regular clang that a regular clang needs to pass -isysroot.

I reversed the option for now since it seems the least objectionable. I figure a future change could make this behavior automatic, or maybe even a compile-time configurable option? Regarding the shim, if this option were to override SDKROOT it would still work with the shim, though I'm not sure which should be higher priority. It would run xcrun twice, which isn't so good, but perhaps Apple could make use of this behavior in the shim, so it works with --target...

[calebzulawski]

Corrected docs and updated upstream