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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski updated this revision to Diff 506434.
calebzulawski added a comment.

Corrected docs and updated upstream


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/test/Driver/darwin-sdk-detect.c
  clang/test/Driver/lit.local.cfg

Index: clang/test/Driver/lit.local.cfg
===
--- clang/test/Driver/lit.local.cfg
+++ clang/test/Driver/lit.local.cfg
@@ -1,4 +1,5 @@
 from lit.llvm import llvm_config
+import subprocess
 
 config.suffixes = ['.c', '.cpp', '.cppm', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
'.cu', '.rs', '.cl', '.clcpp', '.hip', '.hipi', '.hlsl']
@@ -24,3 +25,9 @@
 
 if config.ppc_linux_default_ieeelongdouble:
   config.available_features.add('ppc_linux_default_ieeelongdouble')
+
+if os.path.exists('/usr/bin/xcrun') and sys.platform == 'darwin':
+if subprocess.run(['/usr/bin/xcrun', '--sdk', 'macosx', '--show-sdk-path']).returncode == 0:
+config.available_features.add('macos-sdk')
+if subprocess.run(['/usr/bin/xcrun', '--sdk', 'iphoneos', '--show-sdk-path']).returncode == 0:
+config.available_features.add('ios-sdk')
Index: clang/test/Driver/darwin-sdk-detect.c
===
--- /dev/null
+++ clang/test/Driver/darwin-sdk-detect.c
@@ -0,0 +1,20 @@
+// REQUIRES: system-darwin, ios-sdk, macos-sdk
+
+// Check that we default to running `xcrun --show-sdk-path` if there is no
+// SDKROOT defined in the environment.
+//
+// RUN: env -u SDKROOT %clang --infer-sdkroot-from-xcrun -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 --infer-sdkroot-from-xcrun -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}}"
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -18,15 +18,22 @@
 #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/Threading.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/TargetParser/TargetParser.h"
 #include  // ::getenv
+#include   // std::unique_ptr
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
@@ -2098,21 +2105,89 @@
 void Darwin::AddDeploymentTarget(DerivedArgList ) const {
   const OptTable  = 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 ` 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 

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski updated this revision to Diff 490999.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/test/Driver/darwin-sdk-detect.c
  clang/test/Driver/lit.local.cfg

Index: clang/test/Driver/lit.local.cfg
===
--- clang/test/Driver/lit.local.cfg
+++ clang/test/Driver/lit.local.cfg
@@ -1,4 +1,5 @@
 from lit.llvm import llvm_config
+import subprocess
 
 config.suffixes = ['.c', '.cpp', '.cppm', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
'.cu', '.rs', '.cl', '.clcpp', '.hip', '.hipi', '.hlsl']
@@ -24,3 +25,9 @@
 
 if config.ppc_linux_default_ieeelongdouble:
   config.available_features.add('ppc_linux_default_ieeelongdouble')
+
+if os.path.exists('/usr/bin/xcrun') and sys.platform == 'darwin':
+if subprocess.run(['/usr/bin/xcrun', '--sdk', 'macosx', '--show-sdk-path']).returncode == 0:
+config.available_features.add('macos-sdk')
+if subprocess.run(['/usr/bin/xcrun', '--sdk', 'iphoneos', '--show-sdk-path']).returncode == 0:
+config.available_features.add('ios-sdk')
Index: clang/test/Driver/darwin-sdk-detect.c
===
--- /dev/null
+++ clang/test/Driver/darwin-sdk-detect.c
@@ -0,0 +1,20 @@
+// REQUIRES: system-darwin, ios-sdk, macos-sdk
+
+// Check that we default to running `xcrun --show-sdk-path` if there is no
+// SDKROOT defined in the environment.
+//
+// RUN: env -u SDKROOT %clang --infer-sdkroot-from-xcrun -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 --infer-sdkroot-from-xcrun -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}}"
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -18,15 +18,22 @@
 #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  // ::getenv
+#include   // std::unique_ptr
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
@@ -2078,21 +2085,89 @@
 void Darwin::AddDeploymentTarget(DerivedArgList ) const {
   const OptTable  = 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 ` 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) 

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

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

In D136315#4069849 , @calebzulawski 
wrote:

