This is an archive of the discontinued LLVM Phabricator instance.

[utils] change update_test_checks.py use of 'TMP' value names
ClosedPublic

Authored by spatel on May 26 2020, 12:56 PM.

Details

Summary

As discussed in PR45951:
https://bugs.llvm.org/show_bug.cgi?id=45951

There's a potential name collision between -instnamer and/or manually-generated IR test files with update_test_checks.py because all of them try to use the variable name that should never be used: "tmp".

This patch proposes to reduce the odds of collision and adds a warning if we detect the problem. This will cause regression test churn when regenerating CHECK lines on existing files. We could do a mass update to get that out of the way all at once if that seems better.

Diff Detail

Event Timeline

spatel created this revision.May 26 2020, 12:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2020, 12:56 PM

do we have any idea of the likely scope of the test changes?

spatel added a comment.EditedMay 27 2020, 12:27 PM

do we have any idea of the likely scope of the test changes?

Assuming I did this correctly - check for auto-generated files that contain 'TMP':

test $ grep -l TMP `grep -rl update_test_checks *` | wc -l
    1044
RKSimon accepted this revision.May 30 2020, 8:59 AM

LGTM - let's go with this. I'd not bother with the mass update, but an example refresh as part of this patch commit would make sense.

This revision is now accepted and ready to land.May 30 2020, 8:59 AM
This revision was automatically updated to reflect the committed changes.
jdoerfert reopened this revision.May 31 2020, 4:03 PM
jdoerfert added a subscriber: jdoerfert.

This needs a command line flag so people can change the prefix. The default for that flag should be "TMP" to minimize the noise in updates. We also should have a test.
FWIW, the UTC_FLAGS logic will "remember" the flag chosen by the user so if you need to change from "TMP" to something else it will be recorded and no further interaction will be necessary.

This revision is now accepted and ready to land.May 31 2020, 4:03 PM
spatel added a comment.Jun 1 2020, 3:05 AM

This needs a command line flag so people can change the prefix. The default for that flag should be "TMP" to minimize the noise in updates. We also should have a test.
FWIW, the UTC_FLAGS logic will "remember" the flag chosen by the user so if you need to change from "TMP" to something else it will be recorded and no further interaction will be necessary.

I disagree with adding an option for this. We don't want to turn a bug into a feature and carry that complexity cost for future users forever. The anonymous variable name is an implementation detail of the script that newcomers should not have to think about.

But I understand that test churn causes grief for current users (I'm a big user of the script and take the blame for the bug).

The problem is that this script and -instnamer conflict perfectly on the name "tmp". One option that we didn't consider in the discussion in PR45951: we could leave this script using "TMP" if we change -instnamer to use something else. Does anyone see any legacy constraint on the "tmp" naming in -instnamer?

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2020, 4:15 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
spatel reopened this revision.Jun 1 2020, 4:40 AM

Effectively reverted (but still have the name conflict warning at least) with:
rGe5b877275673

If we can change instnamer, we can probably keep living with this script as-is. Alternatively, let's re-fix this script and mass update test files, so there's no general concern about churn of existing tests.

This revision is now accepted and ready to land.Jun 1 2020, 4:40 AM
jdoerfert added a comment.EditedJun 1 2020, 7:20 AM

First, I think the warning is strictly a good thing. Thanks for keeping that in!

I don't think the options are really "complexity to the user" given a sensible default (= the old value). If a user sees a warning that says: Name conflict, use --nameless-prefix=XXXXX, where XXXXX is a string not otherwise used in your IR, they will happily do so. The UTC_ARGS extension makes it permanent and no one needs to worry about this until they see a warning again. (If we can "hide" the option from the help message, I'm fine with that too.)

Adding a test is just something we should do as it is easy to accidentally break functionality like the warning.

spatel closed this revision.Jun 1 2020, 9:07 AM

First, I think the warning is strictly a good thing. Thanks for keeping that in!

Sure - we've been missing that for a long time.

I don't think the options are really "complexity to the user" given a sensible default (= the old value). If a user sees a warning that says: Name conflict, use --nameless-prefix=XXXXX, where XXXXX is a string not otherwise used in your IR, they will happily do so. The UTC_ARGS extension makes it permanent and no one needs to worry about this until they see a warning again. (If we can "hide" the option from the help message, I'm fine with that too.)

I'm still not convinced that's worth doing. I did change instnamer though:
rGdd54432a0f5a
...so between the warning and at least 1 of these not using 'tmp', I think we're good enough.

Adding a test is just something we should do as it is easy to accidentally break functionality like the warning.

Yep, I didn't remember that we had tests for scripts. So we have that now, and there's a clang test that wiggles on this too...although that seems like we should generalize that file to avoid killing all of the bots like I did. :)

I think we're good enough.

I agree. If this doesn't bother anyone anymore there is no need for any options ;)

Thanks for the test!