[llvm-dev] [RFC] Improving FileCheck

[Jon Roelofs via llvm-dev]

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).

1: https://reviews.llvm.org/D77227

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.

CHECK:            legitimate test
CHECK             gotcha A
CHECK :           gotcha B
CHECKNEXT:        gotcha C
CHECKDAG:         gotcha D
CHECK_NOT:        gotcha E
CHECK-LABEL-NOT:  ??? F
CHECK-SAME-DAG:   ??? G


Gotcha A
--------

CHECK  gotcha A

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.

In the review thread, a couple of issues surfaced with simply matching on
`${CHECKNAME}\b`, making that less attractive as a remedy:

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.

A2) There are a few RUN lines with missing colons, though those seem
infrequent enough to not worry about [5].

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.

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.

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].

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]:

> Consider this example that has a well formed directive that doesn't
follow the rule:
>
> // FIXME(201806L) CHECK: assert: 0
> 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.
>
> 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).
>
> 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 :
2: llvm/test/Transforms/InstCombine/phi-preserve-ir-flags.ll
3: https://reviews.llvm.org/D77227#1955596
4:
https://github.com/llvm/llvm-project/blob/56decd982dc03a74d1796d9d4dbd7d9e0cea98dc/llvm/lib/Support/FileCheck.cpp#L1141
5: llvm/test/CodeGen/AArch64/speculation-hardening.ll
6: llvm/test/MC/ARM/dwarf-asm-multiple-sections.s:88
7: https://reviews.llvm.org/differential/diff/254562
8: https://reviews.llvm.org/D77227#1958228


Gotcha B
--------

CHECK :  gotcha B

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).

9: https://reviews.llvm.org/D77227#1959041


Gotchas C, D, E
---------------

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.


??? F, G
--------

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).


-- 
Jon Roelofs
jroelofs at jroelofs.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200403/a025677c/attachment.html>