> In my situation, at least, I am the vendor of the toolchain and my 
> configuration file contains `--sysroot=/../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 `` 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?




Comment at: clang/test/Driver/darwin-sdk-detect.c:1
+// REQUIRES: system-darwin
+

calebzulawski wrote:
> steven_wu wrote:
> > 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.
> 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.
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.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski added inline comments.



Comment at: clang/test/Driver/darwin-sdk-detect.c:1
+// REQUIRES: system-darwin
+

steven_wu wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski added a comment.

In my situation, at least, I am the vendor of the toolchain and my 
configuration file contains `--sysroot=/../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 `` 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

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




Comment at: clang/test/Driver/darwin-sdk-detect.c:1
+// REQUIRES: system-darwin
+

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski updated this revision to Diff 490698.
calebzulawski added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/test/Driver/darwin-ld-platform-version-macos.c
  clang/test/Driver/darwin-sdk-detect.c

Index: clang/test/Driver/darwin-sdk-detect.c
===
--- /dev/null
+++ clang/test/Driver/darwin-sdk-detect.c
@@ -0,0 +1,20 @@
+// 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}}"
Index: clang/test/Driver/darwin-ld-platform-version-macos.c
===
--- clang/test/Driver/darwin-ld-platform-version-macos.c
+++ clang/test/Driver/darwin-ld-platform-version-macos.c
@@ -41,7 +41,7 @@
 // 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 \
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -18,15 +18,22 @@
 #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  // ::getenv
