This is an archive of the discontinued LLVM Phabricator instance.

[UTC] Enable --function-signature by default
ClosedPublic

Authored by nikic on Mar 2 2023, 1:59 AM.

Details

Summary

This patch enables --function-signature by default under --version 2 and makes --version 2 the default. This means that all newly created tests will check the function signature, while leaving old tests alone.

There's two motivations for this change:

  • Without --function-signature, the generated check lines may fail in a very hard to understand way if the test both includes a function definition and a call to that function. (Though we could address this by making the CHECK-LABEL stricter, without checking the full signature.)
  • This actually checks that uses of the arguments in the function body use the correct argument, instead of matching against any variable.

This is a replacement for D139006 and D140212 based on the --version mechanism.

I did not include an opt-out flag --no-function-signature because I'm not sure we need it. Would be happy to include it though, if desired.

Diff Detail

Event Timeline

nikic created this revision.Mar 2 2023, 1:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2023, 1:59 AM
nikic requested review of this revision.Mar 2 2023, 1:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2023, 1:59 AM
spatel accepted this revision.Mar 3 2023, 6:38 AM

I didn't follow the reviews of D142473 and D144963 while they were in progress, but this is the logical conclusion to the set. LGTM.

This revision is now accepted and ready to land.Mar 3 2023, 6:38 AM
arichardson added inline comments.Mar 3 2023, 3:59 PM
llvm/utils/UpdateTestChecks/common.py
255

I think this would be the place to add if args.version >= 2: args.function_signature = True

llvm/utils/update_cc_test_checks.py
352 ↗(On Diff #501810)

Could we update function_signature to True if version == 2 after parsing? That would avoid the need for the new helper function.

Thanks for working on this! How bad is the test diff if you remove the hardcoded --version=1 from lit.local.cfg?

nikic added a comment.Mar 4 2023, 1:03 AM

Thanks for working on this! How bad is the test diff if you remove the hardcoded --version=1 from lit.local.cfg?

Beyond the actual functional diff, this will touch every single UTC test simply due to the addition of --version 2 to UTC_ARGS. I'd need an update_utc_test_checks.py script for that ;)

llvm/utils/UpdateTestChecks/common.py
255

That wouldn't be sufficient due to the on-the-fly flags update feature (that probably isn't worth the complexity it introduces -- we should consider removing it.) I could extract the parse_args() calls into a common helper though, where we could do this.

nikic updated this revision to Diff 502650.Mar 6 2023, 7:48 AM

Adjust args.function_signature instead of adding a helper to derive the correct value.

arichardson accepted this revision.Mar 6 2023, 8:36 AM

Thanks, LGTM

This revision was landed with ongoing or failed builds.Mar 7 2023, 1:28 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2023, 1:28 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript