[FileCheck] Warn if a prefix is only used in LABEL checks

[SjoerdMeijer]

Summary

I would like FileCheck to warn for cases like this:

    
RUN: llc -mtriple=arm-linux-gnueabi %s -o - | FileCheck %s -check-prefix=FOO
define void @FOO() {
entry:
; FOO-LABEL: FOO:
; DOO:       mov  pc, lr
  ret void
}

Here, prefix FOO is only used in a label check FOO-LABEL. This can easily
happen when a typo is made in the actual checks: here accidentally DOO is
checked instead of FOO. The problem is that this goes unnoticed, as
FileCheck is not producing a diagnostic for this.

I would like to change this behaviour, and with this patch FileCheck will now
warn like this:

foo.ll:5:18: note: Prefix FOO only occurs in a LABEL check, this is probably not what you want
; FOO-LABEL: FOO:
                 ^

[SjoerdMeijer]

I probably need to add a regression test for this, but wanted to check this idea with you first.

[jdenny]

Have you run existing test suites to see how many warnings you get? That would help to determine if there are use cases we haven't thought of. If there are no use cases, we might consider making this an error.

lib/Support/FileCheck.cpp

851

Notes usually follow other diagnostics. Shouldn't this be a warning?

[jdenny]

lib/Support/FileCheck.cpp

852

This implies that your check is per prefix. FileCheck can be run with multiple prefixes at the same time, and I don't think there should be a warning unless none of the prefixes have non-label directives.

[SjoerdMeijer]

Thanks for looking at this! Answering this one first:

Have you run existing test suites to see how many warnings you get?

I just did, and it came back with some interesting results. For example test/CodeGen/ARM/Windows/mangling.ll:

; RUN: llc -mtriple=thumbv7-windows -mcpu=cortex-a9 -o - %s | FileCheck %s
define void @function() nounwind {
entry:
  ret void
}
; CHECK-LABEL: function

This shows that just checking the label is actually a use-case, which is why I thought a warning would be better; looks like we can't promote it to an error (which is okay I think).

Then because of this warning, I found:

• one of our own recently added test cases test/CodeGen/ARM/arm-cgp-calls.ll that was horribly broken in different ways; but it mainly wasn't checking anything because all label prefixes where wrong. I put up the fix for review in D53746.
• another broken test case test/CodeGen/ARM/inlineasm-X-allocation.ll, for which I put up a fix for review here D53748.
• Dubious test cases that only checks labels, but really should test other things too:
  • test/CodeGen/ARM/2011-11-07-PromoteVectorLoadStore.ll,
  • test/CodeGen/ARM/2011-11-09-BitcastVectorDouble.ll,
  • test/CodeGen/ARM/fast-isel-update-valuemap-for-extract.ll
  • test/CodeGen/ARM/thumb2-size-reduction-internal-flags.ll

I think this shows the benefit of adding this to FileCheck, and I've only looked at the tests in the CodeGen/ARM directory.

[jdenny]

In D53710#1277094, @SjoerdMeijer wrote:

Thanks for looking at this! Answering this one first:

Have you run existing test suites to see how many warnings you get?

I just did, and it came back with some interesting results. For example test/CodeGen/ARM/Windows/mangling.ll:

; RUN: llc -mtriple=thumbv7-windows -mcpu=cortex-a9 -o - %s | FileCheck %s
define void @function() nounwind {
entry:
  ret void
}
; CHECK-LABEL: function

This shows that just checking the label is actually a use-case, which is why I thought a warning would be better; looks like we can't promote it to an error (which is okay I think).

Why isn't that just CHECK? Is that a legitimate use case?

Then because of this warning, I found:

• one of our own recently added test cases test/CodeGen/ARM/arm-cgp-calls.ll that was horribly broken in different ways; but it mainly wasn't checking anything because all label prefixes where wrong. I put up the fix for review in D53746.
• another broken test case test/CodeGen/ARM/inlineasm-X-allocation.ll, for which I put up a fix for review here D53748.
• Dubious test cases that only checks labels, but really should test other things too:
  • test/CodeGen/ARM/2011-11-07-PromoteVectorLoadStore.ll,
  • test/CodeGen/ARM/2011-11-09-BitcastVectorDouble.ll,
  • test/CodeGen/ARM/fast-isel-update-valuemap-for-extract.ll
  • test/CodeGen/ARM/thumb2-size-reduction-internal-flags.ll

I think this shows the benefit of adding this to FileCheck, and I've only looked at the tests in the CodeGen/ARM directory.

Nice.

[probinson]

In D53710#1277353, @jdenny wrote:

In D53710#1277094, @SjoerdMeijer wrote:

Thanks for looking at this! Answering this one first:

Have you run existing test suites to see how many warnings you get?

I just did, and it came back with some interesting results. For example test/CodeGen/ARM/Windows/mangling.ll:

; RUN: llc -mtriple=thumbv7-windows -mcpu=cortex-a9 -o - %s | FileCheck %s
define void @function() nounwind {
entry:
  ret void
}
; CHECK-LABEL: function

This shows that just checking the label is actually a use-case, which is why I thought a warning would be better; looks like we can't promote it to an error (which is okay I think).

Why isn't that just CHECK? Is that a legitimate use case?

CHECK-LABEL will complain if it doesn't find a matching string, so the test would fail if the label text does not appear.
However, it's not really a proper use of LABEL, and it would be better to have this test just use CHECK.

[jdenny]

In D53710#1277482, @probinson wrote:

In D53710#1277353, @jdenny wrote:

Why isn't that just CHECK? Is that a legitimate use case?

CHECK-LABEL will complain if it doesn't find a matching string, so the test would fail if the label text does not appear.
However, it's not really a proper use of LABEL, and it would be better to have this test just use CHECK.

Make sense.

The only legitimate LABEL-only use case I've imagined so far involves a test case with multiple FileCheck calls with different combinations of FileCheck prefixes. Some of those combinations might combine LABEL and non-LABEL directives while other combinations have only LABEL directives, which then serve basically as CHECK directives. However, I don't know that this use case actually exists in practice.

[SjoerdMeijer]

• Instead of a Dk_Note, it is a DK_Warning now.
• Added test cases; hopefully demonstrating that when we have a prefix list, and one of them is used, we don't issue the warning.

[SjoerdMeijer]

Friendly ping.

Does this look somewhat reasonable, and do what we want?

[jdenny]

In D53710#1288598, @SjoerdMeijer wrote:

Friendly ping.

Does this look somewhat reasonable, and do what we want?

Hi Sjoerd. Thanks for the changes you made.

The warning is still misleading when there are multiple prefixes. See my earlier inline comment. The easiest solution is probably to reword so that it never mentions a specific prefix.

I know you ran test/CodeGen/ARM, but did you try any other test suites? This is only a warning, but changes to FileCheck have impact throughout LLVM, so it would nice to get a better preview of what others will see before pushing.

[SjoerdMeijer]

Hi Joel, thanks for taking another look!

The warning is still misleading when there are multiple prefixes. See my earlier inline comment. The easiest solution is probably to reword so that it never mentions a specific prefix.

Ah, missed that I think.

I know you ran test/CodeGen/ARM, but did you try any other test suites? This is only a warning, but changes to FileCheck have impact throughout LLVM, so it would nice to get a better preview of what others will see before pushing.

Good point. Will do my homework and get back soon with an update.

[arsenm]

AMDGPU does this a lot intentionally for a shared label prefix and unique per run like ones for different subtargets

[SjoerdMeijer]

AMDGPU does this a lot intentionally for a shared label prefix and unique per run like ones for different subtargets

Yep. Briefly looked at AMDGPU warnings. The very first 4 warnings, in tests basic-loop.ll, basic-call-return.ll, bug-vopc-commute.ll, call-return-types.ll, shows I think that they are dodgy tests, but then I see indeed the intentional use of the shared label prefix.

While these are only warnings, which you normally don't see when you run lit, I agree that they are false positives which ideally we shouldn't have.

I am not sure what the solution is yet, i.e. I am not sure (haven't looked at it) if I can distinguish the intentional uses from the dodgy uses. Or thinking out loud: perhaps we should say that -check-prefix should really check a prefix, and we introduce -check-label to check shared labels...

[probinson]

I think what we've determined so far is that some uses of CHECK-LABEL simply misunderstand the LABEL suffix, which IMO is because the suffix is very poorly named. Renaming it to something harder to misunderstand would be a lot of churn but ultimately benefit the project's tests. The process of updating all tests with LABEL would necessarily find all the dodgy cases. And with a better-understood FileCheck feature, ultimately I think there would be fewer cases of bad tests.
(It would also help if people responsibly made sure their tests failed when their patch was NOT applied, but we have no process for this.)

Renaming CHECK-LABEL to, say, CHECK-BOUND is way more work than you signed up for, but I think it's preferable than piling more options and warnings on top of the existing feature. I'd support an RFC to do this and help with some of the mechanical work.

[jdenny]

In D53710#1289937, @SjoerdMeijer wrote:

AMDGPU does this a lot intentionally for a shared label prefix and unique per run like ones for different subtargets

Yep. Briefly looked at AMDGPU warnings. The very first 4 warnings, in tests basic-loop.ll, basic-call-return.ll, bug-vopc-commute.ll, call-return-types.ll, shows I think that they are dodgy tests, but then I see indeed the intentional use of the shared label prefix.

You two seem to be saying that the concept of a "shared label prefix" justifies this use case. I'm sorry, but I don't know what that phrase means. Could someone please explain? In other words, couldn't these tests just use "CHECK:" (or "GCN:" or whatever prefix they prefer) without "-LABEL"? I believe that's Paul's position.

In D53710#1290271, @probinson wrote:

Renaming CHECK-LABEL to, say, CHECK-BOUND is way more work than you signed up for, but I think it's preferable than piling more options and warnings on top of the existing feature. I'd support an RFC to do this and help with some of the mechanical work.

Renaming sounds like it could be helpful if the community agrees on the new name, but let's not forget the original purpose of this warning: to identify prefix typos. It just so happens that this warning also finds misuses of CHECK-LABEL. If those misuses can be eliminated through the renaming process you describe, it would be interesting to see how many misspellings this warning then finds.

[probinson]

In D53710#1290345, @jdenny wrote:

In D53710#1289937, @SjoerdMeijer wrote:

AMDGPU does this a lot intentionally for a shared label prefix and unique per run like ones for different subtargets

Yep. Briefly looked at AMDGPU warnings. The very first 4 warnings, in tests basic-loop.ll, basic-call-return.ll, bug-vopc-commute.ll, call-return-types.ll, shows I think that they are dodgy tests, but then I see indeed the intentional use of the shared label prefix.

You two seem to be saying that the concept of a "shared label prefix" justifies this use case. I'm sorry, but I don't know what that phrase means. Could someone please explain? In other words, couldn't these tests just use "CHECK:" (or "GCN:" or whatever prefix they prefer) without "-LABEL"? I believe that's Paul's position.

I think a concrete example would help, and it looks like CodeGen/AMDGPU/cttz_zero_undef.ll is one. In this test, there are three RUN lines, each for a different subtarget, and the check-prefix options include FUNC and also some subtarget-specific prefixes. Then you have checks like

; FUNC-LABEL: {{^}}s_cttz_zero_undef_i32:
; SI: something
; EG: some other thing
; FUNC-LABEL: {{^}}v_cttz_zero_undef_i32:
; SI: thing3
; EG: thing4

So the FUNC prefix is used just for the LABELs, and other prefixes are used for target-specific variations within each test function.

This seems like a reasonable use-case to me.

[jdenny]

In D53710#1290508, @probinson wrote:

This seems like a reasonable use-case to me.

Agreed, but this test case shouldn't produce the warning because there's always an active prefix that has a non-LABEL directive. Or did I miss something?

I thought the point of the "shared label prefix" use case was to show a case of false positives. However, the examples that @SjoerdMeijer cited should produce the warning because they only use LABEL directives and thus are not legitimate use cases of LABEL.

[SjoerdMeijer]

Hi Paul and Joel, thanks for your help here, appreciate it! I thought this was
a nice little addition, low-hanging fruit, but it looks like it turns into
proper project! :-) Also from our discussions, it looks like we have not just
one but a few problems. If I try to summarise them:

1. FileCheck: it is lacking diagnostics, not revealing all sorts of problems in tests. This is a very serious issue, because with this patch it took no time to find a long list of test issue, severe and minor, see next point.
2. Test issues: perhaps we can classify tests in this way, we have:
   1. Broken tests: they don't do or test anything because the non-LABEL prefixes used in tests are "undefined": they do not occur on the FileCheck and --check-prefix command line. This can easily happen by making a typo, or refactoring of the tests, etc.
   2. Dodgy tests: looks like we have "compile-only tests". They do check the given --check-prefix, but only the CHECK-LABEL variant and not the non-LABEL prefixes.
   3. Reasonable tests: multiple RUN lines, each with multiple --check-prefixes: one prefix is used just for LABELs ("shared label prefix"), and other prefixes to test variations within each test functions.

I think the objective is to produce a diagnostic for the broken and dodgy
tests, but (obviously) don't produce false positives for the reasonable tests.

In D53710#1290508, @probinson wrote:
This seems like a reasonable use-case to me.

Agreed, but this test case shouldn't produce the warning because there's always an active prefix that has a non-LABEL directive. Or did I miss something?

I think/hope we were talking about the same things. Paul's previous example is a
reasonable use-case, and we shouldn't produce a warning. A more minimal example, a
test that should be added to this patch, is this one:

// RUN: FileCheck -vv -check-prefix=SHARED -check-prefix=FOO -input-file %s %s 2>&1 | FileCheck -check-prefix=DONTWARN %s
// RUN: FileCheck -vv -check-prefix=SHARED -check-prefix=BAR -input-file %s %s 2>&1 | FileCheck -check-prefix=DONTWARN %s
; This is a very normal test, so we shouldn't complain about it:
;
; DONTWARN-NOT: warning: {{.*}} only occurs in a LABEL check, this is probably not what you want.
foo
bar
; SHARED-LABEL: foo
; FOO:          bar

But this test will fail. This is (another) example of a false positive, and that's
what we were talking about I think.

About how to continue:

I think the issues are too serious to ignore, so we should progress this somehow.
A RFC and a proposal sounds like the right approach indeed. I don't mind starting
with the RFC for example. But I will be doing this and the follow up work on the side
(I do need to progress other things as well), so I think I can definitely use some
help. Based on this wall of text (this comment), but of course properly introduced
and with some examples, I can have a RFC ready mid/end of next week. But if you
want to do this, if you e.g. have more bandwidth, can or would like to progress this
faster, etc., please go ahead! Just let me know...

[jdenny]

In D53710#1291486, @SjoerdMeijer wrote:

Hi Paul and Joel, thanks for your help here, appreciate it! I thought this was
a nice little addition, low-hanging fruit, but it looks like it turns into
proper project! :-)

No problem. However all this turns out, thank you for contributing!

Also from our discussions, it looks like we have not just
one but a few problems. If I try to summarise them:

Thanks for that summary, which helps me to better see where we stand.

1. FileCheck: it is lacking diagnostics, not revealing all sorts of problems in tests. This is a very serious issue, because with this patch it took no time to find a long list of test issue, severe and minor, see next point.
2. Test issues: perhaps we can classify tests in this way, we have:
   1. Broken tests: they don't do or test anything because the non-LABEL prefixes used in tests are "undefined": they do not occur on the FileCheck and --check-prefix command line. This can easily happen by making a typo, or refactoring of the tests, etc.

A nit on your wording: The "broken tests" do test something: the LABELs. They just don't test everything they were intended to.

1. Dodgy tests: looks like we have "compile-only tests". They do check the given --check-prefix, but only the CHECK-LABEL variant and not the non-LABEL prefixes.

I'm not sure what you mean by "compile-only" here. I think you're saying that "dodgy tests" are like "broken tests" with one exception: the author is intentionally testing only LABEL directives, and that's a dodgy use case because plain CHECK directives would work.

Reasonable tests: multiple RUN lines, each with multiple --check-prefixes: one

prefix is used just for LABELs ("shared label prefix"), and other prefixes to
test variations within each test functions.

