Page MenuHomePhabricator

[UpdateTestChecks] Emit warning when invalid value for -check-prefix(es) option
ClosedPublic

Authored by xbolva00 on Jul 11 2019, 1:41 PM.

Details

Summary

The script is silent for the following issue:
FileCheck %s -check-prefix=CHECK,POPCOUNT

FileCheck will catch it later, but I think we can warn here too.

Now it warns:
./update_llc_test_checks.py file.ll

WARNING: Supplied prefix 'CHECK,POPCOUNT' is invalid. Prefix must contain only alphanumeric characters, hyphens and underscores. Did you mean --check-prefixes=CHECK,POPCOUNT?

Diff Detail

Repository
rL LLVM

Event Timeline

xbolva00 created this revision.Jul 11 2019, 1:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2019, 1:41 PM

lebedev.ri hit this recently so I added him as reviewer.

Thank you, i have stumbled into this a few times :)

To be noted, the problem is: --check-prefix=A,B,C.
But i'm not sure about --check-prefix=A --check-prefix=B --check-prefix=C.

xbolva00 updated this revision to Diff 209324.Jul 11 2019, 1:55 PM

More precise checker.

Thank you, i have stumbled into this a few times :)

To be noted, the problem is: --check-prefix=A,B,C.
But i'm not sure about --check-prefix=A --check-prefix=B --check-prefix=C.

Thanks, I fixed it.

-check-prefix=A,B -check-prefix=C

^ This won't be detected, not sure if worth to match. What do you think?

I'm not confident that will do the right thing. (i expect this to have some false-negatives, iff it is allowed to specify -check-prefix= more than once)
I see two paths:

  1. split the runline by whitespace, and check every element that starts with -check-prefix= that it does not contain ,. Will not have false-positives and no false-negatives, but kinda ugly.
  2. Revert to the previous diff, complain if there are any -check-prefix= IFF we had more than a single prefix; the warning will need to be changed - it will have false-positives - so it is more of a stylistic guideline.
xbolva00 updated this revision to Diff 209338.Jul 11 2019, 2:28 PM

Improved.

I'm not confident that will do the right thing. (i expect this to have some false-negatives, iff it is allowed to specify -check-prefix= more than once)
I see two paths:

  1. split the runline by whitespace, and check every element that starts with -check-prefix= that it does not contain ,. Will not have false-positives and no false-negatives, but kinda ugly.
  2. Revert to the previous diff, complain if there are any -check-prefix= IFF we had more than a single prefix; the warning will need to be changed - it will have false-positives - so it is more of a stylistic guideline.

I implemented 1. option. Thanks for ideas.

xbolva00 updated this revision to Diff 209342.Jul 11 2019, 2:35 PM
xbolva00 updated this revision to Diff 209344.

Renamed variable.

Nice, thanks for looking into this.
I think this is good, @RKSimon ?

is there anyway that we could move this to common.py so we can reuse it in the other update scripts?

xbolva00 updated this revision to Diff 209517.Jul 12 2019, 9:55 AM
xbolva00 retitled this revision from [UpdateLLCTestChecks] Emit warning when invalid value for -check-prefix option to [UpdateLLC/MIRTestChecks] Emit warning when invalid value for -check-prefix option.
xbolva00 edited the summary of this revision. (Show Details)

Moved to common.

is there anyway that we could move this to common.py so we can reuse it in the other update scripts?

Thanks, I forgot about mir. Moved to common, now it warns for broken LLC/MIR prefixes.

xbolva00 updated this revision to Diff 209546.Jul 12 2019, 11:25 AM
xbolva00 retitled this revision from [UpdateLLC/MIRTestChecks] Emit warning when invalid value for -check-prefix option to [UpdateTestChecks] Emit warning when invalid value for -check-prefix option.

Also check IR.

Please can you add this to update_analyze_test_checks.py and update_mca_test_checks.py as well? update_cc_test_checks.py maybe - not sure what state that is in though tbh.

xbolva00 updated this revision to Diff 209590.Jul 12 2019, 2:21 PM

Add checker to all update scripts.

xbolva00 added a comment.EditedJul 15 2019, 2:16 AM

Any further comments?

gbedwell added inline comments.Jul 15 2019, 3:33 AM
utils/UpdateTestChecks/common.py
272 ↗(On Diff #209590)

I doubt whether it could ever cause an issue in practice, but it's probably safer to specify maxsplit here.

prefix = part.split('=', 1)[1]

If we somehow ever ended up with a string like '--check-prefix=foo=bar' then we'd end up with 'foo=bar' in element 1 rather than 'foo' in element 1 and 'bar' in element 2 which would subsequently be silently ignored. Alternatively you could just check that the number of elements returned from the split is exactly two and raise an error or warning if not.

xbolva00 updated this revision to Diff 209795.Jul 15 2019, 4:16 AM
xbolva00 edited the summary of this revision. (Show Details)

Reworked to use regex to check prefixes.

@gbedwell Thanks for ideas!

I decided to take the regex [0] which FileCheck uses to check prefixes and apply it here.

[0] https://github.com/llvm/llvm-project/blob/2a7f5204602938ae89b0860e9412603d1951d945/llvm/lib/Support/FileCheck.cpp

xbolva00 updated this revision to Diff 209799.Jul 15 2019, 4:43 AM

Handle empty prefixes.

@lebedev.ri @RKSimon

FileCheck says
"Prefixes must be unique, *start with a letter* and contain only alphanumeric characters, hyphens and underscores"

but it does not enforce it in RE, should we enforce it here?

xbolva00 updated this revision to Diff 209801.Jul 15 2019, 4:49 AM
xbolva00 retitled this revision from [UpdateTestChecks] Emit warning when invalid value for -check-prefix option to [UpdateTestChecks] Emit warning when invalid value for -check-prefix(es) option.

Check if prefix is unique.

xbolva00 updated this revision to Diff 209802.Jul 15 2019, 4:55 AM
xbolva00 updated this revision to Diff 209803.
gbedwell added a comment.EditedJul 15 2019, 4:59 AM

I've tried the latest update and it seems to work well for me. The only suggestion I'd make at this stage is adding a message like (did you mean --check-prefixes?) to the end of the warning if a comma is spotted in the prefix string.

--check-prefix=FOO,BAR

gives a fairly generic warning message now:

WARNING: Supplied prefix 'FOO,BAR' is invalid. Prefix must start with a letter and contain only alphanumeric characters, hyphens and underscores.

which I can believe that if I was new to filecheck would probably cause me to spend more time debugging the issue than I'd like.

utils/UpdateTestChecks/common.py
287 ↗(On Diff #209801)

A very nitpicky and minor style nit, but you can contain single quotes in double quoted strings and double quotes in single quotes strings, so it may be more readable like this as you don't have to escape anything.

print('WARNING: Supplied prefix "%s" is not unique in prefix list.'

or:

print("WARNING: Supplied prefix '%s' is not unique in prefix list."
xbolva00 updated this revision to Diff 209867.EditedJul 15 2019, 8:05 AM
xbolva00 edited the summary of this revision. (Show Details)

Updated. Hint added.

xbolva00 marked 2 inline comments as done.Jul 15 2019, 8:06 AM
xbolva00 added inline comments.
utils/UpdateTestChecks/common.py
287 ↗(On Diff #209801)

+1

Yeah, this looks better.

xbolva00 updated this revision to Diff 209869.Jul 15 2019, 8:09 AM
xbolva00 marked an inline comment as done.
xbolva00 updated this revision to Diff 209910.Jul 15 2019, 10:49 AM

Revert unneeded change.

RKSimon accepted this revision.Mon, Jul 29, 3:32 AM

LGTM - cheers!

This revision is now accepted and ready to land.Mon, Jul 29, 3:32 AM

LGTM - cheers!

Sorry for the delay. I've been out of the office a little bit recently!
LGTM too.

Thank you for looking into this!

Thank you all for comments and ideas

This revision was automatically updated to reflect the committed changes.