You can use "Caleb Zulawski <caleb.zulawski@gmail.com>", thanks!

Thanks! Made that small change. By the way, I don't have commit access, if you or @nikic could commit it for me.

Thanks for the great review! I haven't touched codegen before, this helped quite a bit.

Corrected docs and updated upstream

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.

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

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.

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.

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.

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

@jryans thanks.

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.

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

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.

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

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

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

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

Update user manual

Update diff for upstream changes

Update comment

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

Move SDK detection to its own test.

Try to fix windows test

Fix test using RUN in CHECK prefix

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

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

Change tests to specifically remove SDKROOT environment variable.

In D136315#3870829, @ldionne wrote:

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

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

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

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

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.

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

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

In terms of who calls it and might error, it looks like it's all used in IntelPTMultiCoreTrace.cpp

I'm not exactly sure what llvm_unreachable does, but I don't believe it will return 0 (maybe it aborts?)