I think the objective is to produce a diagnostic for the broken and dodgy
tests, but (obviously) don't produce false positives for the reasonable tests.

That was my interpretation as well. But it's the "dodgy tests" where it seems we're still not quite connecting. See below.

In D53710#1290508, @probinson wrote:
This seems like a reasonable use-case to me.

Agreed, but this test case shouldn't produce the warning because there's always an active prefix that has a non-LABEL directive. Or did I miss something?

I think/hope we were talking about the same things. Paul's previous example is a
reasonable use-case, and we shouldn't produce a warning. A more minimal example, a
test that should be added to this patch, is this one:

// RUN: FileCheck -vv -check-prefix=SHARED -check-prefix=FOO -input-file %s %s 2>&1 | FileCheck -check-prefix=DONTWARN %s
// RUN: FileCheck -vv -check-prefix=SHARED -check-prefix=BAR -input-file %s %s 2>&1 | FileCheck -check-prefix=DONTWARN %s
; This is a very normal test, so we shouldn't complain about it:
;
; DONTWARN-NOT: warning: {{.*}} only occurs in a LABEL check, this is probably not what you want.
foo
bar
; SHARED-LABEL: foo
; FOO:          bar

But this test will fail.

Agreed.

This is (another) example of a false positive, and that's
what we were talking about I think.

Why? The second RUN command's first FileCheck call almost surely demonstrates a "broken test" because the BAR prefix is specified but never appears in the file, leaving only LABEL directives. At best, if it was somehow intentional that BAR was specified but never used in a directive , this FileCheck call represents a "dodgy test". You said above that "dodgy tests" and "broken tests" should have diagnostics and are not false positives.

Below is another potential use case I tried to describe many comments ago, but I never gave an example:

// RUN: testprog 0 | FileCheck -check-prefix=SHARED %s
// RUN: testprog 1 | FileCheck -check-prefix=SHARED -check-prefix=CODE1 %s
// RUN: testprog 2 | FileCheck -check-prefix=SHARED -check-prefix=CODE2 %s

; SHARED-LABEL: fnA
; CODE1:  body1
; CODE2:  body2

; SHARED-LABEL: fnB
; CODE1:  body1
; CODE2:  body2

The program under test is testprog. The second and third RUN commands call testprog with different arguments producing different code within each label. These RUN commands represent "reasonable tests", right?

However, the first RUN command calls testprog with an argument that produces the labels, but it doesn't produce any test-worthy code within each label. This RUN command represents a "dodgy test", right? But, in the context of the full test program, it's perfectly reasonable. That is, for the first RUN command only, we could have used "SHARED:" and not "SHARED-LABEL:", but we don't do that because we want to use "SHARED-LABEL:" for the other RUN commands.

So this is a hybrid of a "reasonable test" and a "dodgy test", and the result is something that seems reasonable. To be clear, I have not yet seen an example of this hybrid in any real test (including the tests discussed in this thread), and its possibility is one of the reasons I requested that more test suites be run with this patch.

The more I think about all this, the more doubtful I am about the direction we're heading. The "dodgy tests" are often not really wrong because they are testing what the author intended. They just represent an odd use of CHECK-LABEL because CHECK would be sufficient... except in the hybrid scenario I described. So the "dodgy tests" are potentially wrong only in certain cases... and possibly only if you're squinting your eyes really hard because you really want this warning not to have false positives. :-) I think that's what I've been doing until now.

The rename of LABEL to BOUND seems like a lot of work just to avoid these "dodgy tests" that might not be that dodgy after all. Does it help to avoid some other case we haven't discussed yet?

I agree it's easy to make mistakes with FileCheck, and I too would like to find ways to avoid those. I'm just not convinced this direction is going to prove fruitful, but please tell me if I'm not seeing this clearly.

[probinson]

Because there is nothing to indicate that a line contains a FileCheck directive, other than that it pattern-matches a user-specified string with an optional suffix, we can't reliably report all typos. So for example:

CHECK-LABEL: A
CHECK: B
CHEKC: C

the third line can't be diagnosed by FileCheck under any reasonable heuristic. So, anything we do is a best-effort, and ideally produces no false positives by default.

