[FileCheck] Abort if -NOT is combined with another suffix

[probinson]

Summary

The -NOT suffix by itself is implicitly DAG, but -DAG-NOT is ignored.
And as handy as -SAME-NOT and -NEXT-NOT might be, they are actually
not supported. Saying so will reduce the incidence of bogus tests.

Separate reviews will fix the LLVM and Clang tests that have this problem.

[probinson]

LLVM test fixes are in D17588.
Clang test fixes are in D17589.
I made those separate because they'll have to be committed before the FileCheck change goes in.

[jyknight]

Sounds good after the prerequisite changes go in.

[arsenm]

Another good thing to catch would be CHECK-DAGs mixed with CHECK-NOTs, which does not work as expected

[probinson]

In D17587#362018, @arsenm wrote:

Another good thing to catch would be CHECK-DAGs mixed with CHECK-NOTs, which does not work as expected

What are your expectations?

[arsenm]

In D17587#362039, @probinson wrote:

In D17587#362018, @arsenm wrote:

Another good thing to catch would be CHECK-DAGs mixed with CHECK-NOTs, which does not work as expected

What are your expectations?

I often want to check a sequence of N instructions are in any order, but also excluding others between them. IIRC, the -NOT may just be a sub-problem of multiple identical CHECK-DAGs.

Something like
; CHECK-DAG: add
; CHECK-DAG: sub
; CHECK-DAG: add

Will pass with only 1 add in the sequence. The -NOT to exclude some intermixed instructions acts as a barrier between the sections of CHECK-DAGs.

[probinson]

In D17587#362059, @arsenm wrote:

The -NOT to exclude some intermixed instructions acts as a barrier between the sections of CHECK-DAGs.

Okay, that's how it's documented to work. And some experimentation suggests that despite the documentation, that's not actually how it works. Huh.
A CHECK-DAG followed by CHECK-NOT causes the CHECK-DAG match to bound the range for the CHECK-NOT.
However, a CHECK-NOT followed by a CHECK-DAG does not cause the CHECK-DAG to bound the range of the CHECK-NOT.

That feels more like a bug in FileCheck, than a mistake made by the test-writer? In this patch I'm trying to avoid test-writer mistakes.

[jroelofs]

@probinson: if you're on a general crusade against more such test-writer mistakes, here's another one:

• CHECK lines without the requisite colon

I've fixed a few individual instances of this before, but when I sat down to try to get FileCheck to warn about it, I couldn't see how to do it without making a mess. Perhaps you've got a better handle on how to do it?

[probinson]

In D17587#362099, @jroelofs wrote:

@probinson: if you're on a general crusade against more such test-writer mistakes, here's another one:

• CHECK lines without the requisite colon

I've fixed a few individual instances of this before, but when I sat down to try to get FileCheck to warn about it, I couldn't see how to do it without making a mess. Perhaps you've got a better handle on how to do it?

