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

[calebzulawski]

Summary

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

[ldionne]

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.

[calebzulawski]

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.

[calebzulawski]

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]

clang/lib/Driver/ToolChains/Darwin.cpp

2139

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.

[calebzulawski]

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

[calebzulawski]

In D136315#3870829, @ldionne wrote:

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.

[calebzulawski]

Change tests to specifically remove SDKROOT environment variable.

[tschuett]

clang/lib/Driver/ToolChains/Darwin.cpp

2123

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

2123

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

2150

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

2150

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

2150

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

2086–2087

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

1

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

1

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

1

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

clang/docs/UsersManual.rst

 Operating System Features and Limitations
 -----------------------------------------
 
+Apple
+^^^^^
+
+On Apple platforms, the standard headers and libraries are not provided by
+the base system and are instead part of the Xcode SDK application. The location
+of the SDK is determined any of the following ways:
+
+- If passed to Clang, the ``-isysroot`` option specifies the path to the SDK.
+
+- If the sysroot isn't provided, the ``SDKROOT`` environment variable is checked.
+  This variable is set by various Xcode tools.
+
+- Otherwise, Clang uses Xcode's ``xcrun`` tool to find the SDK.
+
 Windows
 ^^^^^^^
 

clang/include/clang/Driver/Options.td

 def no_integrated_cpp : Flag<["-", "--"], "no-integrated-cpp">, Flags<[NoXarchOption]>;
 def no_pedantic : Flag<["-", "--"], "no-pedantic">, Group<pedantic_Group>;
 def no__dead__strip__inits__and__terms : Flag<["-"], "no_dead_strip_inits_and_terms">;
+def no_detect_xcode : Flag<["--"], "no-detect-xcode">,
+  Flags<[NoXarchOption]>, HelpText<"Disable detection of the Xcode SDK">;
 def nobuiltininc : Flag<["-"], "nobuiltininc">, Flags<[CC1Option, CoreOption]>,
   HelpText<"Disable builtin #include directories">,
   MarshallingInfoNegativeFlag<HeaderSearchOpts<"UseBuiltinIncludes">>;

clang/lib/Driver/ToolChains/Darwin.cpp

 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/Options.h"
 #include "clang/Driver/SanitizerArgs.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/ProfileData/InstrProf.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/Program.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/TargetParser.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include <cstdlib> // ::getenv
