<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 3 Apr 2020 at 19:59, Robinson, Paul via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div lang="EN-US">
<div class="gmail-m_7423310893885913962WordSection1">
<p class="MsoNormal">Gotcha B (space between the directive and the colon): Some tests have this bug, so it would be worth catching.<u></u><u></u></p>
<p class="MsoNormal">James Henderson observed that legalizing it could help prettify some cases where we’re matching whitespace or the entire line. I don’t think it’s that valuable personally.<u></u><u></u></p>
<p class="MsoNormal">If we implement a reasonable diagnostic heuristic for the missing-colon case (Gotcha A), then we’ll catch this mistake in the same net.<u></u><u></u></p>
<p class="MsoNormal"><u></u></p></div></div></blockquote><div><br></div><div>Fine by me, as long as "# CHECK:" continues to be acceptable instead to allow the colons to line up with other check directives on other lines.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div lang="EN-US"><div class="gmail-m_7423310893885913962WordSection1"><p class="MsoNormal"> <u></u></p>
<p class="MsoNormal">Gotchas C,D (missing hyphen): I found exactly one case in the wild. I’d say the value is debatable.<u></u><u></u></p>
<p class="MsoNormal">(It’s a CHECKNEXT in llvm/test/CodeGen/PowerPC/testCompareslleqsi.ll if someone wants to fix it.)</p></div></div></blockquote><div><br></div><div>I'm not keen on this becoming something that is forbidden, since I could imagine myself running into this on occasion, particularly when using custom check prefixes shared between cases - it's not unusual for me to name such prefixes as something like "FOO", "BAR" and "FOOBAR", where FOO is for case 1, BAR is for case2 and FOOBAR is used by both. If BAR happened to be a different name that clashes with a known suffix (e.g. LABEL), then I'd get an unwanted false positive.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div lang="EN-US"><div class="gmail-m_7423310893885913962WordSection1"><p class="MsoNormal"><u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">Gotcha E (underscore instead of hyphen): I found 40 examples across clang/test and llvm/test. I am certain I have caught a few cases in review and pretty sure I’ve had to fix some of these that I typo’d myself. I’d say this is worth
doing.<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">Multiple suffixes: I believe there are NO multiple-suffix combinations that FileCheck currently supports. The tool should detect any multiple suffix combinations and report them as errors. Currently it looks for a limited set (basically,
-NOT in combination with almost anything else), but it’s easy for someone to infer that if FileCheck doesn’t complain, then it will Do The Right Thing™ with other combinations. We should not be that user-unfriendly; we should complain about all multiple-suffix
combinations.</p></div></div></blockquote><div><br></div><div>I agree that checking for them all seems reasonable, though perhaps it might be nice for this to be user-configurable somehow (I'm thinking FileCheck tests themselves might run into things confusingly). It seems to me that it would be easy to auto-generate the combinatorial list in code and then do a simple check for the prefix being in the list.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div lang="EN-US"><div class="gmail-m_7423310893885913962WordSection1"><p class="MsoNormal"><u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">--paulr<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<div style="border-color:currentcolor currentcolor currentcolor blue;border-style:none none none solid;border-width:medium medium medium 1.5pt;padding:0in 0in 0in 4pt">
<div>
<div style="border-color:rgb(225,225,225) currentcolor currentcolor;border-style:solid none none;border-width:1pt medium medium;padding:3pt 0in 0in">
<p class="MsoNormal"><b>From:</b> llvm-dev <<a href="mailto:llvm-dev-bounces@lists.llvm.org" target="_blank">llvm-dev-bounces@lists.llvm.org</a>> <b>On Behalf Of
</b>Jon Roelofs via llvm-dev<br>
<b>Sent:</b> Friday, April 3, 2020 12:58 PM<br>
<b>To:</b> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<b>Subject:</b> [llvm-dev] [RFC] Improving FileCheck<u></u><u></u></p>
</div>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal">I'd like to (re)start a discussion on a few gotchas in FileCheck that I've noticed working on various tests in llvm's suites. This begain in a review [1], but I'll try to summarize here so it gets the right audience before decisions are
made on it (so to speak).<br>
<br>
1: <a href="https://reviews.llvm.org/D77227" target="_blank">https://reviews.llvm.org/D77227</a><br>
<br>
The main sticking point is the abundance of checks in FileCheck tests that appear to be checking something, but are in fact silently hiding failures. The biggest class of this bug appears to be CHECK lines that omit the trailing colon, though there are a few
others.<br>
<br>
CHECK: legitimate test<br>
CHECK gotcha A<br>
CHECK : gotcha B<br>
CHECKNEXT: gotcha C<br>
CHECKDAG: gotcha D<br>
CHECK_NOT: gotcha E<br>
CHECK-LABEL-NOT: ??? F<br>
CHECK-SAME-DAG: ??? G<br>
<br>
<br>
Gotcha A<br>
--------<br>
<br>
CHECK gotcha A<br>
<br>
A lot of cases of (A) are benign, but buried in there are cases where we have tests that don't check what they intend to, which are broken when the missing colons are added [2]. Some grep analysis from paulr in [3] found some 178 tests across 72 test files
that seem like likely mistakes, suggesting that having some automated tooling to catch this is probably not a bad idea.<br>
<br>
In the review thread, a couple of issues surfaced with simply matching on `${CHECKNAME}\b`, making that less attractive as a remedy:<br>
<br>
A1) There are quite a lot of RUN: lines that have CHECK names on them from their --check-prefix/--check-prefixes arguments, and we don't want tooling to match on those. This could be addressed with a script that quotes them all, but that would mean touching
pretty much every test file, which is less than ideal.<br>
<br>
A2) There are a few RUN lines with missing colons, though those seem infrequent enough to not worry about [5].<br>
<br>
A3) There are quite a lot of mentions of CHECK names in comments that are clearly not meant to be tests [6]. Any solution to this, as far as I can tell, will likely need to reword many of those.<br>
<br>
A4) We need some way to comment out CHECK lines that conveys intent better than removing the colon. This appears to be intentional in some testcases, but unintentional in the vast majority of them.<br>
<br>
To address (A1), a number of rules were proposed in [1], the best of which seems to be that we look for lines matching `[#/;*!]\s*CHECK[ \t]`, and emit a diagnostic of some form to help correct it. This gave a pretty good false positive rate of 25% on the 186
tests it "broke" [7].<br>
<br>
An open question here from jdenny is whether it makes sense to require all checks to follow that pattern (with the colon, of course) to make things less user-hostile [8]:
<br>
<br>
> Consider this example that has a well formed directive that doesn't follow the rule:<br>
> <br>
> // FIXME(201806L) CHECK: assert: 0<br>
> Approach A (from a previous comment): FileCheck executes the directive. If the user later accidentally removes the :, FileCheck won't execute the directive and won't diagnose the error unless the user is wiling to endure false positives by opting into the
more verbose mode Paul suggested.<br>
> <br>
> Approach B (from that some comment): FileCheck ignores the directive. That just makes things worse because the above otherwise well formed directive is then an undiagnosed malformed directive (unless the user opts into a more verbose mode).<br>
> <br>
> Approach C (new proposal): FileCheck reports the directive as an error (in any mode). The more verbose mode is still needed to catch the case that the : is missing here, but at least users are guaranteed to get a slap when they write them with :<br>
2: llvm/test/Transforms/InstCombine/phi-preserve-ir-flags.ll<br>
3: <a href="https://reviews.llvm.org/D77227#1955596" target="_blank">https://reviews.llvm.org/D77227#1955596</a><br>
4: <a href="https://github.com/llvm/llvm-project/blob/56decd982dc03a74d1796d9d4dbd7d9e0cea98dc/llvm/lib/Support/FileCheck.cpp#L1141" target="_blank">
https://github.com/llvm/llvm-project/blob/56decd982dc03a74d1796d9d4dbd7d9e0cea98dc/llvm/lib/Support/FileCheck.cpp#L1141</a><br>
5: llvm/test/CodeGen/AArch64/speculation-hardening.ll<br>
6: llvm/test/MC/ARM/dwarf-asm-multiple-sections.s:88<br>
7: <a href="https://reviews.llvm.org/differential/diff/254562" target="_blank">https://reviews.llvm.org/differential/diff/254562</a><br>
8: <a href="https://reviews.llvm.org/D77227#1958228" target="_blank">https://reviews.llvm.org/D77227#1958228</a><br>
<br>
<br>
Gotcha B<br>
--------<br>
<br>
CHECK : gotcha B<br>
<br>
This pattern is a variant of (A) that also disables perfectly good tests, but in a way that isn't obvious that it doesn't work. jhenderson brings up some good points [9] in favor of extending FileCheck to make FileCheck do what the user intended here. Luckily,
that doesn't seem to conflict with the rules proposed in (A).<br>
<br>
9: <a href="https://reviews.llvm.org/D77227#1959041" target="_blank">https://reviews.llvm.org/D77227#1959041</a><br>
<br>
<br>
Gotchas C, D, E<br>
---------------<br>
<br>
I believe these can be handled pretty simply in FileCheck itself, but I have not spent much time trying to estimate how many tests are affected by this class of bug.<br>
<br>
<br>
??? F, G<br>
--------<br>
<br>
There are a number of check suffix combinations that are not explicitly supported (in the docs), but appear (maybe) useful. For these, there is some precedent on mitigating them within FileCheck itself [4], though the combinatorial explosion warrants being
careful about how we go about detecting them (if at all).<br>
<br>
<br>
-- <br>
Jon Roelofs<br>
<a href="mailto:jroelofs@jroelofs.com" target="_blank">jroelofs@jroelofs.com</a><u></u><u></u></p>
</div>
</div>
</div>
</div>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div></div>