This is an archive of the discontinued LLVM Phabricator instance.

[FileCheck] Use %ProtectFileCheckOutput in allow-unused-prefixes.txt
ClosedPublic

Authored by mtrofin on Nov 2 2020, 10:27 AM.

Diff Detail

Event Timeline

mtrofin created this revision.Nov 2 2020, 10:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2020, 10:27 AM
mtrofin requested review of this revision.Nov 2 2020, 10:27 AM

I think only the FileCheck commands that are themselves FileChecked need the protection. The other ones are intended to be rendered to users (humans running the tests) so would benefit from being able to be customized with FILECHECK_OPTS, I think?

dblaikie added inline comments.Nov 2 2020, 10:38 AM
llvm/test/FileCheck/allow-unused-prefixes.txt
3–7

I /think/ these two don't need it, since their output doesn't feed into another FileCheck command (so I assume it's intended to be seen/understood by a human) - but I haven't looked closely at the test. Is there some reason these two would need stabilized/non-customizable output?

mtrofin marked an inline comment as done.Nov 2 2020, 10:41 AM

I think only the FileCheck commands that are themselves FileChecked need the protection. The other ones are intended to be rendered to users (humans running the tests) so would benefit from being able to be customized with FILECHECK_OPTS, I think?

llvm/test/FileCheck/allow-unused-prefixes.txt
3–7

What if the user sets --allow-unused-prefixes in their env?

dblaikie added inline comments.Nov 2 2020, 10:46 AM
llvm/test/FileCheck/allow-unused-prefixes.txt
3–7

My guess would be that that wouldn't be reasonably supported through FILECHECK_OPTS - in the same way that passing --check-prefixes=SOMETHING via FILECHECK_OPTS wouldn't be usefully supported either. FILECHECK_OPTS is for things like --color and --dump-* flags, I believe.

mtrofin marked an inline comment as done.Nov 2 2020, 10:51 AM
mtrofin added inline comments.
llvm/test/FileCheck/allow-unused-prefixes.txt
3–7

The 2 tests are that FileCheck passes - i.e. it returns 0. So I'd argue they are similar to the other cases, in that we rely on the user not accidentally messing with the options.

dblaikie added inline comments.Nov 2 2020, 11:04 AM
llvm/test/FileCheck/allow-unused-prefixes.txt
3–7

That property (that FileCheck passes) is true of every test that uses FileCheck (ie: lots of LLVM tests), not only the ones intended to check FileCheck's behavior.

Perhaps @jhenderson has some perspective here.

jhenderson added inline comments.
llvm/test/FileCheck/allow-unused-prefixes.txt
1–4

The common pattern I've seen is to put %ProtectFileCheckOutput first before anything else like not. There are some good examples in a number of other FileCheck tests, like numeric-defines.txt.

3–7

It is my belief that @dblaikie is correct. @thopre is probably more up-to-speed on the details of this, but based on a test he relatively recently wrote (see again numeric-defines.txt), you only need it where the first FileCheck call is fed into another FileCheck call. In those cases, we are testing the output of FileCheck, so need it constrained. In other cases, like here, we are only testing the matching behaviour.

Using the general environment to change what prefixes are specified seems like a terrible idea, as other people wouldn't have the environment set up in that way and therefore the test behaviour would be different. I can see a niche case for using the environment variable within a limited scope of tests that were explicitly written to support that, so that a user could rapidly test several different variations of behaviour, but that isn't the case for this test, or indeed any of FileCheck's tests. The same basic argument would go for other commands like --allow-unused-prefixes which change FileCheck's behaviour in a way that would cause the test to pass/fail under different environments.

Another point here is that setting %ProtectFileCheckOutput explicitly prevents using --dump-* to get FileCheck to dump more output when this test fails, which defeats the point of the environment variable.

mtrofin updated this revision to Diff 302996.Nov 4 2020, 5:57 PM
mtrofin marked 4 inline comments as done.

feedback

Sorry for the delay - ptal. Thanks!

dblaikie accepted this revision.Nov 4 2020, 6:19 PM

Looks good to me!

This revision is now accepted and ready to land.Nov 4 2020, 6:19 PM
jhenderson accepted this revision.Nov 5 2020, 12:08 AM

LGTM too.

This revision was landed with ongoing or failed builds.Nov 5 2020, 7:08 AM
This revision was automatically updated to reflect the committed changes.