+#include   // std::unique_ptr
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
@@ -2078,21 +2085,90 @@
 void Darwin::AddDeploymentTarget(DerivedArgList ) const {
   const OptTable  = 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 ` 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 = 

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski reopened this revision.
calebzulawski added a comment.
This revision is now accepted and ready to land.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Reverted in a5f446bc4bb1ac78d6852cc8e251a1229899b783 for now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski added a comment.

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

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[J. Ryan Stinnett via Phabricator via cfe-commits]

jryans added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Daniel Thornburgh via Phabricator via cfe-commits]

mysterymath added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[J. Ryan Stinnett via Phabricator via cfe-commits]

jryans added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[J. Ryan Stinnett via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGecade80d9396: [clang][Darwin] Try to guess the SDK root with 
xcrun when unspecified (authored by calebzulawski, committed by jryans).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

Files:
  clang/docs/UsersManual.rst
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/test/Driver/darwin-sdk-detect.c

Index: clang/test/Driver/darwin-sdk-detect.c
===
--- /dev/null
+++ clang/test/Driver/darwin-sdk-detect.c
@@ -0,0 +1,20 @@
+// 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-darwin -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}}"
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -18,15 +18,22 @@
 #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  // ::getenv
+#include   // std::unique_ptr
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
@@ -2078,21 +2085,89 @@
 void Darwin::AddDeploymentTarget(DerivedArgList ) const {
   const OptTable  = 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 ` 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 {
+llvm::SmallString<64> OutputFile;
+llvm::sys::fs::createTemporaryFile("print-sdk-path", "" /* No Suffix */,
+   OutputFile);
+llvm::FileRemover OutputRemover(OutputFile.c_str());
+std::optional Redirects[] = {{""}, OutputFile.str(), {""}};
+
+Optional SDKName = std::nullopt;
+switch (getTriple().getOS()) {
+case 

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[J. Ryan Stinnett via Phabricator via cfe-commits]

jryans added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski updated this revision to Diff 483697.
calebzulawski added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

Files:
  clang/docs/UsersManual.rst
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/test/Driver/darwin-sdk-detect.c

Index: clang/test/Driver/darwin-sdk-detect.c
===
--- /dev/null
+++ clang/test/Driver/darwin-sdk-detect.c
@@ -0,0 +1,20 @@
+// 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-darwin -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}}"
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -18,15 +18,22 @@
 #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  // ::getenv
+#include   // std::unique_ptr
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
@@ -2071,21 +2078,89 @@
 void Darwin::AddDeploymentTarget(DerivedArgList ) const {
   const OptTable  = 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 ` 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 {
+llvm::SmallString<64> OutputFile;
+llvm::sys::fs::createTemporaryFile("print-sdk-path", "" /* No Suffix */,
+   OutputFile);
+llvm::FileRemover OutputRemover(OutputFile.c_str());
+std::optional Redirects[] = {{""}, OutputFile.str(), {""}};
+
+Optional SDKName = std::nullopt;
+switch (getTriple().getOS()) {
+case llvm::Triple::OSType::WatchOS:
+  if (getTriple().isSimulatorEnvironment())
+   

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski updated this revision to Diff 483416.
calebzulawski added a comment.

Update user manual


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

Files:
  clang/docs/UsersManual.rst
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/test/Driver/darwin-sdk-detect.c

Index: clang/test/Driver/darwin-sdk-detect.c
===
--- /dev/null
+++ clang/test/Driver/darwin-sdk-detect.c
@@ -0,0 +1,20 @@
+// 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-darwin -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}}"
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -18,15 +18,22 @@
 #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  // ::getenv
+#include   // std::unique_ptr
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
@@ -2071,21 +2078,89 @@
 void Darwin::AddDeploymentTarget(DerivedArgList ) const {
   const OptTable  = 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 ` 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 {
+llvm::SmallString<64> OutputFile;
+llvm::sys::fs::createTemporaryFile("print-sdk-path", "" /* No Suffix */,
+   OutputFile);
+llvm::FileRemover OutputRemover(OutputFile.c_str());
+std::optional Redirects[] = {{""}, OutputFile.str(), {""}};
+
+Optional SDKName = std::nullopt;
+switch (getTriple().getOS()) {
+case llvm::Triple::OSType::WatchOS:
+  if (getTriple().isSimulatorEnvironment())
+SDKName = "watchsimulator";
+  else
+

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

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski updated this revision to Diff 482697.
calebzulawski added a comment.

Update diff for upstream changes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

Files:
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/test/Driver/darwin-sdk-detect.c

Index: clang/test/Driver/darwin-sdk-detect.c
===
--- /dev/null
+++ clang/test/Driver/darwin-sdk-detect.c
@@ -0,0 +1,20 @@
+// 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-darwin -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}}"
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -18,15 +18,22 @@
 #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  // ::getenv
+#include   // std::unique_ptr
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
@@ -2071,21 +2078,89 @@
 void Darwin::AddDeploymentTarget(DerivedArgList ) const {
   const OptTable  = 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 ` 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 {
+llvm::SmallString<64> OutputFile;
+llvm::sys::fs::createTemporaryFile("print-sdk-path", "" /* No Suffix */,
+   OutputFile);
+llvm::FileRemover OutputRemover(OutputFile.c_str());
+std::optional Redirects[] = {{""}, OutputFile.str(), {""}};
+
+Optional SDKName = std::nullopt;
+switch (getTriple().getOS()) {
+case llvm::Triple::OSType::WatchOS:
+  if (getTriple().isSimulatorEnvironment())
+SDKName = "watchsimulator";
+  else
+SDKName = "watchos";

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

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski updated this revision to Diff 482650.
calebzulawski added a comment.

Update comment


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

Files:
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/test/Driver/darwin-sdk-detect.c

Index: clang/test/Driver/darwin-sdk-detect.c
===
--- /dev/null
+++ clang/test/Driver/darwin-sdk-detect.c
@@ -0,0 +1,20 @@
+// 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-darwin -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}}"
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -18,15 +18,22 @@
 #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  // ::getenv
+#include   // std::unique_ptr
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
@@ -2071,21 +2078,90 @@
 void Darwin::AddDeploymentTarget(DerivedArgList ) const {
   const OptTable  = 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 ` 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 {
+llvm::SmallString<64> OutputFile;
+llvm::sys::fs::createTemporaryFile("print-sdk-path", "" /* No Suffix */,
+   OutputFile);
+llvm::FileRemover OutputRemover(OutputFile.c_str());
+llvm::Optional Redirects[] = {
+{""}, OutputFile.str(), {""}};
+
+Optional SDKName = None;
+switch (getTriple().getOS()) {
+case llvm::Triple::OSType::WatchOS:
+  if (getTriple().isSimulatorEnvironment())
+SDKName = "watchsimulator";
+  else
+SDKName = "watchos";
+  break;
+ 

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

[Louis Dionne via Phabricator via cfe-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

This LGTM.




Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2084-2089
+  // 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 ` is passed explicitly, use that.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski updated this revision to Diff 469987.
calebzulawski added a comment.

Move SDK detection to its own test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

Files:
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/test/Driver/darwin-sdk-detect.c

Index: clang/test/Driver/darwin-sdk-detect.c
===
--- /dev/null
+++ clang/test/Driver/darwin-sdk-detect.c
@@ -0,0 +1,20 @@
+// 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-darwin -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}}"
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -18,15 +18,22 @@
 #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  // ::getenv
+#include   // std::unique_ptr
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
@@ -2074,21 +2081,90 @@
 void Darwin::AddDeploymentTarget(DerivedArgList ) const {
   const OptTable  = 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 ` 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 {
+llvm::SmallString<64> OutputFile;
+llvm::sys::fs::createTemporaryFile("print-sdk-path", "" /* No Suffix */,
+   OutputFile);
+llvm::FileRemover OutputRemover(OutputFile.c_str());
+llvm::Optional Redirects[] = {
+{""}, OutputFile.str(), {""}};
+
+Optional SDKName = None;
+switch (getTriple().getOS()) {
+case llvm::Triple::OSType::WatchOS:
+  if (getTriple().isSimulatorEnvironment())
+SDKName = "watchsimulator";
+  else
+SDKName = "watchos";
+  break;
+

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski updated this revision to Diff 469953.
calebzulawski added a comment.

Try to fix windows test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

Files:
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/test/Driver/darwin-sdkroot.c

Index: clang/test/Driver/darwin-sdkroot.c
===
--- clang/test/Driver/darwin-sdkroot.c
+++ clang/test/Driver/darwin-sdkroot.c
@@ -1,4 +1,5 @@
-// 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
@@ -39,6 +40,25 @@
 //
 // 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 -u SDKROOT %clang -target x86_64-apple-darwin -c %s -### 2> %t.log
+// RUN: if [[ -f "/usr/bin/xcrun" ]]; then FileCheck --check-prefix=CHECK-XC < %t.log %s; fi
+//
+// 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: if [[ -f "/usr/bin/xcrun" ]]; then FileCheck --check-prefix=CHECK-XC-IOS < %t.log %s; fi
+//
+// CHECK-XC-IOS: clang
+// CHECK-XC-IOS: "-cc1"
+// CHECK-XC-IOS: "-isysroot" "{{.*iPhoneOS[0-9\.]*\.sdk}}"
+
 // Check if clang set the correct deployment target from -sysroot
 //
 // RUN: rm -rf %t/SDKs/iPhoneOS8.0.0.sdk
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -18,15 +18,22 @@
 #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  // ::getenv
+#include   // std::unique_ptr
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
@@ -2074,21 +2081,90 @@
 void Darwin::AddDeploymentTarget(DerivedArgList ) const {
   const OptTable  = 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 ` 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 {
+llvm::SmallString<64> 

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski updated this revision to Diff 469948.
calebzulawski added a comment.

Fix test using `RUN` in `CHECK` prefix


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

Files:
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/test/Driver/darwin-sdkroot.c

Index: clang/test/Driver/darwin-sdkroot.c
===
--- clang/test/Driver/darwin-sdkroot.c
+++ clang/test/Driver/darwin-sdkroot.c
@@ -1,4 +1,5 @@
-// 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
@@ -39,6 +40,25 @@
 //
 // 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 -u SDKROOT %clang -target x86_64-apple-darwin -c %s -### 2> %t.log
+// RUN: if test -f /usr/bin/xcrun; then FileCheck --check-prefix=CHECK-XC < %t.log %s; fi
+//
+// 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: if test -f /usr/bin/xcrun; then FileCheck --check-prefix=CHECK-XC-IOS < %t.log %s; fi
+//
+// CHECK-XC-IOS: clang
+// CHECK-XC-IOS: "-cc1"
+// CHECK-XC-IOS: "-isysroot" "{{.*iPhoneOS[0-9\.]*\.sdk}}"
+
 // Check if clang set the correct deployment target from -sysroot
 //
 // RUN: rm -rf %t/SDKs/iPhoneOS8.0.0.sdk
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -18,15 +18,22 @@
 #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  // ::getenv
+#include   // std::unique_ptr
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
@@ -2074,21 +2081,90 @@
 void Darwin::AddDeploymentTarget(DerivedArgList ) const {
   const OptTable  = 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 ` 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 {
+

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski marked an inline comment as done.
calebzulawski added inline comments.



Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2147
+default:
+  // We shouldn't get here, unless the target OS doesn't have an SDK.
+  break;

calebzulawski wrote:
> carlocab wrote:
> > `llvm_unreachable`? Or `assert`, at least.
> 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.
Realizing this is never hit for non-darwin targets anyway, added the 
`llvm_unreachable`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski updated this revision to Diff 469940.
calebzulawski added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

Files:
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/test/Driver/darwin-sdkroot.c

Index: clang/test/Driver/darwin-sdkroot.c
===
--- clang/test/Driver/darwin-sdkroot.c
+++ clang/test/Driver/darwin-sdkroot.c
@@ -1,4 +1,5 @@
-// 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
@@ -39,6 +40,25 @@
 //
 // 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 -u SDKROOT %clang -target x86_64-apple-darwin -c %s -### 2> %t.log
+// RUN: if test -f /usr/bin/xcrun; then FileCheck --check-prefix=CHECK-XCRUN < %t.log %s; fi
+//
+// CHECK-XCRUN: clang
+// CHECK-XCRUN: "-cc1"
+// CHECK-XCRUN: "-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: if test -f /usr/bin/xcrun; then FileCheck --check-prefix=CHECK-XCRUN-IOS < %t.log %s; fi
+//
+// CHECK-XCRUN-IOS: clang
+// CHECK-XCRUN-IOS: "-cc1"
+// CHECK-XCRUN-IOS: "-isysroot" "{{.*iPhoneOS[0-9\.]*\.sdk}}"
+
 // Check if clang set the correct deployment target from -sysroot
 //
 // RUN: rm -rf %t/SDKs/iPhoneOS8.0.0.sdk
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -18,15 +18,22 @@
 #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  // ::getenv
+#include   // std::unique_ptr
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
@@ -2074,21 +2081,90 @@
 void Darwin::AddDeploymentTarget(DerivedArgList ) const {
   const OptTable  = 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 ` 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
+  //  

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski added inline comments.



Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2147
+default:
+  // We shouldn't get here, unless the target OS doesn't have an SDK.
+  break;

carlocab wrote:
> `llvm_unreachable`? Or `assert`, at least.
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.



Comment at: clang/test/Driver/darwin-sdkroot.c:1
-// Check that SDKROOT is used to define the default for -isysroot on Darwin.
+// REQUIRES: system-darwin
+

carlocab wrote:
> 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?
That would work, but I'm not sure how to exclude a particular test if it's not 
found.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Carlo Cabrera via Phabricator via cfe-commits]

carlocab added inline comments.



Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2147
+default:
+  // We shouldn't get here, unless the target OS doesn't have an SDK.
+  break;

`llvm_unreachable`? Or `assert`, at least.



Comment at: clang/test/Driver/darwin-sdkroot.c:1
-// Check that SDKROOT is used to define the default for -isysroot on Darwin.
+// REQUIRES: system-darwin
+

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski added inline comments.



Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2120
+Optional SDKName = None;
+if (getTriple().isWatchOS()) {
+  if (getTriple().isSimulatorEnvironment())

tschuett wrote:
> Will there be an enum over the Apple variants to make this less error prone 
> and future proof? I want a switch statement.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski updated this revision to Diff 469909.
calebzulawski added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

Files:
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/test/Driver/darwin-sdkroot.c

Index: clang/test/Driver/darwin-sdkroot.c
===
--- clang/test/Driver/darwin-sdkroot.c
+++ clang/test/Driver/darwin-sdkroot.c
@@ -1,4 +1,7 @@
-// Check that SDKROOT is used to define the default for -isysroot on Darwin.
+// REQUIRES: system-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
@@ -39,6 +42,25 @@
 //
 // 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 -u SDKROOT %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" "{{.*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-XCRUN-IOS < %t.log %s
+//
+// CHECK-XCRUN-IOS: clang
+// CHECK-XCRUN-IOS: "-cc1"
+// CHECK-XCRUN-IOS: "-isysroot" "{{.*iPhoneOS[0-9\.]*\.sdk}}"
+
 // Check if clang set the correct deployment target from -sysroot
 //
 // RUN: rm -rf %t/SDKs/iPhoneOS8.0.0.sdk
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -18,15 +18,22 @@
 #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  // ::getenv
+#include   // std::unique_ptr
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
@@ -2074,21 +2081,89 @@
 void Darwin::AddDeploymentTarget(DerivedArgList ) const {
   const OptTable  = 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 ` 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 {
+

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

[Thorsten via Phabricator via cfe-commits]

tschuett added inline comments.



Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2120
+Optional SDKName = None;
+if (getTriple().isWatchOS()) {
+  if (getTriple().isSimulatorEnvironment())

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski updated this revision to Diff 469901.
calebzulawski added a comment.

Change tests to specifically remove `SDKROOT` environment variable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

Files:
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/test/Driver/darwin-sdkroot.c

Index: clang/test/Driver/darwin-sdkroot.c
===
--- clang/test/Driver/darwin-sdkroot.c
+++ clang/test/Driver/darwin-sdkroot.c
@@ -1,4 +1,5 @@
-// 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
@@ -39,6 +40,25 @@
 //
 // 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 -u SDKROOT %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" "{{.*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-XCRUN-IOS < %t.log %s
+//
+// CHECK-XCRUN-IOS: clang
+// CHECK-XCRUN-IOS: "-cc1"
+// CHECK-XCRUN-IOS: "-isysroot" "{{.*iPhoneOS[0-9\.]*\.sdk}}"
+
 // Check if clang set the correct deployment target from -sysroot
 //
 // RUN: rm -rf %t/SDKs/iPhoneOS8.0.0.sdk
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -18,15 +18,22 @@
 #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  // ::getenv
+#include   // std::unique_ptr
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
@@ -2074,21 +2081,77 @@
 void Darwin::AddDeploymentTarget(DerivedArgList ) const {
   const OptTable  = 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 ` 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 {
+llvm::SmallString<64> 

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski updated this revision to Diff 469859.
calebzulawski added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

Files:
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/test/Driver/darwin-sdkroot.c

Index: clang/test/Driver/darwin-sdkroot.c
===
--- clang/test/Driver/darwin-sdkroot.c
+++ clang/test/Driver/darwin-sdkroot.c
@@ -1,4 +1,5 @@
-// 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
@@ -39,6 +40,25 @@
 //
 // 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" "{{.*MacOSX[0-9\.]*\.sdk}}"
+
+// Check once again that we default to running `xcrun`, this time with another target.
+//
+// RUN: env -i %clang -target arm64-apple-ios -c %s -### 2> %t.log
+// RUN: FileCheck --check-prefix=CHECK-XCRUN-IOS < %t.log %s
+//
+// CHECK-XCRUN-IOS: clang
+// CHECK-XCRUN-IOS: "-cc1"
+// CHECK-XCRUN-IOS: "-isysroot" "{{.*iPhoneOS[0-9\.]*\.sdk}}"
+
 // Check if clang set the correct deployment target from -sysroot
 //
 // RUN: rm -rf %t/SDKs/iPhoneOS8.0.0.sdk
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -18,15 +18,22 @@
 #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  // ::getenv
+#include   // std::unique_ptr
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
@@ -2074,21 +2081,77 @@
 void Darwin::AddDeploymentTarget(DerivedArgList ) const {
   const OptTable  = 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 ` 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 {
+llvm::SmallString<64> OutputFile;
+

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

[Carlo Cabrera via Phabricator via cfe-commits]

carlocab added inline comments.



Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2136
+} else {
+sdk = "macosx";
+}

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Caleb Zulawski via Phabricator via cfe-commits]

calebzulawski added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

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 

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.




Comment at: clang/test/Driver/darwin-sdkroot.c:43-44
 
+// Check that we default to running `xcrun --show-sdk-path` if there is no
+// SDKROOT defined in the environment.
+//

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136315/new/

https://reviews.llvm.org/D136315

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits