This is an archive of the discontinued LLVM Phabricator instance.

[FileCheck] - Fix the false positive when -implicit-check-not is used with an unknown -check-prefix.
ClosedPublic

Authored by grimar on Apr 13 2020, 8:40 AM.

Details

Summary

Imagine we have the following invocation:

FileCheck -check-prefix=UNKNOWN-PREFIX -implicit-check-not=something

When the check prefix does not exist I'd expect it to fail, but it does not.

This patch fixes the issue.

Diff Detail

Event Timeline

grimar created this revision.Apr 13 2020, 8:40 AM
jdenny added a subscriber: jdenny.
jdenny added inline comments.
llvm/test/FileCheck/implicit-check-not.txt
3–11

All of these new tests pass for me even without your changes to FileCheck.

4

Please add %ProtectFileCheckOutput on the first FileCheck command here because its textual output affects test results.

jdenny added inline comments.Apr 13 2020, 11:11 AM
llvm/lib/Support/FileCheck.cpp
1277

I think a clearer name is FoundUsedPrefix.

1374–1375

What if the only explicitly specified prefix is CHECK? Oddly, it happens.

grimar updated this revision to Diff 257260.Apr 14 2020, 3:16 AM
grimar marked 4 inline comments as done.
  • Addressed review comments.
  • Fixed the broken test case in LLD missed previously.
grimar updated this revision to Diff 257263.Apr 14 2020, 3:24 AM
grimar added inline comments.Apr 14 2020, 3:25 AM
llvm/lib/Support/FileCheck.cpp
1374–1375

Yeah, I thought about this strange case.
In the implementation of the first diff, such case could produce a false-positive result when
CHECK lines are missing.

I was not sure how we might want to handle it. I prefer to not use --check-prefix=CHECK alone
as it probably just adds noise to the test cases. FileCheck perhaps could report a error to ban this case.

Though thinking about it again, there can be other cases where CHECK is involved,
perhaps the simplest/cleanest way to go is just to recognize the default case better and handle it too?
I've added a flag to FileCheckRequest to implement it.
(That is was what I was not sure is worth doing - adding a new flag).

llvm/test/FileCheck/implicit-check-not.txt
3–11

Ah, I am sorry. For the first test case it happened because of the following check line:
error: no check strings found with prefix 'UNKNOWN-PREFIX:'
which reported the unknown prefix in its body (and so FoundUsedPrefix was set to true because of that.
It just a bit unusual to me that a check line can trigger the different test case behavior..).

The second test is new in the latest diff.

And the third test is expected to pass with or without this patch, I've added it because it is related
to changes done by this patch and there seems to be no test case anywhere which checks the same.

jdenny marked an inline comment as done.Apr 14 2020, 11:37 AM

What test suites did you try this on? It's good to be cautious when increasing FileCheck's strictness.

llvm/lib/Support/FileCheck.cpp
1374–1375

I think that's a good solution. Thanks.

1374–1376

I find this logic confusing. Its goal appears to be to constrain when DagNotMatches are added to CheckStrings. However, the real goal is to constrain when FileCheck complains that there are no used prefixes. Would the following logic work?