Staying out of the 'dodgy' and 'broken' terminology debate, it seems that a heuristic of "prefix occurs only with LABEL suffix" will have too many false positives (or at least, marginal positives, for tests that do behave correctly even if they don't use LABEL the way it was intended). So, if we proceed with this heuristic, it needs to be under an off-by-default command line option. (You can set the option using Joel's new environment variable, and look at particular tests or sets of tests, at your leisure. If we get to a point where the option provides a clean test run for all components and all targets, then we can make it on-by-default.)

Another useful diagnostic (probably best done separately) would be when a given prefix triggers no directives at all. That's obviously an incorrect test; either it has typos or has a dangling -check-prefix whose directives have all been removed. Right now, this diagnostic occurs only when there are no matching directives at all. But if you specify two prefixes, and only one of them matches a directive, FileCheck is silent. I think that's bad.

Renaming LABEL to BOUND is probably a pipe dream, let's take that off the table at least for now. Improved understanding of the suffix would help in general, but it's a bit tangential to the current goal.

To summarize, then:

1. Put the current diagnostic under an off-by-default option.
2. In a follow-up patch, emit a diagnostic for any prefix that has no matches (even if other prefixes do have matches). This might require fixing some tests, or it will need to be off-by-default while we do test cleanup.
3. Ignore the RFC and rename LABEL to BOUND ideas.

[jdenny]

In D53710#1291939, @probinson wrote:

we can't reliably report all typos.

Agreed.

Staying out of the 'dodgy' and 'broken' terminology debate,

:-) Sorry for that. My goal really was just to be sure we were discussing the same thing.

To summarize, then:

1. Put the current diagnostic under an off-by-default option.
2. In a follow-up patch, emit a diagnostic for any prefix that has no matches (even if other prefixes do have matches). This might require fixing some tests, or it will need to be off-by-default while we do test cleanup.
3. Ignore the RFC and rename LABEL to BOUND ideas.

I think this plan makes a lot of sense.

Should off-by-default diagnostics be errors instead of warnings? Errors are harder to miss, and, if they're off-by-default, they're probably harmless. Alternatively, we could start developing flags like -Wfoo, -Wbar, -Wall, -Werror, etc.

[SjoerdMeijer]