I was not necessarily on a general campaign (although clearly FileCheck and its documentation could use a little love). Your suggestion in particular could be subject to false positives (for example if you said "RUN: ... -check-prefix MYSTUFF" you don't want to report that RUN line).

Another one I've tripped over is that if you specify multiple -check-prefix options, FileCheck won't detect typos. It insists that any specified combination of prefixes must result in a nonzero number of checks, but it doesn't verify that each individual prefix occurs at least once.

[arsenm]

In D17587#362122, @probinson wrote:

Another one I've tripped over is that if you specify multiple -check-prefix options, FileCheck won't detect typos. It insists that any specified combination of prefixes must result in a nonzero number of checks, but it doesn't verify that each individual prefix occurs at least once.

I would not want it to verify that each individual prefix occurs at least once

[probinson]

In D17587#362136, @arsenm wrote:

In D17587#362122, @probinson wrote:

Another one I've tripped over is that if you specify multiple -check-prefix options, FileCheck won't detect typos. It insists that any specified combination of prefixes must result in a nonzero number of checks, but it doesn't verify that each individual prefix occurs at least once.

I would not want it to verify that each individual prefix occurs at least once

What is your use-case for a -check-prefix option that specifies a prefix that does not exist?

llvm/trunk/test/FileCheck/no-multi-suffixes.txt

+; RUN: not FileCheck -check-prefix=DAGNOT -input-file %s %s 2>&1 | FileCheck -check-prefix=ERROR -check-prefix=DAGNOT-ERROR %s
+; RUN: not FileCheck -check-prefix=NOTDAG -input-file %s %s 2>&1 | FileCheck -check-prefix=ERROR -check-prefix=NOTDAG-ERROR %s
+; RUN: not FileCheck -check-prefix=NEXTNOT -input-file %s %s 2>&1 | FileCheck -check-prefix=ERROR -check-prefix=NEXTNOT-ERROR %s
+; RUN: not FileCheck -check-prefix=NOTNEXT -input-file %s %s 2>&1 | FileCheck -check-prefix=ERROR -check-prefix=NOTNEXT-ERROR %s
+; RUN: not FileCheck -check-prefix=SAMENOT -input-file %s %s 2>&1 | FileCheck -check-prefix=ERROR -check-prefix=SAMENOT-ERROR %s
+; RUN: not FileCheck -check-prefix=NOTSAME -input-file %s %s 2>&1 | FileCheck -check-prefix=ERROR -check-prefix=NOTSAME-ERROR %s
+
+foo
+; DAGNOT: foo
+; DAGNOT-DAG-NOT: bar
+; NOTDAG: foo
+; NOTDAG-NOT-DAG: bar
+; NEXTNOT: foo
+; NEXTNOT-NEXT-NOT: bar
+; NOTNEXT: foo
+; NOTNEXT-NOT-NEXT: bar
+; SAMENOT: foo
+; SAMENOT-SAME-NOT: bar
+; NOTSAME: foo
+; NOTSAME-NOT-SAME: bar
+
+; ERROR: error: unsupported -NOT combo on prefix
+; DAGNOT-ERROR-SAME:  'DAGNOT'
+; NOTDAG-ERROR-SAME:  'NOTDAG'
+; NEXTNOT-ERROR-SAME: 'NEXTNOT'
+; NOTNEXT-ERROR-SAME: 'NOTNEXT'
+; SAMENOT-ERROR-SAME: 'SAMENOT'
+; NOTSAME-ERROR-SAME: 'NOTSAME'

llvm/trunk/utils/FileCheck/FileCheck.cpp

enum CheckType {
     CheckNext,
     CheckSame,
     CheckNot,
     CheckDAG,
     CheckLabel,
 
     /// MatchEOF - When set, this pattern only matches the end of file. This is
     /// used for trailing CHECK-NOTs.
-    CheckEOF
+    CheckEOF,
+    /// CheckBadNot - Found -NOT combined with another CHECK suffix.
+    CheckBadNot
   };
 }
 
 class Pattern {
   SMLoc PatternLoc;
 
   Check::CheckType CheckTy;
 

 static bool IsPartOfWord(char c) {
   return (isalnum(c) || c == '-' || c == '_');
 }
 
 // Get the size of the prefix extension.
 static size_t CheckTypeSize(Check::CheckType Ty) {
   switch (Ty) {
   case Check::CheckNone:
+  case Check::CheckBadNot:
     return 0;
 
   case Check::CheckPlain:
     return sizeof(":") - 1;
 
   case Check::CheckNext:
     return sizeof("-NEXT:") - 1;
 
if (Rest.startswith("NOT:"))
     return Check::CheckNot;
 
   if (Rest.startswith("DAG:"))
     return Check::CheckDAG;
 
   if (Rest.startswith("LABEL:"))
     return Check::CheckLabel;
 
+  // You can't combine -NOT with another suffix.
+  if (Rest.startswith("DAG-NOT:") || Rest.startswith("NOT-DAG:") ||
+      Rest.startswith("NEXT-NOT:") || Rest.startswith("NOT-NEXT:") ||
+      Rest.startswith("SAME-NOT:") || Rest.startswith("NOT-SAME:"))
+    return Check::CheckBadNot;
+
   return Check::CheckNone;
 }
 
 // From the given position, find the next character after the word.
 static size_t SkipWord(StringRef Str, size_t Loc) {
   while (Loc < Str.size() && IsPartOfWord(Str[Loc]))
     ++Loc;
   return Loc;
while (1) {
     Buffer = Buffer.drop_front(PrefixLoc);
 
     // Location to use for error messages.
     const char *UsedPrefixStart = Buffer.data() + (PrefixLoc == 0 ? 0 : 1);
 
     // PrefixLoc is to the start of the prefix. Skip 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 and trailing whitespace.
     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.