if (!FoundUsedPrefix && (ImplicitNegativeChecks.empty() || !Req.IsDefaultCheckPrefix)) {
  errs() << "error: no check strings found with prefix"
  . . .
}
if (!DagNotMatches.empty()) {
  CheckStrings->emplace_back(
  . . .
}
grimar updated this revision to Diff 257671.Apr 15 2020, 4:29 AM
grimar marked an inline comment as done.
  • Addressed review comments.
  • Refined comments slightly in the test case.

What test suites did you try this on? It's good to be cautious when increasing FileCheck's strictness.

I used run check-llvm and check-lld previously for this patch.
For the latest diff I've used check-all (which revealed one more test case failture in CodeGen/catch-implicit-conversions-basics-negatives.c)..

llvm/lib/Support/FileCheck.cpp
1374–1376

This looks better and works, thanks!

(The code above has DagNotMatches = ImplicitNegativeChecks; line which is also a bit confusing probably)

jdenny accepted this revision.Apr 15 2020, 6:17 AM

What test suites did you try this on? It's good to be cautious when increasing FileCheck's strictness.

I used run check-llvm and check-lld previously for this patch.
For the latest diff I've used check-all (which revealed one more test case failture in CodeGen/catch-implicit-conversions-basics-negatives.c)..

In case you didn't configure all LLVM projects or some tests were skipped for your config, please watch the bots carefully.

LGTM except for some nits on the comments.

Thanks for this!

llvm/lib/Support/FileCheck.cpp
1374–1376

This looks better and works, thanks!

Thanks for making that change.

(The code above has DagNotMatches = ImplicitNegativeChecks; line which is also a bit confusing probably)

I'm fine with that one. DagNotMatches starts out as ImplicitNegativeChecks, and CHECK-NOTs might be added later.

I think the second sentence in the following comment still reflects the old logic:

// When there are no used prefixes we report an error.
// We do not allow using -implicit-check-not when an explicitly specified
// check prefix is not present in the input buffer.

Something like the following seems better to me:

// When there are no used prefixes we report an error except in the case that
// no prefix is specified explicitly but -implicit-check-not is specified.
1393–1394

Your original patch mentioned -implicit-check-not here. I thought that was helpful. Is there a reason to drop it?

This revision is now accepted and ready to land.Apr 15 2020, 6:17 AM
grimar marked an inline comment as done.Apr 15 2020, 6:28 AM
grimar added inline comments.
llvm/lib/Support/FileCheck.cpp
1393–1394

Yeah, I've dropped it because it is unrelated to the latest version of the patch, as now this piece of the code is unchanged, it just moved.
I'll commit a NFC change for this comment separatelly then.

Thanks for review :)

MaskRay accepted this revision.Apr 15 2020, 10:56 PM

LGTM.

llvm/test/FileCheck/implicit-check-not.txt
3

;;

grimar marked an inline comment as done.Apr 16 2020, 2:43 AM
grimar added inline comments.
llvm/test/FileCheck/implicit-check-not.txt
3

No FileCheck test does that, so I'd avoid introducing it in this patch.

