[PATCH] D77227: [RFC][FileCheck] Require colon immediately after CHECK directives

[Jonathan Roelofs via Phabricator via llvm-commits]

jroelofs created this revision.
jroelofs added a reviewer: jdoerfert.
Herald added subscribers: llvm-commits, thopre, hiraditya, arichardson.
Herald added a project: LLVM.
jroelofs edited the summary of this revision.
jroelofs edited the summary of this revision.

There are a lot of common pattern that results in testcases that look like they
are checking something, but in fact they silently hide failures, leading to
accidental false negatives.

  CHECK:          legitimate test
  CHECK           gotcha 1
  CHECK :         gotcha 2
  CHECK-SAME-DAG: gotcha 3
  CHECKNEXT:      gotcha 4
  CHECKDAG:       gotcha 5

This patch attempts to catch 1 & 2, but it unfortunately still has a lot of
false positives, as it triggers on comments in testcases that refer to the
CHECKs themselves. The vast majority of false positives come from this
diagnostic triggering on RUN lines, which could be ameliorated (mostly) with a
precursor patch that quotes all the CHECK prefixes:

  find -E llvm/test -regex ".*\.(c|cpp|m|ll|mir|yaml|s|td|test|txt)" -exec perl -p -i -e "s/check-prefix(es)?[= ]([^ \r\n]+)/check-prefix\1=\"\2\"/g" {} \;

But even with that, there are still ~455 "failures" this causes in llvm/test alone.

Is this worth moving forward on? If so, how else can we improve FileCheck to
prevent these kinds of issues?


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77227

Files:
  llvm/include/llvm/Support/FileCheck.h
  llvm/lib/Support/FileCheck.cpp
  llvm/utils/FileCheck/FileCheck.cpp


Index: llvm/utils/FileCheck/FileCheck.cpp
===================================================================
--- llvm/utils/FileCheck/FileCheck.cpp
+++ llvm/utils/FileCheck/FileCheck.cpp
@@ -287,6 +287,8 @@
     return "bad-not";
   case Check::CheckBadCount:
     return "bad-count";
+  case Check::CheckBadColon:
+    return "bad-colon";
   case Check::CheckNone:
     llvm_unreachable("invalid FileCheckType");
   }
Index: llvm/lib/Support/FileCheck.cpp
===================================================================
--- llvm/lib/Support/FileCheck.cpp
+++ llvm/lib/Support/FileCheck.cpp
@@ -1088,6 +1088,8 @@
     return "bad NOT";
   case Check::CheckBadCount:
     return "bad COUNT";
+  case Check::CheckBadColon:
+    return "bad colon";
   }
   llvm_unreachable("unknown FileCheckType");
 }
@@ -1104,6 +1106,9 @@
   if (NextChar == ':')
     return {Check::CheckPlain, Rest};
 
+  if (NextChar == ' ' || NextChar == '\t')
+    return {Check::CheckBadColon, Rest};
+
   if (NextChar != '-')
     return {Check::CheckNone, StringRef()};
 
@@ -1144,6 +1149,11 @@
       Rest.startswith("EMPTY-NOT:") || Rest.startswith("NOT-EMPTY:"))
     return {Check::CheckBadNot, Rest};
 
+  if (Rest.consume_front("NEXT") || Rest.consume_front("SAME") ||
+      Rest.consume_front("NOT") || Rest.consume_front("DAG") ||
+      Rest.consume_front("LABEL") || Rest.consume_front("EMPTY"))
+    return {Check::CheckBadColon, Rest};
+
   return {Check::CheckNone, Rest};
 }
 
@@ -1306,6 +1316,14 @@
       return true;
     }
 
+    // Complain about directives that are not followed immediately by a colon.
+    if (CheckTy == Check::CheckBadColon) {
+      SM.PrintMessage(SMLoc::getFromPointer(Buffer.data()), SourceMgr::DK_Error,
+                      "colon required after check directive '" + UsedPrefix +
+                          "'");
+      return true;
+    }
+
     // Complain about invalid count specification.
     if (CheckTy == Check::CheckBadCount) {
       SM.PrintMessage(SMLoc::getFromPointer(Buffer.data()), SourceMgr::DK_Error,
Index: llvm/include/llvm/Support/FileCheck.h
===================================================================
--- llvm/include/llvm/Support/FileCheck.h
+++ llvm/include/llvm/Support/FileCheck.h
@@ -61,7 +61,10 @@
   CheckBadNot,
 
   /// Marks when parsing found a -COUNT directive with invalid count value.
-  CheckBadCount
+  CheckBadCount,
+
+  /// Marks when parsing found a directive followed by whitespace (not a colon).
+  CheckBadColon,
 };
 
 class FileCheckType {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D77227.254230.patch
Type: text/x-patch
Size: 2525 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200401/c43d2cb4/attachment.bin>