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

[grimar]

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.

[jdenny]

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]

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]

• Addressed review comments.
• Fixed the broken test case in LLD missed previously.

[grimar]

• Removed the fix for LLD test from this patch (I've committed it separatelly, rG09331fd742ef72836fc651b71dce90dfae63c2d8)

[grimar]

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]

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]

• Addressed review comments.
• Refined comments slightly in the test case.

In D78024#1981409, @jdenny wrote:

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]

In D78024#1983304, @grimar wrote:

In D78024#1981409, @jdenny wrote:

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?

[grimar]

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.

[grimar]

Thanks for review :)

[MaskRay]

LGTM.

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

3

;;

[grimar]

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

[grimar]

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?

[MaskRay]

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]

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]

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]

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.

[probinson]

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.

[grimar]

In D78024#1989287, @probinson wrote:

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

[Higuoxing]

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

[grimar]

In D78024#2087800, @Higuoxing wrote:

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.

clang/test/CodeGen/catch-implicit-conversions-basics-negatives.c

-// RUN: %clang_cc1 -fsanitize=implicit-unsigned-integer-truncation,implicit-signed-integer-truncation,implicit-integer-sign-change -fsanitize-recover=implicit-unsigned-integer-truncation,implicit-signed-integer-truncation,implicit-integer-sign-change -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_implicit_conversion" --check-prefixes=CHECK
+// RUN: %clang_cc1 -fsanitize=implicit-unsigned-integer-truncation,implicit-signed-integer-truncation,implicit-integer-sign-change -fsanitize-recover=implicit-unsigned-integer-truncation,implicit-signed-integer-truncation,implicit-integer-sign-change -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_implicit_conversion"
 
 // If we have an enum, it will be promoted to an unsigned integer.
 // But both types are unsigned, and have same bitwidth.
 // So we should not emit any sanitization. Also, for inc/dec we currently assume
 // (assert) that we will only have cases where at least one of the types
 // is signed, which isn't the case here.
 typedef enum { a } b;
 b t0(b c) {
   c--;
   return c;
 }

llvm/include/llvm/Support/FileCheck.h

 struct FileCheckRequest {
   std::vector<std::string> CheckPrefixes;
   bool NoCanonicalizeWhiteSpace = false;
   std::vector<std::string> ImplicitCheckNot;
   std::vector<std::string> GlobalDefines;
   bool AllowEmptyInput = false;
   bool MatchFullLines = false;
   bool IgnoreCase = false;
+  bool IsDefaultCheckPrefix = false;
   bool EnableVarScope = false;
   bool AllowDeprecatedDagOverlap = false;
   bool Verbose = false;
   bool VerboseVerbose = false;
 };
 
 //===----------------------------------------------------------------------===//
 // Summary of a FileCheck diagnostic.

llvm/lib/Support/FileCheck.cpp

bool FileCheck::readCheckFile(SourceMgr &SM, StringRef Buffer,
   }
 
   std::vector<Pattern> DagNotMatches = ImplicitNegativeChecks;
 
   // LineNumber keeps track of the line on which CheckPrefix instances are
   // found.
   unsigned LineNumber = 1;
 
+  bool FoundUsedPrefix = false;
   while (1) {
     Check::FileCheckType CheckTy;
 
     // See if a prefix occurs in the memory buffer.
     StringRef UsedPrefix;
     StringRef AfterSuffix;
     std::tie(UsedPrefix, AfterSuffix) =
         FindFirstMatchingPrefix(PrefixRE, Buffer, LineNumber, CheckTy);
     if (UsedPrefix.empty())
       break;
+    FoundUsedPrefix = true;
+
     assert(UsedPrefix.data() == Buffer.data() &&
            "Failed to move Buffer's start forward, or pointed prefix outside "
            "of the buffer!");
     assert(AfterSuffix.data() >= Buffer.data() &&
            AfterSuffix.data() < Buffer.data() + Buffer.size() &&
            "Parsing after suffix doesn't start inside of buffer!");
 
     // Location to use for error messages.
while (1) {
     }
 
     // Okay, add the string we captured to the output vector and move on.
     CheckStrings->emplace_back(P, UsedPrefix, PatternLoc);
     std::swap(DagNotMatches, CheckStrings->back().DagNotStrings);
     DagNotMatches = ImplicitNegativeChecks;
   }
 
