Page MenuHomePhabricator

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

Authored by dmajor on Feb 18 2020, 12:00 PM.

Details

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.

Diff Detail

Event Timeline

dmajor created this revision.Feb 18 2020, 12:00 PM

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.

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

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.

@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, 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 this revision to Diff 246771.Feb 26 2020, 9:49 AM
dmajor edited the summary of this revision. (Show Details)

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

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

dexonsmith accepted this revision.Feb 26 2020, 11:20 AM

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!

This revision is now accepted and ready to land.Feb 26 2020, 11:20 AM

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

hans added a subscriber: hans.Feb 27 2020, 4:59 AM

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.

This revision was automatically updated to reflect the committed changes.