[driver][darwin] Don't use -platform_version flag by default

[dmajor]

Summary

(Note, I don't currently have commit access.)

The code in llvmorg-10-init-12188-g25ce33a6e4f is a breaking change for users of older linkers who don't pass a version parameter, which prevents a drop-in clang upgrade. Old tools can't know about what future tools will do, so as a general principle the burden should be new tools to be compatible by default. Also, for comparison, none of the other tests of Version within AddLinkArgs add any new behaviors unless the version is explicitly specified. Therefore, this patch changes the -platform_version behavior from opt-out to opt-in.

[dexonsmith]

Code change looks correct to me. Thanks for the fix!

@arphaman, can you confirm the test changes are reasonable? My instinct would have been, instead of changing all of the 400s to 0s, to just adding a single RUN line somewhere to confirm we don't do the wrong thing for 0.

[steven_wu]

I forgot if there is reason to use the option by default at all time (I did ask that in the previous review but Alex might have given more context offline).

You should definitely add test for this change. The fact that you change all -mlinker-version=400 to -mlinker-version=0 but not change any CHECK lines means the change is definitely not tested :)

[dmajor]

In D74784#1882974, @steven_wu wrote:

I forgot if there is reason to use the option by default at all time (I did ask that in the previous review but Alex might have given more context offline).

I would really like to hear from @arphaman, but they haven't responded to anything since I filed PR44813 nearly two weeks ago. Do you have any means to contact them?

You should definitely add test for this change. The fact that you change all -mlinker-version=400 to -mlinker-version=0 but not change any CHECK lines means the change is definitely not tested :)

Can you elaborate on what you would like to see tested? I thought I _was_ updating the tests to match the code. The -mlinker-version=0 lines check that we now use the old behavior by default, and the existing -mlinker-version=520 lines in e.g. darwin-ld-platform-version-macos.c test that we still use the new behavior when requested.

[dmajor]

@steven_wu, ping, could you clarify about the tests please?

You should definitely add test for this change. The fact that you change all -mlinker-version=400 to -mlinker-version=0 but not change any CHECK lines means the change is definitely not tested :)

Can you elaborate on what you would like to see tested? I thought I _was_ updating the tests to match the code. The -mlinker-version=0 lines check that we now use the old behavior by default, and the existing -mlinker-version=520 lines in e.g. darwin-ld-platform-version-macos.c test that we still use the new behavior when requested.

[steven_wu]

In D74784#1891750, @dmajor wrote:

@steven_wu, ping, could you clarify about the tests please?

You want to have testcase to cover all following 3 cases:

• Default (version = 0): not using -platform_version
• Old version (0 < version < 520): not using -platform_version
• New version (version >= 520): using -platform_version

It is fine to leave most of the tests with -mlinker-version=0, but you want to go into the tests of -platform_version and make sure all situations above are handled correctly.

[dmajor]

Updated the tests: three cases for {default, old, new} linkers in the platform-version tests; left alone the tests not specifically targeting this path.

[steven_wu]

Thanks. LGTM. I will let @arphaman to approve this.

[dexonsmith]

LGTM to me too. With @steven_wu and me on board, I don't think you need to wait for @arphaman. Thanks for the fix!

[dmajor]

Thanks! I'm still working on getting commit access, would one of you be able to submit for me?

[hans]

In D74784#1894484, @dmajor wrote:

Thanks! I'm still working on getting commit access, would one of you be able to submit for me?

I've committed as 5122e828701c88f8d53ee881bc68f3904454d154 and pushed to 10.x as 38ee10d08cb5982bfc02dc4eea9a22743dccc6b6.

clang/lib/Driver/ToolChains/Darwin.cpp