(Generally the idea is probably good, it is consistent with ## we use for comments in tools tests.)

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2020, 5:03 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
grimar marked an inline comment as done.Apr 16 2020, 7:50 AM
grimar added inline comments.
llvm/lib/Support/FileCheck.cpp
1374–1376

the real goal is to constrain when FileCheck complains that there are no used prefixes

btw, do you know why FileCheck seems intentionally allows the case when --check-prefixes=KNOWN,UNKNOWN?
I've mentioned in the description of D78110 that there are about 1000 tests that have this. but is it a feature or a something that we might want to fix?

btw, do you know why FileCheck seems intentionally allows the case when --check-prefixes=KNOWN,UNKNOWN? I've mentioned in the description of D78110 that there are about 1000 tests that have this. but is it a feature or a something that we might want to fix?

Confirmed. Both FileCheck --check-prefix=KNOWN --check-prefix=UNKNOWN and FileCheck --check-prefixes=KNOWN,UNKNOWN are allowed. This looks to me another bug-prone area which we should clean up.

jdenny marked an inline comment as done.Apr 16 2020, 2:31 PM
jdenny added a subscriber: SjoerdMeijer.
jdenny added inline comments.
llvm/lib/Support/FileCheck.cpp
1374–1376

There was a long discussion about diagnostics like that over a year ago. I believe we described that case as prefixes that are defined but not used.

In some of my downstream work, I often create FileCheck prefix schemes reflecting various combinations of features I want to test. For example: FOO, BAR, FOO-AND-BAR, FOO-NOT-BAR, etc. It's far easier to maintain those tests if I list all appropriate prefixes for each FileCheck command even if some prefixes are not currently used in the test file. I don't know if this use case exists upstream right now.

Also, when developing or debugging tests involving many prefixes and many FileCheck commands, I think it could be painful to have to update all FileCheck commands every time you temporarily remove the only uses of some prefix.

If you pursue this diagnostic, please make it optional, even if on by default. At one time, we discussed a warning system for such diagnostics. Warnings could be configurable via command-line options, possibly passed via the FILECHECK_OPTS env var so that it's easy to relax them during debugging or based on a test suite's specific needs.

@probinson and @SjoerdMeijer might want to chime in as they were also part of previous discussions.

grimar marked an inline comment as done.Apr 17 2020, 1:26 AM
grimar added inline comments.
llvm/lib/Support/FileCheck.cpp
1374–1376

I see, thanks for details.

D78110 and this patch (which revealed rG09331fd7) showed that we had test cases committed which
had unknown prefixes placed by mistake and hence false positives.
I remember how accidentally pushed on review the patch with the same issue and nobody noticed..
I just think that making FileCheck stricter could prevent such issues.

If we'll have a consensus about making --check-prefixes stricter I'll be happy to work on this.

jdenny marked an inline comment as done.Apr 17 2020, 6:35 AM
jdenny added inline comments.
llvm/lib/Support/FileCheck.cpp
1374–1376

As long as it's optional, I think I'd be fine with that diagnostic, and I agree it would probably be more helpful than harmful in most cases. Of course, check-all would need to be run before pushing to be sure.

Instead of the system of warning options I mentioned, we might just have something simple like -allow-unused-prefix (similar to -allow-empty) to disable it when it gets in the way.

To me, "unknown prefix" is ambiguous and sounds more like "undefined prefix", which I interpret to mean "never specified in a -prefix[es]". I prefer "unused prefix" for this diagnostic.

I'm just reading this review for the first time, and my thought was, why is this interacting with implicit-check-not?

I think specifying a --check-prefix=FOO with no uses of FOO should be diagnosed. I can't say I recall the previous discussion in detail, but that's what I think now.

I'm just reading this review for the first time, and my thought was, why is this interacting with implicit-check-not?

It was interacting (this patch was committed and fixed the issue), because the code used the logic to add an EOF pattern
for "any trailing --implicit-check-not/CHECK-DAG/-NOTs" before the error about "no check strings found with prefix" was reported.
I.e. this logic suppressed the error, because CheckStrings was not empty anymore:

  if (CheckStrings->empty()) {
    errs() << "error: no check strings found with prefix"
...

btw, do you know why FileCheck seems intentionally allows the case when --check-prefixes=KNOWN,UNKNOWN?
I've mentioned in the description of D78110 that there are about 1000 tests that have this. but is it a feature or a something that we might want to fix?>

I noticed this strange behavior of FileCheck as well. When I try to fix it, there are lots of test failures. /subscribe

Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2020, 9:00 AM
grimar added a comment.EditedJun 12 2020, 1:15 AM

btw, do you know why FileCheck seems intentionally allows the case when --check-prefixes=KNOWN,UNKNOWN?
I've mentioned in the description of D78110 that there are about 1000 tests that have this. but is it a feature or a something that we might want to fix?>

I noticed this strange behavior of FileCheck as well. When I try to fix it, there are lots of test failures. /subscribe

Yeah. To fix it we should prepare patches for those ~1000 tests in

CodeGen/*
DebugInfo/X86/*
Instrumentation/*
Linker/*
MC/*
Other/*
Transforms/*

(and few other places) first. When I investigated them, I came to conclusion they just have unused prefixes
for no solid reason. I am not an expert in those folders/areas though. I thought about starting from a patch for one of those folders to see
if it will be approved without problems by people who work on the features there. But I had no chance to work on that yet.