Page MenuHomePhabricator

[lld-macho][nfc] Centralize usages of ld64.lld in tests
ClosedPublic

Authored by thevinster on Feb 9 2022, 5:06 PM.

Details

Reviewers
int3
oontvoo
Group Reviewers
Restricted Project
Commits
rGef764ee20746: [lld-macho][nfc] Centralize usages of ld64.lld in tests
Summary

We have a mix of substituted lld (%lld) and hard-coded lld (ld64.lld) commands.
When testing with different versions of LLD, this would require going into every place
where lld is hard-coded and changing that. If we centralize it, this'll only require us
to modify it in only one place and will make it easy to run the same test suite. Plus,
this will make it be consistent with how we write other tests.

Diff Detail

Event Timeline

thevinster created this revision.Feb 9 2022, 5:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 9 2022, 5:06 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
thevinster published this revision for review.Feb 9 2022, 5:09 PM
thevinster edited the summary of this revision. (Show Details)
Herald added a project: Restricted Project. · View Herald TranscriptFeb 9 2022, 5:09 PM
thevinster edited the summary of this revision. (Show Details)Feb 9 2022, 5:37 PM

Yeah I think this will make testing a bit more convenient. @thakis @oontvoo, any objections?

Bikeshedding: How about %lld-noarg? It's a bit more descriptive and only two characters longer.

int3 added a comment.Feb 9 2022, 5:43 PM

Or %lld-no-arg, to be a bit more consistent with %no_fatal_warnings_lld. (I like that we're using hyphens instead of underscores though; IMO %no_fatal_warnings_lld should use hyphens too)

Bikeshedding: How about %lld-noarg? It's a bit more descriptive and only two characters longer.

Unfortunately, we can't start with %lld anymore otherwise it would get replaced with the value of %lld and then just append -noarg which makes the command invalid. I'm happy to change it to %no-arg-lld.

Yeah I think this will make testing a bit more convenient. @thakis @oontvoo, any objections?

LGTM

Bikeshedding: How about %lld-noarg? It's a bit more descriptive and only two characters longer.

(slightly) prefer lld-noarg as well. :)
On the other hand, why doesn't re-using %lld plus additional args, which would override the defalut ones provided by %lld anyways, work?

Unfortunately, we can't start with %lld anymore otherwise it would get replaced with the value of %lld and then just append -noarg which makes the command invalid. I'm happy to change it to %no-arg-lld.

I stand corrected. If we move that substitution before we define %lld then it should work. So, in theory %lld-no-arg and %lld-noarg should work. Although, I do like %no-arg-lld a bit better since it matches with how no_fatal_warnings is defined.

why doesn't re-using %lld plus additional args, which would override the defalut ones provided by %lld anyways, work?

The substitution happens in order and it sees %lld within %lld-noarg first, so it ends up replacing that whole command and the command ends up failing a whole bunch of tests with ld64.lld: error: unknown argument '-fatal_warnings-raw'. So, moving that statement up should solve the issue. If the majority feels %lld-noarg is a better fit, I'm happy to change it to that; although, isn't noarg technically two words? (So, it should be %lld-no-arg?)

Switch name to lld-noarg

int3 accepted this revision.Feb 10 2022, 11:29 AM

I think lld-no-arg is a bit nicer, but this is good too :)

This revision is now accepted and ready to land.Feb 10 2022, 11:29 AM
oontvoo accepted this revision.Feb 10 2022, 11:32 AM

why doesn't re-using %lld plus additional args, which would override the defalut ones provided by %lld anyways, work?

The substitution happens in order and it sees %lld within %lld-noarg first, so it ends up replacing that whole command and the command ends up failing a whole bunch of tests with ld64.lld: error: unknown argument '-fatal_warnings-raw'.

ok, gotcha!

So, moving that statement up should solve the issue. If the majority feels %lld-noarg is a better fit, I'm happy to change it to that; although, isn't noarg technically two words? (So, it should be %lld-no-arg?)

On a second thought, I think you have a good point on %no_arg_lld , to be consistent with _no_fatal_warning_* .

Rename to no-arg-lld

Overall, I gave it some thought on the naming. To be consistent with a part of the naming (e.g. no_fatal_warnings_), it makes sense to have no-arg be as the prefix as well. We do have a mix of - and _ (e.g. lld-watchos and no_fatal_warnings_lld). I broke the tie and slightly prefer -. Hence, the result no-arg-lld.

This revision was landed with ongoing or failed builds.Feb 10 2022, 5:27 PM
This revision was automatically updated to reflect the committed changes.