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

[ldionne]

Summary

On Apple platforms, the base system does not contain headers (e.g. in /usr/include).
As a result, one needs to point Clang to those headers manually when running it,
which can be cumbersome. Instead, if the SDK root can't be inferred with the
usual logic (using the SDKROOT env var), try to guess it by running 'xcrun'.

This should provide a default that's sane most of the time when running clang
without impeding the ability for users to select the SDK of their choice with
the usual means.

http://llvm.org/PR45880

[ldionne]

@arphaman This is a WIP patch -- I'm trying to see whether this idea is good or just bad from the start.

To summarize, people have been complaining (e.g. here) that Clang on Darwin doesn't work out of the box, because we don't ship headers in /usr/include. This is an attempt to provide a sane default by running xcrun --show-sdk-path from within Clang when no SDK root can be found otherwise.

I was pretty sure this was a good idea until I implemented it and then noticed the tests failing. Previously, when no SDK can be found (which is the case in most Clang tests since we don't run with xcrun), we would use whatever deployment target Clang defaults to. After this patch, however, we'll often find a SDK even though none is specified, and that will have the effect of setting a different deployment target than the one we expect in those tests. Do you think that is just a sign that this whole idea of trying to infer the SDK using xcrun is broken from the start, or those tests should simply be fixed?

I guess another option would be that if the target is specified, we try to infer the SDK path using xcrun --sdk <sdk-inferred-from-target> --show-sdk-path. That's starting to be complex, but it might solve some of these issues.

[thakis]

This is a nice idea.

One drawback is that if you don't pass -isysroot, things just wouldn't work before but now every clang invocation will call xcrun, so things will work but will be slower than if you passed -isysroot. But the convenience for one-off invocations is probably worth this subtlety.

Tests must be hermetic, so we should have some flag to disable this behavior and expand %clang and %clang_cc1 to pass the disable flag, to make sure tests don't rely on this.

Independent of tests, can we make the driver look at the triple and pass a matching -sdk flag to xcrun?

Also, on my system xcrun prints different paths depending on if I pass -sdk for some reason:

% xcrun -show-sdk-path
/Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk
% xcrun -show-sdk-path -sdk macosx
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk

I have to do -isysroot $(xcrun -show-sdk-path -sdk macosx) for things to work. That's another point in favor of passing an -sdk flag.

[calebzulawski]

I'm interested in this feature--is there any reason this wasn't completed? If not, I might be able to take over implementing this.

[ldionne]

In D109460#3860750, @calebzulawski wrote:

I'm interested in this feature--is there any reason this wasn't completed? If not, I might be able to take over implementing this.

Lack of bandwidth. It would be awesome if someone took this over.

[ldionne]

Abandoning in favor of D136315.

clang/lib/Driver/ToolChains/Darwin.cpp

 #include "clang/Basic/AlignedAllocation.h"
 #include "clang/Basic/ObjCRuntime.h"
 #include "clang/Config/config.h"
 #include "clang/Driver/Compilation.h"
 #include "clang/Driver/Driver.h"
 #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;
 using namespace clang::driver::toolchains;
 using namespace clang;
 using namespace llvm::opt;
 
 static VersionTuple minimumMacCatalystDeploymentTarget() {
Optional<DarwinSDKInfo> parseSDKSettings(llvm::vfs::FileSystem &VFS,
   return *SDKInfoOrErr;
 }
 
 } // namespace
 
 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, C and C++ Standard Library headers are not provided
+  // with the base system. 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")) {
+
+  // (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 {
+    llvm::SmallString<64> OutputFile;
+    llvm::sys::fs::createTemporaryFile("print-sdk-path", "" /* No Suffix */, OutputFile);
+    llvm::FileRemover OutputRemover(OutputFile.c_str());
+    llvm::Optional<llvm::StringRef> Redirects[] = {{""}, OutputFile.str(), {""}};
+    int Result = llvm::sys::ExecuteAndWait(
+        "/usr/bin/xcrun",
+        {"/usr/bin/xcrun", "--show-sdk-path"},
+        /* Inherit environment from parent process */ llvm::None, 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));
+        }
+      }
+    }
   }
 
   // Read the SDKSettings.json file for more information, like the SDK version
   // that we can pass down to the compiler.
   SDKInfo = parseSDKSettings(getVFS(), Args, getDriver());
 
   // The OS and the version can be specified using the -target argument.
   Optional<DarwinPlatform> OSTarget =

clang/test/Driver/darwin-sdkroot.c

-// Check that SDKROOT is used to define the default for -isysroot on Darwin.
+// Check that SDKROOT is used to define the default for -isysroot on Darwin,
+// or that we call `xcrun --show-sdk-path` if SDKROOT is not in the environment.
 //
 // RUN: rm -rf %t.tmpdir
 // RUN: mkdir -p %t.tmpdir
 // RUN: env SDKROOT=%t.tmpdir %clang -target x86_64-apple-darwin10 --sysroot="" \
 // RUN:   -c %s -### 2> %t.log
 // RUN: FileCheck --check-prefix=CHECK-BASIC < %t.log %s
 //
 // CHECK-BASIC: clang

 // This test fails with MSYS or MSYS2 env.exe, since it does not preserve
 // root, expanding / into C:/MINGW/MSYS/1.0 or c:/msys64. To reproduce the
 // problem, run:
 //
 //   env SDKROOT=/ cmd //c echo %SDKROOT%
 //
 // This test do pass using GnuWin32 env.exe.
 
+// Check that we default to running `xcrun --show-sdk-path` if there is no
+// SDKROOT defined in the environment.
+//
+// RUN: env -i %clang -target x86_64-apple-darwin -c %s -### 2> %t.log
+// RUN: FileCheck --check-prefix=CHECK-XCRUN < %t.log %s
+//
+// CHECK-XCRUN: clang
+// CHECK-XCRUN: "-cc1"
+// CHECK-XCRUN: "-isysroot" "TODO"
+
 // Check if clang set the correct deployment target from -sysroot
 //
 // RUN: rm -rf %t/SDKs/iPhoneOS8.0.0.sdk
 // RUN: mkdir -p %t/SDKs/iPhoneOS8.0.0.sdk
 // RUN: env SDKROOT=%t/SDKs/iPhoneOS8.0.0.sdk %clang -fuse-ld= -target arm64-apple-darwin -mlinker-version=400 --sysroot="" %s -### 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-IPHONE %s
 //
 // CHECK-IPHONE: clang