void darwin::Linker::AddLinkArgs(Compilation &C, const ArgList &Args,
   Args.AddAllArgs(CmdArgs, options::OPT_exported__symbols__list);
   Args.AddLastArg(CmdArgs, options::OPT_flat__namespace);
   Args.AddAllArgs(CmdArgs, options::OPT_force__load);
   Args.AddAllArgs(CmdArgs, options::OPT_headerpad__max__install__names);
   Args.AddAllArgs(CmdArgs, options::OPT_image__base);
   Args.AddAllArgs(CmdArgs, options::OPT_init);
 
   // Add the deployment target.
-  if (!Version[0] || Version[0] >= 520)
+  if (Version[0] >= 520)
     MachOTC.addPlatformVersionArgs(Args, CmdArgs);
   else
     MachOTC.addMinVersionArgs(Args, CmdArgs);
 
   Args.AddLastArg(CmdArgs, options::OPT_nomultidefs);
   Args.AddLastArg(CmdArgs, options::OPT_multi__module);
   Args.AddLastArg(CmdArgs, options::OPT_single__module);
   Args.AddAllArgs(CmdArgs, options::OPT_multiply__defined);

clang/test/Driver/darwin-ld-platform-version-ios.c

 // RUN: touch %t.o
 
+// RUN: %clang -target arm64-apple-ios12.3 -isysroot %S/Inputs/iPhoneOS13.0.sdk -mlinker-version=0 -### %t.o 2>&1 \
+// RUN:   | FileCheck --check-prefix=LINKER-OLD %s
+// RUN: %clang -target arm64-apple-ios12.3 -isysroot %S/Inputs/iPhoneOS13.0.sdk -mlinker-version=400 -### %t.o 2>&1 \
+// RUN:   | FileCheck --check-prefix=LINKER-OLD %s
 // RUN: %clang -target arm64-apple-ios12.3 -isysroot %S/Inputs/iPhoneOS13.0.sdk -mlinker-version=520 -### %t.o 2>&1 \
-// RUN:   | FileCheck %s
+// RUN:   | FileCheck --check-prefix=LINKER-NEW  %s
 // RUN: %clang -target x86_64-apple-ios13-simulator -isysroot %S/Inputs/iPhoneOS13.0.sdk -mlinker-version=520 -### %t.o 2>&1 \
 // RUN:   | FileCheck --check-prefix=SIMUL %s
 
-// CHECK: "-platform_version" "ios" "12.3.0" "13.0"
+// LINKER-OLD: "-iphoneos_version_min" "12.3.0"
+// LINKER-NEW: "-platform_version" "ios" "12.3.0" "13.0"
 // SIMUL: "-platform_version" "ios-simulator" "13.0.0" "13.0"

clang/test/Driver/darwin-ld-platform-version-macos.c

 // RUN: touch %t.o
 
 // RUN: %clang -target x86_64-apple-macos10.13 -isysroot %S/Inputs/MacOSX10.14.sdk -mlinker-version=0 -### %t.o 2>&1 \
-// RUN:   | FileCheck %s
+// RUN:   | FileCheck --check-prefix=LINKER-OLD %s
+// RUN: %clang -target x86_64-apple-macos10.13 -isysroot %S/Inputs/MacOSX10.14.sdk -mlinker-version=400 -### %t.o 2>&1 \
+// RUN:   | FileCheck --check-prefix=LINKER-OLD %s
 // RUN: env SDKROOT=%S/Inputs/MacOSX10.14.sdk %clang -target x86_64-apple-macos10.13.0.1 -mlinker-version=520 -### %t.o 2>&1 \
-// RUN:   | FileCheck %s
+// RUN:   | FileCheck --check-prefix=LINKER-NEW %s
 
-// CHECK: "-platform_version" "macos" "10.13.0" "10.14"
+// LINKER-OLD: "-macosx_version_min" "10.13.0"
+// LINKER-NEW: "-platform_version" "macos" "10.13.0" "10.14"
 
 // RUN: %clang -target x86_64-apple-macos10.13  -mlinker-version=520 -### %t.o 2>&1 \
 // RUN:   | FileCheck --check-prefix=NOSDK %s
 // NOSDK: "-platform_version" "macos" "10.13.0" "0.0.0"

clang/test/Driver/darwin-ld-platform-version-tvos.c

 // RUN: touch %t.o
 
+// RUN: %clang -target arm64-apple-tvos12.3 -isysroot %S/Inputs/iPhoneOS13.0.sdk -mlinker-version=0 -### %t.o 2>&1 \
+// RUN:   | FileCheck --check-prefix=LINKER-OLD %s
+// RUN: %clang -target arm64-apple-tvos12.3 -isysroot %S/Inputs/iPhoneOS13.0.sdk -mlinker-version=400 -### %t.o 2>&1 \
+// RUN:   | FileCheck --check-prefix=LINKER-OLD %s
 // RUN: %clang -target arm64-apple-tvos12.3 -isysroot %S/Inputs/iPhoneOS13.0.sdk -mlinker-version=520 -### %t.o 2>&1 \
-// RUN:   | FileCheck %s
+// RUN:   | FileCheck --check-prefix=LINKER-NEW %s
 // RUN: %clang -target x86_64-apple-tvos13-simulator -isysroot %S/Inputs/iPhoneOS13.0.sdk -mlinker-version=520 -### %t.o 2>&1 \
 // RUN:   | FileCheck --check-prefix=SIMUL %s
 
-// CHECK: "-platform_version" "tvos" "12.3.0" "13.0"
+// LINKER-OLD: "-tvos_version_min" "12.3.0"
+// LINKER-NEW: "-platform_version" "tvos" "12.3.0" "13.0"
 // SIMUL: "-platform_version" "tvos-simulator" "13.0.0" "13.0"

clang/test/Driver/darwin-ld-platform-version-watchos.c

 // RUN: touch %t.o
 
+// RUN: %clang -target arm64_32-apple-watchos5.2 -isysroot %S/Inputs/WatchOS6.0.sdk -mlinker-version=0 -### %t.o 2>&1 \
+// RUN:   | FileCheck --check-prefix=LINKER-OLD %s
+// RUN: %clang -target arm64_32-apple-watchos5.2 -isysroot %S/Inputs/WatchOS6.0.sdk -mlinker-version=400 -### %t.o 2>&1 \
+// RUN:   | FileCheck --check-prefix=LINKER-OLD %s
 // RUN: %clang -target arm64_32-apple-watchos5.2 -isysroot %S/Inputs/WatchOS6.0.sdk -mlinker-version=520 -### %t.o 2>&1 \
-// RUN:   | FileCheck %s
+// RUN:   | FileCheck --check-prefix=LINKER-NEW %s
 // RUN: %clang -target x86_64-apple-watchos6-simulator -isysroot %S/Inputs/WatchOS6.0.sdk -mlinker-version=520 -### %t.o 2>&1 \
 // RUN:   | FileCheck --check-prefix=SIMUL %s
 
-// CHECK: "-platform_version" "watchos" "5.2.0" "6.0.0"
+// LINKER-OLD: "-watchos_version_min" "5.2.0"
+// LINKER-NEW: "-platform_version" "watchos" "5.2.0" "6.0.0"
 // SIMUL: "-platform_version" "watchos-simulator" "6.0.0" "6.0.0"