This is an archive of the discontinued LLVM Phabricator instance.

[UpdateTestChecks] Match define for labels
ClosedPublic

Authored by sebastian-ne on Nov 30 2022, 5:15 AM.

Details

Summary

Previously, the label also matched function calls with the function
name, which caused tests to fail because the label matched on the wrong
line.
Add the define prefix, so only function defines are matched.

Diff Detail

Event Timeline

sebastian-ne created this revision.Nov 30 2022, 5:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2022, 5:15 AM
sebastian-ne requested review of this revision.Nov 30 2022, 5:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2022, 5:15 AM

This is going to cause such a churn :(
[Un]fortunately, i think i've just yesterday run into that.
Could you please ensure that the utils/update_cc_test_checks.py is also being fixed, and maybe add a test?

(Also, it may be good to add the test with the actual problematic case...)

Thanks for the review!
I updated the update_cc_tests tests and added a test where the FileCheck failed previously.

Herald added a project: Restricted Project. · View Herald Transcript
lebedev.ri accepted this revision.Nov 30 2022, 6:32 AM

LGTM.
I do think this is an important fix, not the least because i've also just hit that,
but i do want to explicitly call out that this will cause a massive test case churn,
so i do want a second reviewer here.

This revision is now accepted and ready to land.Nov 30 2022, 6:32 AM
mtrofin accepted this revision.Nov 30 2022, 7:30 AM

LGTM, but I'd wait for Johannes to review it, too (because of e67f6477fd1ed29acbeddf8482c25d8db826912f. I for one don't quite follow the reasoning there wrt adding the else)

LGTM, but I'd wait for Johannes to review it, too (because of e67f6477fd1ed29acbeddf8482c25d8db826912f. I for one don't quite follow the reasoning there wrt adding the else)

That commit explicitly links to https://reviews.llvm.org/D68819,
after reading that thread, it is obvious that rGe67f6477fd1ed29acbeddf8482c25d8db826912f
was made prevent the churn we are about to expirience.

Agreed that the churn is annoying, but at least unlike the function-signature flag (which I'd quite like to have on by default tbh) it only affects a single line rather than also including variable captures.

llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/define_after_call.ll.expected
14

Does this actually fail without the define match? I wouldn't expect it to?

sebastian-ne added inline comments.Dec 1 2022, 2:07 AM
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/define_after_call.ll.expected
14

Yes, it fails (which is why I put the opt | FileCheck line into the .test script as well).
CHECK-LABEL: @b( matches the call i32 @b(i32 0) line and and as labels are matched before check-lines, then the CHECK-NEXT: [[VAL:%.*]] = call i32 @b(i32 0) line doesn’t find its match anymore (or the ret i32 match, I’m not quite sure, but it definitely fails).

I guess this is fine to merge. I’ll leave it for a day in case someone has more comments.

I guess this is fine to merge. I’ll leave it for a day in case someone has more comments.

I guess, we could be pedantic and post an RFC to disscuss.
But this really is a bug, and the alternative solutions would only hide it.

This revision was automatically updated to reflect the committed changes.

So now every single test needs to be regenerated? It'll create straw diff from nowhere...

So now every single test needs to be regenerated? It'll create straw diff from nowhere...

We don’t need to regenerate every test.
Similar to how we don’t reformat all of LLVM after a change in clang-format, we can just do that when regenerating tests when they have changes anyway.
So, the churn is that we have extra changes when regenerating a test for a patch.
(In my personal experience there are often extra changes anyway, because metadata was added or similar changes that pass the old CHECKs but show up on a freshly regenerated test.)

nikic added a comment.Dec 12 2022, 1:47 PM

So now every single test needs to be regenerated? It'll create straw diff from nowhere...

We don’t need to regenerate every test.
Similar to how we don’t reformat all of LLVM after a change in clang-format, we can just do that when regenerating tests when they have changes anyway.
So, the churn is that we have extra changes when regenerating a test for a patch.

This kind of change requires generating check lines in a separate commit prior to the patch, because functional patches must not contain this kind of noise. This kind of change to UTC output is very annoying.

Please revert this patch pending further discussion.

I think there's three options here:

  1. The status quo, which is that you must pass --function-signature if you encounter this issue.
  2. What this patch does, which causes massive churn.
  3. Something like adding --function-signature to UTC_ARGS by default for new tests, while not doing so for old ones.

If we are not content with the status quo, then we should do something along the lines of 3. Possibly not with --function-signature but something like a --version N flag that defaults to the latest but gets persisted in UTC_ARGS. This way we are free to improve UTC without being worried about test churn.

I agree with the revert - we'd need lots of commits like this:
b7232dafe69eb04c14217 (cc @arsenm)

Another possibility to fix this with less churn:
Add a warning during (re-)generation for any test where the CHECK-LABEL could be going wrong. So scan the file for a call to a function name that is also defined in that file?
We already have a warning like that for variables named %tmp. So if a test file contains the potentially buggy construct, then warn the user that they should regenerate with "--function-signature" to be safe.

jdoerfert added a comment.EditedDec 15 2022, 4:48 PM

Why not just --function-define as a way to enable the define but not the captures, if that is really something necessary.
It is unclear to me why function-signature is not sufficient here and what the big problem with using it is.

FWIW, I also already redid all OpenMP and Attributor tests now, double churn!

Why not just --function-define as a way to enable the define but not the captures, if that is really something necessary.
It is unclear to me why function-signature is not sufficient here and what the big problem with using it is.

I believe the motivation here is the default behavior: Experienced contributors know how to recognize this problem and use --function-signature to avoid it, but new contributors will not be aware of it, and the error message produced by FileCheck is not super helpful.

That's why I'm suggesting to change the default behavior for new tests, and @spatel suggests to add detection for the scenario and a warning that tells you to use --function-signature. Either of those should address the discoverability issue.

I believe the motivation here is the default behavior

Correct, update_test_checks produces a broken test for some input and I think this is a bug that we should fix.

Sorry for the test churn and thanks for the revert (I only got to work a few hours after nikic already reverted).

That's why I'm suggesting to change the default behavior for new tests

Adding --function-signature by default sounds like a good idea to me.
Is there any reason why we wouldn’t want to enable this by default (for new tests)?

Adding --function-signature by default sounds like a good idea to me.
Is there any reason why we wouldn’t want to enable this by default (for new tests)?

No objection from me for new files. There's some small time cost for added checks, but AFAIK nobody cares about that, so the added verification to also make sure we didn't accidentally swap function args during regex is worth it.

I like the idea of defaulting to --function-signature for new tests a lot, so I've implemented that in D140212