-  // Add an EOF pattern for any trailing CHECK-DAG/-NOTs, and use the first
-  // prefix as a filler for the error message.
-  if (!DagNotMatches.empty()) {
-    CheckStrings->emplace_back(
-        Pattern(Check::CheckEOF, PatternContext.get(), LineNumber + 1),
-        *Req.CheckPrefixes.begin(), SMLoc::getFromPointer(Buffer.data()));
-    std::swap(DagNotMatches, CheckStrings->back().DagNotStrings);
-  }
-
-  if (CheckStrings->empty()) {
+  // 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.
+  if (!FoundUsedPrefix &&
+      (ImplicitNegativeChecks.empty() || !Req.IsDefaultCheckPrefix)) {
     errs() << "error: no check strings found with prefix"
            << (Req.CheckPrefixes.size() > 1 ? "es " : " ");
     auto I = Req.CheckPrefixes.begin();
     auto E = Req.CheckPrefixes.end();
     if (I != E) {
       errs() << "\'" << *I << ":'";
       ++I;
     }
     for (; I != E; ++I)
       errs() << ", \'" << *I << ":'";
 
     errs() << '\n';
     return true;
   }
 
+  // Add an EOF pattern for any trailing CHECK-DAG/-NOTs, and use the first
+  // prefix as a filler for the error message.
+  if (!DagNotMatches.empty()) {
+    CheckStrings->emplace_back(
+        Pattern(Check::CheckEOF, PatternContext.get(), LineNumber + 1),
+        *Req.CheckPrefixes.begin(), SMLoc::getFromPointer(Buffer.data()));
+    std::swap(DagNotMatches, CheckStrings->back().DagNotStrings);
+  }
+
   return false;
 }
 
 static void PrintMatch(bool ExpectedMatch, const SourceMgr &SM,
                        StringRef Prefix, SMLoc Loc, const Pattern &Pat,
                        int MatchedCount, StringRef Buffer, size_t MatchPos,
                        size_t MatchLen, const FileCheckRequest &Req,
                        std::vector<FileCheckDiag> *Diags) {
bool FileCheck::ValidateCheckPrefixes() {
   }
 
   return true;
 }
 
 Regex FileCheck::buildCheckPrefixRegex() {
   // I don't think there's a way to specify an initial value for cl::list,
   // so if nothing was specified, add the default
-  if (Req.CheckPrefixes.empty())
+  if (Req.CheckPrefixes.empty()) {
     Req.CheckPrefixes.push_back("CHECK");
+    Req.IsDefaultCheckPrefix = true;
+  }
 
   // We already validated the contents of CheckPrefixes so just concatenate
   // them as alternatives.
   SmallString<32> PrefixRegexStr;
   for (StringRef Prefix : Req.CheckPrefixes) {
     if (Prefix != Req.CheckPrefixes.front())
       PrefixRegexStr.push_back('|');
 

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

 ; RUN: sed 's#^;.*##' %s | FileCheck -check-prefix=CHECK-PASS -implicit-check-not=warning: %s
+
+; Check we report an error when an unknown prefix is used together with `-implicit-check-not`.
+; RUN: sed 's#^;.*##' %s | %ProtectFileCheckOutput not FileCheck -check-prefix=UNKNOWN-PREFIX -implicit-check-not=abc %s 2>&1 | FileCheck %s -DPREFIX=UNKNOWN-PREFIX -check-prefix CHECK-PREFIX-ERROR
+; CHECK-PREFIX-ERROR: error: no check strings found with prefix '[[PREFIX]]:'
+
+; Check we report an error when the "CHECK" prefix is used explicitly with `-implicit-check-not`, but not present in the input.
+; RUN: sed 's#^;.*##' %s | %ProtectFileCheckOutput not FileCheck -check-prefix=CHECK -implicit-check-not=abc %s 2>&1 | FileCheck %s -DPREFIX=CHECK -check-prefix CHECK-PREFIX-ERROR
+
+; Check we allow using `-implicit-check-not` when there is no `-check-prefix` specified and there
+; is no default `CHECK` line in an input.
+; RUN: sed 's#^;.*##' %s | FileCheck -implicit-check-not="unique_string" %s
+
 ; RUN: sed 's#^;.*##' %s | %ProtectFileCheckOutput not FileCheck -check-prefix=CHECK-FAIL1 -implicit-check-not=warning: %s 2>&1 | FileCheck %s -check-prefix CHECK-ERROR1
 ; RUN: sed 's#^;.*##' %s | %ProtectFileCheckOutput not FileCheck -check-prefix=CHECK-FAIL2 -implicit-check-not=warning: %s 2>&1 | FileCheck %s -check-prefix CHECK-ERROR2
 ; RUN: sed 's#^;.*##' %s | %ProtectFileCheckOutput not FileCheck -check-prefix=CHECK-FAIL3 -implicit-check-not=warning: %s 2>&1 | FileCheck %s -check-prefix CHECK-ERROR3
 ; RUN: sed 's#^;.*##' %s | %ProtectFileCheckOutput not FileCheck -check-prefix=CHECK-FAIL1 -implicit-check-not='{{aaa|bbb|ccc}}' %s 2>&1 | FileCheck %s -check-prefix CHECK-ERROR4
 ; RUN: sed 's#^;.*##' %s | %ProtectFileCheckOutput not FileCheck -check-prefix=CHECK-FAIL1 -implicit-check-not=aaa -implicit-check-not=bbb -implicit-check-not=ccc %s 2>&1 | FileCheck %s -check-prefix CHECK-ERROR5
 ; RUN: sed 's#^;.*##' %s | %ProtectFileCheckOutput not FileCheck -check-prefix=CHECK-FAIL2 -implicit-check-not=aaa -implicit-check-not=bbb -implicit-check-not=ccc %s 2>&1 | FileCheck %s -check-prefix CHECK-ERROR6
 ; RUN: sed 's#^;.*##' %s | %ProtectFileCheckOutput not FileCheck -check-prefix=CHECK-FAIL3 -implicit-check-not=aaa -implicit-check-not=bbb -implicit-check-not=ccc %s 2>&1 | FileCheck %s -check-prefix CHECK-ERROR7
 

llvm/test/tools/llvm-objcopy/MachO/strip-debug.test

 ## Show that llvm-objcopy/llvm-strip removes all symbols and debug sections.
 
 # RUN: yaml2obj %p/Inputs/strip-all-with-dwarf.yaml -o %t
 
 # RUN: llvm-objcopy --strip-debug %t %t.stripped
-# RUN: llvm-readobj --sections %t.stripped | FileCheck /dev/null --check-prefix=NODWARF \
+# RUN: llvm-readobj --sections %t.stripped | FileCheck /dev/null \
 # RUN:   --implicit-check-not='Name: __debug' --implicit-check-not='Name: __apple'
 
 ## Make sure that all symbols are kept.
 # RUN: llvm-readobj --symbols %t | FileCheck %s --check-prefix=SYM
 # RUN: llvm-readobj --symbols %t.stripped | FileCheck %s --check-prefix=SYM
 
 # SYM:         Symbols [
 # SYM-COUNT-3: Symbol