Hello all,
TL;DR; if you used FileCheck --check-prefixes and you missed (misspelled, for instance) one of the prefixes in your test, FileCheck silently ignores that and the test passes.
1579 tests have this property.
The details
Thanks for doing this! I am impressed that the test count is so high; what is that, around 2% of all tests? I wonder how many are typos (bad) versus genuinely unused (benign), but that would be tedious to determine.
Example of a typo: clang/test/AST/ast-dump-decl.c, MODULE vs MODULES
Example of unused: llvm/test/DebugInfo/X86/dbg-value-inlined-parameter.ll, specifies LINUX and DARWIN but neither is used.
I’ll take this opportunity to mention that I’m planning a 10% project to look at the inverse question: What directives are in the test files but aren’t exercised by any FileCheck run? This is trickier as you need to collect data across multiple runs, and tests run in parallel. Also it’s a bit ambiguous what “looks like” a directive, they aren’t really all intended to be directives. Should be fun.
If anyone is already delving into this, I’d be curious to hear about the approach.
Thanks,
–paulr
Hello all,
TL;DR; if you used FileCheck --check-prefixes and you missed (misspelled, for instance) one of the prefixes in your test, FileCheck silently ignores that and the test passes.
1579 tests have this property.
Wow, that is quite a “whoops”. Thank you for catching this, your plan sounds really great.
-Chris
Hello all,
TL;DR; if you used FileCheck --check-prefixes and you missed (misspelled, for instance) one of the prefixes in your test, FileCheck silently ignores that and the test passes.
1579 tests have this property.
Wow, that is quite a “whoops”. Thank you for catching this, your plan sounds really great.
Thanks, credit also goes to maskray for helping flesh out the plan.
Something that can be done to progressively roll-out the feature to more and more directories is to use the lit configuration.
For example I cleaned up MLIR to be compliant with the FileCheck stricter flag -enable-var-scope which catches another class of bugs, but couldn’t practically go through the thousands of failures in the monorepo, so the flag is enabled by default in the MLIR test folder: https://github.com/llvm/llvm-project/blob/master/mlir/test/lit.cfg.py#L84-L87
You can do the same on a per-directory basis anywhere. If you get to change the default, you can use the same strategy to opt-out entire subtrees.
(I’ll likely try to do the same as enable-var-scope in MLIR and enable it by default there)
Thanks!
Hello all,
TL;DR; if you used FileCheck --check-prefixes and you missed (misspelled, for instance) one of the prefixes in your test, FileCheck silently ignores that and the test passes.
1579 tests have this property.
Wow, that is quite a “whoops”. Thank you for catching this, your plan sounds really great.
It has yet to be seen how many are actual "bugs" or "oversights" vs. intentional.
We should try to track that as well 
~ Johannes
This seems like a great change! In order to find the specific unused prefixes it would be helpful if you landed the patch with the true default (I applied the patch locally in the meantime)
An update: as of 871d658c9ceb, the flag is now available, if folks need to use it.
There are currently 1350 owner-less failures in the spreadsheet. These seem to be the larger areas there.
If you see an area you have ownership or expertise in, please sign up for fixing the tests by Monday, Nov. 9.
Otherwise, I will “blanket-add” --allow-unused-prefixes=true to the remaining failing tests.
There are currently 1350 owner-less failures in the spreadsheet. These seem to be the larger areas there.
If you see an area you have ownership or expertise in, please sign up for fixing the tests by Monday, Nov. 9.
Otherwise, I will “blanket-add” --allow-unused-prefixes=true to the remaining failing tests.
If/when you do that, probably worth adding a comment at each site to clarify that this was added automatically, not vetted/intentionally added by a human. Something like “// FIXME: Verify that unused prefixes are used intentionally” or the like.
There are currently 1350 owner-less failures in the spreadsheet. These seem to be the larger areas there.
If you see an area you have ownership or expertise in, please sign up for fixing the tests by Monday, Nov. 9.
Otherwise, I will “blanket-add” --allow-unused-prefixes=true to the remaining failing tests.
If/when you do that, probably worth adding a comment at each site to clarify that this was added automatically, not vetted/intentionally added by a human. Something like “// FIXME: Verify that unused prefixes are used intentionally” or the like.
Ack. or, we can grep for -allow-unused-prefixes=true, wdyt?
There are currently 1350 owner-less failures in the spreadsheet. These seem to be the larger areas there.
If you see an area you have ownership or expertise in, please sign up for fixing the tests by Monday, Nov. 9.
Otherwise, I will “blanket-add” --allow-unused-prefixes=true to the remaining failing tests.
If/when you do that, probably worth adding a comment at each site to clarify that this was added automatically, not vetted/intentionally added by a human. Something like “// FIXME: Verify that unused prefixes are used intentionally” or the like.
Ack. or, we can grep for -allow-unused-prefixes=true, wdyt?
Not sure I understand who/when they would grep for that?
I was suggesting adding an explicit “this use of --allow-unused-prefixes=true hasn’t been confirmed as intentional” so that the backwards compatibility cases can be distinguished from the intentional cases when someone is reading the test case, rather than puzzling over why this flag was added (which looks intentional) though the unused prefix may not make sense in that particular test. It’ll make it easier in the future when someone does look at the test for them to not feel like they’re being implicitly told “this use of unused prefixes is intentional” (by the presence of an explicit flag requesting such support) while staring at the test and not being able to see why someone would’ve done that intentionally.
There are currently 1350 owner-less failures in the spreadsheet. These seem to be the larger areas there.
If you see an area you have ownership or expertise in, please sign up for fixing the tests by Monday, Nov. 9.
Otherwise, I will "blanket-add" --allow-unused-prefixes=true to the remaining failing tests.
If/when you do that, probably worth adding a comment at each site to clarify that this was added automatically, not vetted/intentionally added by a human. Something like "// FIXME: Verify that unused prefixes are used intentionally" or the like.
Ack. or, we can grep for -allow-unused-prefixes=true, wdyt?
Not sure I understand who/when they would grep for that?
I was suggesting adding an explicit "this use of --allow-unused-prefixes=true hasn't been confirmed as intentional" so that the backwards compatibility cases can be distinguished from the intentional cases when someone is reading the test case, rather than puzzling over why this flag was added (which looks intentional) though the unused prefix may not make sense in that particular test. It'll make it easier in the future when someone does look at the test for them to not feel like they're being implicitly told "this use of unused prefixes is intentional" (by the presence of an explicit flag requesting such support) while staring at the test and not being able to see why someone would've done that intentionally.
Directly CCing some folks who can fix or find people to fix some
directories (*/AMDGPU, */X86, */OpenMP, */AArch64) on
After these big directories are cleaned up, the remaining tests should
be manageable in amount.
How to reproduce:
sed -i '/allow-unused-prefixes/s/true/false/' llvm/utils/FileCheck/FileCheck.cpp
git update-index --assume-unchanged llvm/utils/FileCheck/FileCheck.cpp
ninja check-llvm # or check-clang ...
Thanks!
There are currently 1350 owner-less failures in the spreadsheet. These seem to be the larger areas there.
If you see an area you have ownership or expertise in, please sign up for fixing the tests by Monday, Nov. 9.
Otherwise, I will “blanket-add” --allow-unused-prefixes=true to the remaining failing tests.
If/when you do that, probably worth adding a comment at each site to clarify that this was added automatically, not vetted/intentionally added by a human. Something like “// FIXME: Verify that unused prefixes are used intentionally” or the like.
Ack. or, we can grep for -allow-unused-prefixes=true, wdyt?
Not sure I understand who/when they would grep for that?
AAh! Nevermind me 
forgot that there are reasonable cases using it. Yes, it makes sense to add a FIXME, perhaps at the start of each file
I think it would make more sense to add it at each individual call site. This ensures that all cases are fixed, rather than just one in a file. It also ensures that in the (hopefully unlikely) event that there are both intentional and unintentional use-cases within a file, each one gets checked.
I think it would make more sense to add it at each individual call site. This ensures that all cases are fixed, rather than just one in a file. It also ensures that in the (hopefully unlikely) event that there are both intentional and unintentional use-cases within a file, each one gets checked.
That would be better indeed, but may be trickier to automate - the challenge, in my mind, comes from multi-line RUN statements, but maybe I’m missing something / maybe there aren’t that many. If folks have suggestions, please don’t hesitate to throw them my way!
Thanks!
I recently discovered that multi-line RUN statements can actually be interrupted with non-RUN lines, without changing the behaviour. In other words, you can do something like:
RUN: some command --option1 \
Comment
CHECK: check something
RUN: --option2
And you’d end up with “some command --option1 --option2” being run. It’s rather surprising behaviour, and not one I’d generally recommend exercising, but maybe in this context it would be okay?
Oh! Perfect - thanks!
(plus, if it becomes unsupported, there’s another nudge to fix )
There’s a wrinkle in this: some tests (clang ones, for instance) have output checks depending on the line position of the input. For example, they check debug info. Adding // FIXME: comments shift that.
If the goal is easy identification of auto-inserted -allow-unused-prefixes directives, how about:
- we make the flag an enum: true, false, and auto_inserted
- we use -allow-unused-prefixes=auto_inserted for the scripts
This allows easy identification, and should be easier to script without requiring more significant test by test surgery.
WDYT?
or - a variation - adding a --script-inserted-allow-unused-prefixes - to still allow stuff like --allow-unused-prefixes (meaning “true”).