+#include <memory>  // std::unique_ptr
 
 using namespace clang::driver;
 using namespace clang::driver::tools;

 void Darwin::AddDeploymentTarget(DerivedArgList &Args) const {
   const OptTable &Opts = getDriver().getOpts();
 
-  // Support allowing the SDKROOT environment variable used by xcrun and other
-  // Xcode tools to define the default sysroot, by making it the default for
-  // isysroot.
+  // On Apple platforms, standard headers and libraries are not provided with
+  // the base system (e.g. in /usr/{include,lib}). Instead, they are provided
+  // in various SDKs for the different Apple platforms. Clang needs to know
+  // where that SDK lives, and there are a couple ways this can be achieved:
+  //
+  // (1) If `-isysroot <path-to-SDK>` is passed explicitly, use that.
   if (const Arg *A = Args.getLastArg(options::OPT_isysroot)) {
     // Warn if the path does not exist.
     if (!getVFS().exists(A->getValue()))
       getDriver().Diag(clang::diag::warn_missing_sysroot) << A->getValue();
-  } else {
-    if (char *env = ::getenv("SDKROOT")) {
-      // We only use this value as the default if it is an absolute path,
-      // exists, and it is not the root path.
-      if (llvm::sys::path::is_absolute(env) && getVFS().exists(env) &&
-          StringRef(env) != "/") {
-        Args.append(Args.MakeSeparateArg(
-            nullptr, Opts.getOption(options::OPT_isysroot), env));
+  }
+
+  // (2) If the SDKROOT environment variable is defined and points to a valid
+  //     path, use that. $SDKROOT is set by `xcrun` and other Xcode tools, so
+  //     running `xcrun clang` will work by going through this path.
+  else if (char *env = ::getenv("SDKROOT")) {
+    // We only use this value as the default if it is an absolute path,
+    // exists, and it is not the root path.
+    if (llvm::sys::path::is_absolute(env) && getVFS().exists(env) &&
+        StringRef(env) != "/") {
+      Args.append(Args.MakeSeparateArg(
+          nullptr, Opts.getOption(options::OPT_isysroot), env));
+    }
+  }
+
+  // (3) Otherwise, we try to guess the path of the default SDK by running
+  //     `xcrun --show-sdk-path`. This won't always be correct, but if the
+  //      user wants to use the non-default SDK, they should specify it
+  //      explicitly with methods (1) or (2) above.
+  else if (!Args.hasArg(options::OPT_no_detect_xcode)) {
+    llvm::SmallString<64> OutputFile;
+    llvm::sys::fs::createTemporaryFile("print-sdk-path", "" /* No Suffix */,
+                                       OutputFile);
+    llvm::FileRemover OutputRemover(OutputFile.c_str());
+    std::optional<llvm::StringRef> Redirects[] = {{""}, OutputFile.str(), {""}};
+
+    Optional<StringRef> SDKName = std::nullopt;
+    switch (getTriple().getOS()) {
+    case llvm::Triple::OSType::WatchOS:
+      if (getTriple().isSimulatorEnvironment())
+        SDKName = "watchsimulator";
+      else
+        SDKName = "watchos";
+      break;
+    case llvm::Triple::OSType::TvOS:
+      if (getTriple().isSimulatorEnvironment())
+        SDKName = "appletvsimulator";
+      else
+        SDKName = "appletvos";
+      break;
+    case llvm::Triple::OSType::IOS:
+      if (getTriple().isSimulatorEnvironment())
+        SDKName = "iphonesimulator";
+      else
+        SDKName = "iphoneos";
+      break;
+    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");
+    }
+
+    if (SDKName) {
+      int Result = llvm::sys::ExecuteAndWait(
+          "/usr/bin/xcrun",
+          {"/usr/bin/xcrun", "--sdk", *SDKName, "--show-sdk-path"},
+          /* Inherit environment from parent process */ std::nullopt, Redirects,
+          /* SecondsToWait */ 0, /*MemoryLimit*/ 0);
+      if (Result == 0) {
+        llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> OutputBuf =
+            llvm::MemoryBuffer::getFile(OutputFile.c_str(), /* IsText */ true);
+        if (OutputBuf) {
+          llvm::StringRef SdkPath = (*OutputBuf)->getBuffer().trim();
+          if (getVFS().exists(SdkPath)) {
+            Args.append(Args.MakeSeparateArg(
+                nullptr, Opts.getOption(options::OPT_isysroot), SdkPath));
+          }
+        }
       }
     }
   }

clang/test/Driver/darwin-ld-platform-version-macos.c

 // 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: %clang --no-detect-xcode -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 \

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

+// REQUIRES: system-darwin
+
+// 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-macos -c %s -### 2> %t.log
+// RUN: FileCheck --check-prefix=CHECK-XC < %t.log %s
+//
+// CHECK-XC: clang
+// CHECK-XC: "-cc1"
+// CHECK-XC: "-isysroot" "{{.*MacOSX[0-9\.]*\.sdk}}"
+
+// Check once again that we default to running `xcrun`, this time with another target.
+//
+// RUN: env -u SDKROOT %clang -target arm64-apple-ios -c %s -### 2> %t.log
+// RUN: FileCheck --check-prefix=CHECK-XC-IOS < %t.log %s
+//
+// CHECK-XC-IOS: clang
+// CHECK-XC-IOS: "-cc1"
+// CHECK-XC-IOS: "-isysroot" "{{.*iPhoneOS[0-9\.]*\.sdk}}"