This is an archive of the discontinued LLVM Phabricator instance.

[darwin] parse the SDK settings from SDKSettings.json if it exists and pass in the -target-sdk-version to the compiler and backend
ClosedPublic

Authored by arphaman on Dec 13 2018, 2:15 PM.

Details

Summary

This patch is a follow-up to the LLVM SDK Version metadata support: https://reviews.llvm.org/D55612.

This patch adds support for reading the SDKSettings.json file in the Darwin driver. This file is used by the driver to determine the SDK's version, and it uses that information to pass it down to the compiler using the new -target-sdk-version=. This option is then used to set the appropriate SDK Version module metadata introduced in https://reviews.llvm.org/D55612.

Diff Detail

Repository
rC Clang

Event Timeline

arphaman created this revision.Dec 13 2018, 2:15 PM

See comments inline.

include/clang/Driver/DarwinSDKInfo.h
2

Can this just be in Toolchains/Darwin.h?

37

Isn't parseSDKSettings enough? And it can just return Optional<VersionTuple>?

lib/Driver/ToolChains/Darwin.cpp
2102

We also has this InferredFromSDK when we infer deployment target, which can be used as a fallback method.

The result of parsing JSON should be available to InferredFromSDK in deployment target setting.

Bonus point is to set "-sdk_version" flag passing to ld64.

arphaman marked an inline comment as done.Dec 13 2018, 2:48 PM
arphaman added inline comments.
include/clang/Driver/DarwinSDKInfo.h
2

No, we need to expose it to clients like Swift.

arphaman marked an inline comment as done.Dec 13 2018, 2:50 PM
arphaman added inline comments.
include/clang/Driver/DarwinSDKInfo.h
37

We will support other fields besides VersionTuple in the SDKSettings, so that's why we have a structure.

arphaman updated this revision to Diff 178307.Dec 14 2018, 3:42 PM

Updated to infer deployment target version from SDK versions specified in the JSON file and to allow inferring the SDK version from the SDK path.

arphaman marked 2 inline comments as done.Dec 14 2018, 3:42 PM
arphaman added inline comments.
lib/Driver/ToolChains/Darwin.cpp
2102

Done.
We're not planning to pass it down to the linker.

arphaman updated this revision to Diff 178309.Dec 14 2018, 3:48 PM
arphaman marked an inline comment as done.

Ensure test will pass on non-darwin.

steven_wu accepted this revision.Dec 17 2018, 10:18 AM

Other than a small design choice commented inline, LGTM.

include/clang/Driver/DarwinSDKInfo.h
37

I feel like for this usage, it is better to return Expected<DarwinSDKInfo> with all the fields being Optional?

This revision is now accepted and ready to land.Dec 17 2018, 10:18 AM
arphaman marked an inline comment as done.Dec 17 2018, 10:33 AM
arphaman added inline comments.
include/clang/Driver/DarwinSDKInfo.h
37

Hmm, we want to assume that version exists for future uses. I feel like the current type captures the intention better.

steven_wu added inline comments.Dec 17 2018, 10:38 AM
include/clang/Driver/DarwinSDKInfo.h
37

I think it is fine for current use. We can always change in the future.

This revision was automatically updated to reflect the committed changes.
tra added a subscriber: tra.Jan 30 2019, 10:24 AM

Would that be OK to use target_sdk_version to pass *CUDA* SDK version to the CC1 compilations?
I have upcoming changes that need to know the version to generate correct glue IR for CUDA. The driver currently figures out detected CUDA version in lib/Driver/ToolChains/Cuda.cpp and I could use -target-sdk-version to pass it on to CC1 instances.

On one hand CUDA is a target SDK and the option appears to be accessible exactly where I need it. On the other hand, my use case is not *exactly* the case -target-sdk-version was intended for (i.e. it's not darwin and it has nothing to do with module metadata, though it *may* be eventually useful there even for CUDA).

If using the option for CUDA sounds like too much of a hack, I guess I can add a separate -cuda-sdk-version=, though it would effectively replicate some of this patch.

Opinions?

In D55673#1377404, @tra wrote:

Would that be OK to use target_sdk_version to pass *CUDA* SDK version to the CC1 compilations?
I have upcoming changes that need to know the version to generate correct glue IR for CUDA. The driver currently figures out detected CUDA version in lib/Driver/ToolChains/Cuda.cpp and I could use -target-sdk-version to pass it on to CC1 instances.

On one hand CUDA is a target SDK and the option appears to be accessible exactly where I need it. On the other hand, my use case is not *exactly* the case -target-sdk-version was intended for (i.e. it's not darwin and it has nothing to do with module metadata, though it *may* be eventually useful there even for CUDA).

If using the option for CUDA sounds like too much of a hack, I guess I can add a separate -cuda-sdk-version=, though it would effectively replicate some of this patch.

Opinions?

I would be ok with reusing that option, as long as it's documented that there is a difference in terms of how it can be used.

tra added a comment.Jan 30 2019, 4:52 PM

I would be ok with reusing that option, as long as it's documented that there is a difference in terms of how it can be used.

The patch is in https://reviews.llvm.org/D57487
It does not look like we're formally documenting CC1 options anywhere. I've added some comments next to SDKVersion in TargetOptions.h. PTAL.