This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by calebzulawski on Oct 19 2022, 9:45 PM.

Diff Detail

Event Timeline

calebzulawski created this revision.Oct 19 2022, 9:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 19 2022, 9:45 PM
calebzulawski requested review of this revision.Oct 19 2022, 9:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 19 2022, 9:45 PM
calebzulawski retitled this revision from Diff 2 to [clang][Darwin] Try to guess the SDK root with xcrun when unspecified.Oct 19 2022, 9:46 PM
calebzulawski edited the summary of this revision. (Show Details)
calebzulawski added a reviewer: arphaman.
calebzulawski added subscribers: thakis, carlocab, hans.

Thanks for picking this up! I looked at my local changes and I had started modifying inferDeploymentTargetFromSDK. I had left the comment:

/// TODO: We should only infer it if the SDK was provided with SDKROOT or -isysroot.
///       If we inferred the SDK with xcrun, it doesn't make sense to infer the
///       deployment target because <REASONS>

I don't remember what I was thinking at the time, but I encourage you to pause for a minute and think about it. I can't think of the reason anymore. If you cant' either, maybe just disregard my comment entirely.

clang/test/Driver/darwin-sdkroot.c
43–44 ↗(On Diff #469111)

You should test with multiple target triples if that's possible.

That's a good point, I was a little suspicious about that function.

I'm not sure it makes sense to infer the target at all from the SDK (e.g. macOS could be x86_64 or arm64) rather than infer the SDK from the target. Rust, for example, disregards SYSROOT and runs xcrun if it doesn't match the target triple. I wonder if that behavior is cemented enough that it shouldn't change, though. At a minimum, you're right that we should skip inferring the target in the case where the sysroot isn't specified at all.

If clang -target arm64-apple-ios -isysroot path/to/MacOSX.sdk changes the target to x86_64-apple-macos to match the SDK, that would be very confusing behavior :) I'll have to investigate that function some more.

carlocab added inline comments.Oct 20 2022, 1:19 PM
clang/lib/Driver/ToolChains/Darwin.cpp
2136

Since we’re already doing a bunch of triple checks above, it probably doesn’t hurt to check that we really are targeting macOS here.

This may not help with any real use cases, but, in the past, some tests get broken when LLVM is configured with -DDEFAULT_SYSROOT, and this change is likely to break those tests in the same way.

Changed to not invoke xcrun if the SDK name can't be determined.

calebzulawski marked an inline comment as done.Oct 21 2022, 8:57 PM
calebzulawski marked an inline comment as done.
calebzulawski added a comment.EditedOct 21 2022, 9:02 PM

Thanks for picking this up! I looked at my local changes and I had started modifying inferDeploymentTargetFromSDK. I had left the comment:

/// TODO: We should only infer it if the SDK was provided with SDKROOT or -isysroot.
///       If we inferred the SDK with xcrun, it doesn't make sense to infer the
///       deployment target because <REASONS>

I don't remember what I was thinking at the time, but I encourage you to pause for a minute and think about it. I can't think of the reason anymore. If you cant' either, maybe just disregard my comment entirely.

I took another look at this, and I think it makes sense to determine the deployment target regardless. The target platform shouldn't change since the platform is passed to xcrun, but there is still value in determining the default deployment version. It may have mattered in your initial changes, since you weren't passing the target platform to xcrun.

Change tests to specifically remove SDKROOT environment variable.

tschuett added inline comments.Oct 22 2022, 8:15 AM
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.

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

calebzulawski added inline comments.Oct 22 2022, 8:55 AM
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 added inline comments.Oct 22 2022, 9:11 AM
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 added inline comments.Oct 22 2022, 9:17 AM
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.

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

calebzulawski marked an inline comment as done.Oct 22 2022, 3:28 PM
calebzulawski added inline comments.
clang/lib/Driver/ToolChains/Darwin.cpp
2147

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

Fix test using RUN in CHECK prefix

Try to fix windows test

Move SDK detection to its own test.

jryans added a subscriber: jryans.Oct 26 2022, 1:07 PM

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

ldionne accepted this revision.Dec 12 2022, 12:58 PM

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

This revision is now accepted and ready to land.Dec 12 2022, 12:58 PM

Update comment

calebzulawski marked an inline comment as done.Dec 13 2022, 3:37 PM

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

Update diff for upstream changes

hans added a comment.Dec 14 2022, 5:57 AM

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

Update user manual

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

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

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

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

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

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.

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

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.

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)

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.

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

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.

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.

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

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

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 added a comment.EditedJan 19 2023, 4:47 PM

@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

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)

calebzulawski reopened this revision.Jan 19 2023, 6:33 PM

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

This revision is now accepted and ready to land.Jan 19 2023, 6:33 PM

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.

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.

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

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.

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 added a comment.EditedJan 20 2023, 11:12 AM

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.

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 added inline comments.Jan 20 2023, 11:43 AM
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.

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.

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 marked 2 inline comments as done.Jan 20 2023, 4:12 PM

Corrected docs and updated upstream

ldionne resigned from this revision.Sep 8 2023, 8:59 AM
This revision now requires review to proceed.Sep 8 2023, 8:59 AM
Via Web