Agreed with all previous comments! And yes, I messed up my last lit test example :-( and I looked at yours and Paul's, and agree. But anyway, we have a plan...

But perhaps one more quick discussion on this:

1. Put the current diagnostic under an off-by-default option

and:

Should off-by-default diagnostics be errors instead of warnings?

What do you think about this: we don't put it under an option, and kind of leave it as it is? This means:

• you don't see warnings (including this one) when you e.g. do a ninja check
• you don't see warnings when you do e.g. llvm-lit on a test.
• I think you'll only see it when you enable llvm-lit -v (or llvm -a which I always use when I develop new tests).

Thus, I think it is already under and option, which is not enabled by default. I appreciate I might be asking the same question again, but just wanted to double check what you think. The reason I am asking, is that I am afraid that when we put this under a new, separate option (if I understood correctly), people won't use it, and there's perhaps little benefit of having it?

[jdenny]

In D53710#1292880, @SjoerdMeijer wrote:

Agreed with all previous comments! And yes, I messed up my last lit test example :-( and I looked at yours and Paul's, and agree. But anyway, we have a plan...

No problem. It looks like we're converging.

Thus, I think it is already under and option, which is not enabled by default.

It's enabled by default at least for anyone who runs lit with -v or -a. I don't know how noisy that could be, but running with more of LLVM's test suites would help to determine that.

The reason I am asking, is that I am afraid that when we put this under a new, separate option (if I understood correctly), people won't use it, and there's perhaps little benefit of having it?

Maybe so, but starting with it off by default gives everyone a chance to try it out before deciding it should be on by default. That is, a follow-up patch could change the default to on.

We all seem to agree it's easy to make mistakes with FileCheck, and Paul has already suggested a second possible warning, which I also think is valuable. A -W infrastructure would make it easy for us to experiment with new warnings without bothering people who don't want the noise. -W[no]-foo lets people choose the opposite of the default, and -Werror makes it hard to miss warnings people value.

[jdenny]

Of course, let's see what Paul thinks before proceeding.

[probinson]

I hadn't thought about -v; I don't think the new diag is currently under -v? The first test isn't using that option...

I think we should encourage people to have warnings on when they develop their tests. Having that separate from -v seems like a good thing, as -v obviously dumps a lot of info, most of which would ordinarily describe a test behaving exactly as intended. A -warn option to turn everything on/off (default off) should be sufficient, at least for now when the number of different warnings is pretty small. More fine-grained -W options seems like overkill. These diags are mainly to help people write cleaner tests. Oh, we should probably add a note to the docs somewhere, advising people to set -warn in the environment variable while writing new tests.

[jdenny]

In D53710#1293198, @probinson wrote:

I hadn't thought about -v; I don't think the new diag is currently under -v? The first test isn't using that option...

I think we should encourage people to have warnings on when they develop their tests. Having that separate from -v seems like a good thing, as -v obviously dumps a lot of info, most of which would ordinarily describe a test behaving exactly as intended.

I believe you're thinking of FileCheck's -v. Sjoerd was describing lit's -v.

A -warn option to turn everything on/off (default off) should be sufficient, at least for now when the number of different warnings is pretty small. More fine-grained -W options seems like overkill.

OK. I was imagining that building that infrastructure now would encourage more warning explorations in the future, and that could benefit FileCheck. But we can always wait to see if people come up with more warnings.

[probinson]

Okay, clearly I have not been paying attention. It's not that lit -v enables the warning, it is simply not suppressing the message. This is why lit without -v appears to turn it "off." I get it now. As far as FileCheck itself is concerned, this is always-on.
Have you tried running the entire suite with lit -v to see how often the diagnostic comes up? Now that you don't emit a diagnostic for the multiple-prefix case.

test/FileCheck/check-prefixes-only-labels.txt

4 ↗

(On Diff #171475)

FileCheck's own tests generally use unnecessary regexes in the check lines, so that that check lines don't match themselves. That is, this line would want to be something like this:

FOO-LABEL: {{f}}oo

Now in this particular test, it doesn't really matter, but I think for consistency across FileCheck tests I would prefer to see the braces.

Note this comment applies to all the tests.

[SjoerdMeijer]

Sorry, just a quick message: I'm a few days out of office, and will get back to this end of this week.

[SjoerdMeijer]

I'm back :-) Catching up on the last messages.... I will post a patch for review soon that adds an option -warn to FileCheck, and will do some initial plumbing for that. If I understood correctly, it looks like we think that is best. After that initial plumbing, I will then return to this patch. Is that a plan?

[jdenny]

In D53710#1304102, @SjoerdMeijer wrote:

I'm back :-) Catching up on the last messages.... I will post a patch for review soon that adds an option -warn to FileCheck, and will do some initial plumbing for that. If I understood correctly, it looks like we think that is best. After that initial plumbing, I will then return to this patch. Is that a plan?

Seems fine to me.

[thopre]

Sorry to react only now but for some reason Herald only subscribed me today. First of all, thanks Sjoerd for working on this. I agree with the current consensus, that seems the right approach to me. Below are some wishlist items this discussion made me think about in case any of you find them of interest.

In D53710#1291939, @probinson wrote:

Because there is nothing to indicate that a line contains a FileCheck directive, other than that it pattern-matches a user-specified string with an optional suffix, we can't reliably report all typos. So for example:

CHECK-LABEL: A
CHECK: B
CHEKC: C

the third line can't be diagnosed by FileCheck under any reasonable heuristic. So, anything we do is a best-effort, and ideally produces no false positives by default.

While I agree we cannot diagnose all typos I do think we ought to be able to catch simple cases and I think 1 letter swap, 1 missing letter and 1 extra letter all ought to be diagnosed. That can be combined with Sjoerd's patch by adding an extra message in case the condition for the warning does occur: carets pointing to the suspected typoed CHECK followed by "did you mean <correct CHECK>?".

I would also like in a separate patch trying to diagnose things that look like a directive and are close to one of the --check-prefixes, e.g.:

FileCheck --check-prefix FOO

FOO: correct check
FO: typoed check

This example would not trigger under the follow-up patch proposed by Paul since there's one correct occurence of the directive FOO yet probably suggest an error. So to summarize:

• I would like to see Sjoerd's new warning give suggestion for places which might just be a typo. Just to be clear, this can be done as a follow-up patch, and I am not asking you Sjoerd to do it, just advertising it in case some brave soul feel like taking on the challenge
• I would like to see a new warning to find what looks like a typoed directive of one of the CHECK prefix

[SjoerdMeijer]

This was actually still on my todo list.....I really would like to finish this one day because I've spent already quite some time on this, but I guess I was getting doubts if I could get this deployed. That is, I believe there were doubts about its usefulness if this is done optionally under a flag, or a big bang of fixing all test cases first, and then making this default behaviour..... But to be honest, I need to reread this thread to remind myself where we exactly were with this.

[jdenny]

Before proceeding, keep in mind that D54769 is closely related and was reviewed at the same time, so the discussion there should be considered too.

lib/Support/FileCheck.cpp

ImplicitNegativeChecks.back().ParsePattern(PatternInBuffer,
                                                "IMPLICIT-CHECK", SM, 0, Req);
   }
 
   std::vector<FileCheckPattern> DagNotMatches = ImplicitNegativeChecks;
 
   // LineNumber keeps track of the line on which CheckPrefix instances are
   // found.
   unsigned LineNumber = 1;
+  bool HasNonLabelPrefix = false;
+  StringRef MatchedPrefix;
 
   while (1) {
     Check::FileCheckType CheckTy;
 
     // See if a prefix occurs in the memory buffer.
     StringRef UsedPrefix = FindFirstMatchingPrefix(PrefixRE, Buffer, LineNumber,
                                                    CheckTy);
     if (UsedPrefix.empty())
       break;
+    MatchedPrefix = UsedPrefix;
     assert(UsedPrefix.data() == Buffer.data() &&
            "Failed to move Buffer's start forward, or pointed prefix outside "
            "of the buffer!");
 
     // Location to use for error messages.
     const char *UsedPrefixStart = UsedPrefix.data();
 
     // Skip the buffer to the end.
     Buffer = Buffer.drop_front(UsedPrefix.size() + CheckTypeSize(CheckTy));
 
     // Complain about useful-looking but unsupported suffixes.
     if (CheckTy == Check::CheckBadNot) {
       SM.PrintMessage(SMLoc::getFromPointer(Buffer.data()), SourceMgr::DK_Error,
                       "unsupported -NOT combo on prefix '" + UsedPrefix + "'");
       return true;
     }
 
-    // Okay, we found the prefix, yay. Remember the rest of the line, but ignore
-    // leading whitespace.
+    // Okay, we found the prefix, yay.
+    if (CheckTy != Check::CheckLabel)
+      HasNonLabelPrefix = true;
+
+    // Remember the rest of the line, but ignore leading whitespace.
     if (!(Req.NoCanonicalizeWhiteSpace && Req.MatchFullLines))
       Buffer = Buffer.substr(Buffer.find_first_not_of(" \t"));
 
     // Scan ahead to the end of line.
     size_t EOL = Buffer.find_first_of("\n\r");
 
     // Remember the location of the start of the pattern, for diagnostics.
     SMLoc PatternLoc = SMLoc::getFromPointer(Buffer.data());
if (CheckStrings.empty()) {
     }
     for (; I != E; ++I)
       errs() << ", \'" << *I << ":'";
 
     errs() << '\n';
     return true;
   }
 
+  if (!HasNonLabelPrefix)
+    SM.PrintMessage(SMLoc::getFromPointer(Buffer.data()),
+                    SourceMgr::DK_Note,
+                    "Prefix " + MatchedPrefix + " only occurs in a LABEL check, "
+                    "this is probably not what you want");
+
   return false;
 }
 
 static void PrintMatch(bool ExpectedMatch, const SourceMgr &SM,
                        StringRef Prefix, SMLoc Loc, const FileCheckPattern &Pat,
                        StringRef Buffer, StringMap<StringRef> &VariableTable,
                        size_t MatchPos, size_t MatchLen,
                        const FileCheckRequest &Req) {