This is an archive of the discontinued LLVM Phabricator instance.

[driver][darwin] Take the OS version specified in "-target" as the target OS instead of inferring it from SDK / environment
ClosedPublic

Authored by arphaman on Dec 7 2017, 6:58 PM.

Diff Detail

Repository
rC Clang

Event Timeline

arphaman created this revision.Dec 7 2017, 6:58 PM
dexonsmith requested changes to this revision.Dec 7 2017, 7:31 PM
dexonsmith added inline comments.
lib/Driver/ToolChains/Darwin.cpp
1366–1367

This logic seems to be duplicated from the -target handling below. Perhaps this would be simpler if -target were processed first.

I suggest the following refactoring in an NFC prep patch:

  • First, check for a -target option. If it exists, extract all the bits.
  • Then, handle the logic for -arch, -m*-version-min, and -isysroot (matching the current semantics, which I think are "override -target").

In a follow-up patch, change -isysroot/SDKROOT not to override a -target-specified environment (the semantic change from this patch). After the refactoring, I suspect this will be trivial.

We should also warn/error when -arch disagrees with -target, and likely the same for -m*-version-min. I suspect these follow-ups will also be trivial.

1405

I'd spell this "invalid OS".

This revision now requires changes to proceed.Dec 7 2017, 7:31 PM
arphaman updated this revision to Diff 126243.Dec 8 2017, 4:54 PM
arphaman edited edge metadata.

I rewrote the patch on top of https://reviews.llvm.org/D41035 as suggested by Duncan.

I also changed some of the semantics:

  • If -target is used with Darwin OS, then the OS will be determined using the old semantics.
  • If -target specifies a concrete OS (even without version), that OS is always used, no matter what other options/environment variables are given. The driver will warn about superfluous -m<os>-version-min and environment variables in this case.
bob.wilson requested changes to this revision.Dec 9 2017, 9:26 PM
bob.wilson added inline comments.
lib/Driver/ToolChains/Darwin.cpp
1519–1524

I don't think there should be a warning in this case. It is common (at least within Apple) to set the environment variable as a default but then override it for some cases. Warning would be really annoying, and for anyone using -Werror it will break their builds.

This revision now requires changes to proceed.Dec 9 2017, 9:26 PM
arphaman updated this revision to Diff 126388.Dec 11 2017, 9:43 AM
arphaman edited edge metadata.
arphaman marked an inline comment as done.

Don't warn about the redundant environment variable

This revision was not accepted when it landed; it landed in state Needs Review.Dec 19 2017, 11:05 AM
This revision was automatically updated to reflect the committed changes.