[FileCheck] Add check modifier capability

[jpienaar]

Summary

Introduce CHECK modifiers that change the behavior of the CHECK directive. Also add a LITERAL modifier for cases where matching could end requiring escaping strings interpreted as regex where only literal/fixed string matching is desired (where LITERAL would make the CHECK's easier to write, less fragile/difficult to interpret).

[thopre]

You should add examples for the other directives as well (CHECK-LITERAL-DAG, CHECK-LITERAL-NOT, CHECK-LITERAL-COUNT)

llvm/docs/CommandGuide/FileCheck.rst

663–669

Proposed rewording. If not convinced by it you should at least fix the syntax for mentioning a directive and the typo on "escaping".

llvm/include/llvm/FileCheck/FileCheck.h

79

I know that's what the above line did but I'd simply phrase it as per suggestion.

llvm/lib/FileCheck/FileCheck.cpp

1597–1600

Given how simple is this I would put it in the class declaration.

[thopre]

On a more general note, I wonder if having new directives is the right way to go. Why not introduce a concept of directive modifier? It could allow multiple modifier to cumulate. We could even introduce modifier in the middle of a directive at some point (though obviously not for a directive that was marked literal).

How about: CHECK{LITERAL}? Or perhaps CHECK{\L}. Other directives would follow naturally and CHECK-EMPTY{\L} would be a nop.

[jpienaar]

In D91761#2405139, @thopre wrote:

On a more general note, I wonder if having new directives is the right way to go. Why not introduce a concept of directive modifier? It could allow multiple modifier to cumulate. We could even introduce modifier in the middle of a directive at some point (though obviously not for a directive that was marked literal).

How about: CHECK{LITERAL}? Or perhaps CHECK{\L}. Other directives would follow naturally and CHECK-EMPTY{\L} would be a nop.

Good point, indirectly this is what this is doing, LITERAL is more a modifier than a directive (and yes CHECK-EMPTY is a nop - I was in between asserting that CHECK-LITERAL-EMPTY is not used, or just allowing it as it means the same). This pushes the modifier parsing to the end. Let me give that a go.

[jpienaar]

Swap it to be a directive & incorporate doc suggestions.

[probinson]

I have to say, I'm not super enthused about this feature. Typically we've added directives when there's no other way to achieve the desired effect; the LITERAL feature doesn't meet that bar. It's a way to simplify something that you could do already. Not to say I'm going to insist on killing it, but I would like to hear more justification.

Does this excessive-escaping situation come up very often in practice? I saw a relatively simple case recently, {{{.*}}} which is, a pair of braces with anything in between. (Which FileCheck doesn't parse in the intuitive way, but it does work as intended.) But that's about the most complicated thing I've come across lately.

[jpienaar]

In D91761#2407009, @probinson wrote:

I have to say, I'm not super enthused about this feature. Typically we've added directives when there's no other way to achieve the desired effect

SAME could be achieved without needing SAME, COUNT too, and NEXT could be folded in too by extending to multi-line regex and verifying no newlines so too DAG. So I believe there is a goal with directives to make checks readable for users & simple to interpret rather than just possible. Escaping is possible but obscures what is being tested and easy to get wrong.

Does this excessive-escaping situation come up very often in practice?

Very often indeed in MLIR as dense constants use [] pairs for values, so if you have a multi-dimensional tensor value you end up with a lot of escaping.

[probinson]

In D91761#2407164, @jpienaar wrote:

In D91761#2407009, @probinson wrote:

I have to say, I'm not super enthused about this feature. Typically we've added directives when there's no other way to achieve the desired effect

SAME could be achieved without needing SAME, COUNT too, and NEXT could be folded in too by extending to multi-line regex and verifying no newlines so too DAG. So I believe there is a goal with directives to make checks readable for users & simple to interpret rather than just possible. Escaping is possible but obscures what is being tested and easy to get wrong.

Fair. I'd forgotten about COUNT and that multi-line regexes are possible. (Although I will point out that SAME does not always do the same thing as pasting the directives together with .* separators; see my "FileCheck Follies" lightning talk from the Fall 2016 Dev Meeting.)

"Easy to get wrong" is a worthwhile point too.

Does this excessive-escaping situation come up very often in practice?

Very often indeed in MLIR as dense constants use [] pairs for values, so if you have a multi-dimensional tensor value you end up with a lot of escaping.

Okay.

[jpienaar]

In D91761#2408120, @probinson wrote:

Fair. I'd forgotten about COUNT and that multi-line regexes are possible. (Although I will point out that SAME does not always do the same thing as pasting the directives together with .* separators; see my "FileCheck Follies" lightning talk from the Fall 2016 Dev Meeting.)

Nice talk, I had not seen it before.

"Easy to get wrong" is a worthwhile point too.

Yes, I had a couple of O(10) chat messages exchange with different folks that were frustrated and thought "mmm, could we make this better/easier"

[jpienaar]

Fix missing :

[jhenderson]

llvm/docs/CommandGuide/FileCheck.rst

664

Fix this line to have the right number of ~ or the doc build will break. If you don't have it setup, I recommend you enable the docs build on your machine and build the html pages, to flush out any problems.

It would be nice to have an example of how to use the LITERAL modifier, both in conjunction with a normal check and a more complicated directive. I think this was there before, looking at the diff history, but it's disappeared again.

llvm/include/llvm/FileCheck/FileCheck.h

96

literal -> Literal, as per clang-tidy warning.

Same issue in various other cases in the new code (variables in the core LLVM project use UpperCase)

llvm/lib/FileCheck/FileCheck.cpp

1611

1612

Would it be simpler to just do Prefix + str + getModifiersDescription()? Not sure formatv really gets us anything here.

llvm/test/FileCheck/check-literal.txt

2

It would be nice to have a brief comment at the start of the test explaining what this test is actually testing (specifically, it's testing the literal modifier, and not something else that might be considered "literal").

You should probably have a case with a suffix following {LITERAL} to show the behaviour when the : doesn't immediately follow it. Also, where there's no closing } after LITERAL.

Is it worth checking the use with -NOT too? Not sure on that one, but maybe.

[jpienaar]

Fix naming conventions & expand test

[jpienaar]

Accidentally included unrelated change in previous diff

[jpienaar]

Fix missing empty line in rst

[jhenderson]

llvm/test/FileCheck/check-literal.txt

7

"but"?

23

I was thinking more that you need another FileCheck run which will fail because it is doing A-NOT{LITERAL} but found a match.

[jpienaar]

Fixing redundant word in test case and adding negative test.

[jhenderson]

llvm/test/FileCheck/check-literal.txt

4

Here and in other comments, I suggest using ;; for the comment marker - the double marker helps distinguish it from an actual FileCheck or lit directive.

We have tended to do that in some newer tests in the LLVM binutils at least, and I think others I've been involved in reviewing.

28–29

I think this needs %ProtectFileCheckOutput, but am not 100% sure if that applies for "not" cases (I think it does).

Also, I have a personal preference for continuation lines to be formatted as suggested in the inline edit. The idea is that it clearly shows from looking at the first line that you've got to the end of that command, whilst the second line is indented shwoing that it is a continuation.

[jpienaar]

Updated test comments to be ;; , using ProtectFileCheckOutput for piped case, switch | location for continuation

[jhenderson]

The implementation of the feature looks fine to me, but I'd like to defer to @thopre and @probinson on whether the feature actually gets accepted or not, so not marking as accepted myself.

[thopre]

Could you add some more tests:

• negative tests for the modifier syntax (e.g. "{LITERAL,}", "{}", "{LITTERAL}", "{LITERAL"
• positive tests with several modifiers (i.e. "{LITERAL,LITERAL}")

It's looking good otherwise. Thanks for making the code already prepared for multiple modifiers. I think we might want to have a shorter version of the modifier but better keep it for later when the need is felt.

llvm/docs/CommandGuide/FileCheck.rst

667

I suggest to match the formalism used to introduced numeric expression, as per edit

669

When there'll be more than one I'll suggest using a list but since there's only one supported value I propose folding it inside the text above (as per edit)

llvm/include/llvm/FileCheck/FileCheck.h

73

I'd remove the value, it'll automatically take the last enumerator + 1 and won't require changing for every new modifier.

llvm/lib/FileCheck/FileCheck.cpp

1668–1673

Perhaps allow whitespaces?

1679

This comment needs updating

llvm/test/FileCheck/check-literal.txt

31

It took me a while to parse that sentence but I'm not sure how to improve it. Any suggestion @jhenderson ?

Perhaps "This ensures an error is correctly reported when a NOT directive with a LITERAL modifier matches"?

[jpienaar]

Adding more negative tests, updating doc & comments, allow whitespace in modifier list.

[jhenderson]

llvm/lib/FileCheck/FileCheck.cpp

1683–1684

1685–1686

Looking at this again, I'm not sure these two should be folded together. I think it would make more sense to have the : check separate to the { check, and then not to call ConsumeModifiers in the event of the :, since there are none.

llvm/test/FileCheck/check-literal.txt

21

Also A{,LITERAL}?

31

Also add a space before the comma, so that the before and after aspects of the whitespace trimming are tested.

31

The latest version seems reasonable to me. I might suggest "an error" -> "a failure" since error could be implied to mean a problem (e.g. undefined variable) as opposed to a failed CHECK directive.

45

I'd make this more obvious, e.g.INVALID.

[jpienaar]

Add additional test & split checking for plain CHECK.

[jhenderson]

llvm/lib/FileCheck/FileCheck.cpp

1681–1682

Can you get rid of this check now?

llvm/test/FileCheck/check-literal.txt

46

Sorry, I was referring to the {LITTERAL} bit actually :)

The single character difference to LITERAL makes it look like there's a typo, and not a deliberate decision.

[jpienaar]

Change from LITTERAL to BADMODIFIER to make it clearer

[jpienaar]

llvm/lib/FileCheck/FileCheck.cpp

1681–1682

No because these are used by CHECK-NEXT and the rest, so I would need to repeat this for each of them there.

[jhenderson]

The test is currently failing on the pre-merge bot :-)

llvm/test/FileCheck/check-literal.txt

47

Should this be 'INVALID'?

[jpienaar]

D'oh fix incorrect typo in test testing typo :)

[jhenderson]

llvm/test/FileCheck/check-literal.txt

47

Any reason, you're not testing the closing '?

[jpienaar]

llvm/test/FileCheck/check-literal.txt

47

Yes, as it ends with :' which I feel could be improved (either by just dropping the : or also adding a regex for modifiers as optional there, preference towards the former at the moment). So wanted to just rest the essential part here and have the update of error message in follow up as it could affect more tests (e.g., make this and it more directed changes).

[jhenderson]

No more comments from me at this time, but as before, I'd prefer others to confirm they're happy with the concept.

llvm/test/FileCheck/check-literal.txt

47

Thanks for the explanation.

[thopre]

In D91761#2448777, @jhenderson wrote:

No more comments from me at this time, but as before, I'd prefer others to confirm they're happy with the concept.

I suggested making it a modifier rather than a new directive so I'm of course happy with it. Will let others speak up if they want but you're good to go from me

[jpienaar]

In D91761#2448795, @thopre wrote:

In D91761#2448777, @jhenderson wrote:

No more comments from me at this time, but as before, I'd prefer others to confirm they're happy with the concept.

I suggested making it a modifier rather than a new directive so I'm of course happy with it. Will let others speak up if they want but you're good to go from me

SG, I'll wait a couple of days to see if any other feedback and then proceed. Thanks

[jpienaar]

Rebase to trigger presubmit again

[jpienaar]

Closed in https://github.com/llvm/llvm-project/commit/44f399ccc12e27d20bae1ea7e712ef7f71e2ff3a (unfortunately omitted from commit description :-/)

llvm/docs/CommandGuide/FileCheck.rst

 failures to be detected in a single invocation.
 
 There is no requirement that ``CHECK-LABEL:`` directives contain strings that
 correspond to actual syntactic labels in a source or output language: they must
 simply uniquely match a single line in the file being verified.
 
 ``CHECK-LABEL:`` directives cannot contain variable definitions or uses.
 
+Directive modifiers
+~~~~~~~~~~~~~~~~~~~
+
+A directive modifier can be append to a directive by following the directive
+with ``{modifier}``. Where modifier can be one of the following:
+
+LITERAL
+  The ``LITERAL`` directive modifier can be used to perform a literal match. The
+  modifier results in the directive not recognizing any syntax to perform regex
+  matching, variable capture or any substitutions. This is useful when the text
+  to match would require excessive escaping otherwise. For example, the
+  following will perform literal matches rather than considering these as
+  regular expressions:
+
+  .. code-block:: text
+
+     Input: [[[10, 20]], [[30, 40]]]
+     Output %r10: [[10, 20]]
+     Output %r10: [[30, 40]]
+
+     ; CHECK{LITERAL}: [[[10, 20]], [[30, 40]]]
+     ; CHECK-DAG{LITERAL}: [[30, 40]]
+     ; CHECK-DAG{LITERAL}: [[10, 20]]
+
 FileCheck Regex Matching Syntax
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
 All FileCheck directives take a pattern to match.
 For most uses of FileCheck, fixed string matching is perfectly sufficient.  For
 some things, a more flexible form of matching is desired.  To support this,
 FileCheck allows you to specify regular expressions in matching strings,
 surrounded by double braces: ``{{yourregex}}``. FileCheck implements a POSIX

llvm/include/llvm/FileCheck/FileCheck.h

 
 #ifndef LLVM_FILECHECK_FILECHECK_H
 #define LLVM_FILECHECK_FILECHECK_H
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Support/SourceMgr.h"
+#include <bitset>
 #include <string>
 #include <vector>
 
 namespace llvm {
 
 /// Contains info about various FileCheck options.
 struct FileCheckRequest {
   std::vector<StringRef> CheckPrefixes;
enum FileCheckKind {
 
   /// Marks when parsing found a -NOT check combined with another CHECK suffix.
   CheckBadNot,
 
   /// Marks when parsing found a -COUNT directive with invalid count value.
   CheckBadCount
 };
 
+enum FileCheckKindModifier {
+  /// Modifies directive to perform literal match.
+  ModifierLiteral = 0,
+
+  // The number of modifier.
+  Size = 1
+};
+
 class FileCheckType {
   FileCheckKind Kind;
   int Count; ///< optional Count for some checks
+  /// Modifers for the check directive.
+  std::bitset<FileCheckKindModifier::Size> Modifiers;
 
 public:
-  FileCheckType(FileCheckKind Kind = CheckNone) : Kind(Kind), Count(1) {}
+  FileCheckType(FileCheckKind Kind = CheckNone)
+      : Kind(Kind), Count(1), Modifiers() {}
   FileCheckType(const FileCheckType &) = default;
   FileCheckType &operator=(const FileCheckType &) = default;
 
   operator FileCheckKind() const { return Kind; }
 
   int getCount() const { return Count; }
   FileCheckType &setCount(int C);
 
+  bool isLiteralMatch() const {
+    return Modifiers[FileCheckKindModifier::ModifierLiteral];
+  }
+  FileCheckType &setLiteralMatch(bool Literal = true) {
+    Modifiers.set(FileCheckKindModifier::ModifierLiteral, Literal);
+    return *this;
+  }
+
   // \returns a description of \p Prefix.
   std::string getDescription(StringRef Prefix) const;
+
+  // \returns a description of \p Modifiers.
+  std::string getModifiersDescription() const;
 };
 } // namespace Check
 
 /// Summary of a FileCheck diagnostic.
 struct FileCheckDiag {
   /// What is the FileCheck directive for this diagnostic?
   Check::FileCheckType CheckTy;
   /// Where is the FileCheck directive for this diagnostic?

llvm/lib/FileCheck/FileCheck.cpp

if (!PatternStr.empty() && CheckTy == Check::CheckEmpty) {
     return true;
   }
 
   if (CheckTy == Check::CheckEmpty) {
     RegExStr = "(\n$)";
     return false;
   }
 
+  // If literal check, set fixed string.
+  if (CheckTy.isLiteralMatch()) {
+    FixedStr = PatternStr;
+    return false;
+  }
+
   // Check to see if this is a fixed string, or if it has regex pieces.
   if (!MatchFullLinesHere &&
       (PatternStr.size() < 2 || (PatternStr.find("{{") == StringRef::npos &&
                                  PatternStr.find("[[") == StringRef::npos))) {
     FixedStr = PatternStr;
     return false;
   }
 

 Check::FileCheckType &Check::FileCheckType::setCount(int C) {
   assert(Count > 0 && "zero and negative counts are not supported");
   assert((C == 1 || Kind == CheckPlain) &&
          "count supported only for plain CHECK directives");
   Count = C;
   return *this;
 }
 
+std::string Check::FileCheckType::getModifiersDescription() const {
+  if (Modifiers.none())
+    return "";
+  std::string Ret;
+  raw_string_ostream OS(Ret);
+  OS << '{';
+  if (isLiteralMatch())
+    OS << "LITERAL";
+  OS << '}';
+  return OS.str();
+}
+
 std::string Check::FileCheckType::getDescription(StringRef Prefix) const {
+  // Append directive modifiers.
+  auto WithModifiers = [this, Prefix](StringRef Str) -> std::string {
+    return (Prefix + Str + getModifiersDescription()).str();
+  };
+
   switch (Kind) {
   case Check::CheckNone:
     return "invalid";
   case Check::CheckPlain:
     if (Count > 1)
-      return Prefix.str() + "-COUNT";
-    return std::string(Prefix);
+      return WithModifiers("-COUNT");
+    return WithModifiers("");
   case Check::CheckNext:
-    return Prefix.str() + "-NEXT";
+    return WithModifiers("-NEXT");
   case Check::CheckSame:
-    return Prefix.str() + "-SAME";
+    return WithModifiers("-SAME");
   case Check::CheckNot:
-    return Prefix.str() + "-NOT";
+    return WithModifiers("-NOT");
   case Check::CheckDAG:
-    return Prefix.str() + "-DAG";
+    return WithModifiers("-DAG");
   case Check::CheckLabel:
-    return Prefix.str() + "-LABEL";
+    return WithModifiers("-LABEL");
   case Check::CheckEmpty:
-    return Prefix.str() + "-EMPTY";
+    return WithModifiers("-EMPTY");
   case Check::CheckComment:
     return std::string(Prefix);
   case Check::CheckEOF:
     return "implicit EOF";
   case Check::CheckBadNot:
     return "bad NOT";
   case Check::CheckBadCount:
     return "bad COUNT";
   }
   llvm_unreachable("unknown FileCheckType");
 }
 
 static std::pair<Check::FileCheckType, StringRef>
 FindCheckType(const FileCheckRequest &Req, StringRef Buffer, StringRef Prefix) {
   if (Buffer.size() <= Prefix.size())
     return {Check::CheckNone, StringRef()};
 
-  char NextChar = Buffer[Prefix.size()];
-
-  StringRef Rest = Buffer.drop_front(Prefix.size() + 1);
-
+  StringRef Rest = Buffer.drop_front(Prefix.size());
   // Check for comment.
   if (llvm::is_contained(Req.CommentPrefixes, Prefix)) {
-    if (NextChar == ':')
+    if (Rest.consume_front(":"))
       return {Check::CheckComment, Rest};
     // Ignore a comment prefix if it has a suffix like "-NOT".
     return {Check::CheckNone, StringRef()};
   }
 
+  auto ConsumeModifiers = [&](Check::FileCheckType Ret)
+      -> std::pair<Check::FileCheckType, StringRef> {
+    if (Rest.consume_front(":"))
+      return {Ret, Rest};
+    if (!Rest.consume_front("{"))
+      return {Check::CheckNone, StringRef()};
+
+    // Parse the modifiers, speparated by commas.
+    do {
+      if (Rest.consume_front("LITERAL"))
+        Ret.setLiteralMatch();
+      else
+        return {Check::CheckNone, Rest};
+    } while (Rest.consume_front(","));
+    if (!Rest.consume_front("}:"))
+      return {Check::CheckNone, Rest};
+    return {Ret, Rest};
+  };
+
   // Verify that the : is present after the prefix.
-  if (NextChar == ':')
-    return {Check::CheckPlain, Rest};
+  if (Rest.front() == ':' || Rest.front() == '{')
+    return ConsumeModifiers(Check::CheckPlain);
 
-  if (NextChar != '-')
+  if (!Rest.consume_front("-"))
     return {Check::CheckNone, StringRef()};
 
   if (Rest.consume_front("COUNT-")) {
     int64_t Count;
     if (Rest.consumeInteger(10, Count))
       // Error happened in parsing integer.
       return {Check::CheckBadCount, Rest};
     if (Count <= 0 || Count > INT32_MAX)
       return {Check::CheckBadCount, Rest};
-    if (!Rest.consume_front(":"))
+    if (Rest.front() != ':' && Rest.front() != '{')
       return {Check::CheckBadCount, Rest};
-    return {Check::FileCheckType(Check::CheckPlain).setCount(Count), Rest};
+    return ConsumeModifiers(
+        Check::FileCheckType(Check::CheckPlain).setCount(Count));
   }
 
-  if (Rest.consume_front("NEXT:"))
-    return {Check::CheckNext, Rest};
-
-  if (Rest.consume_front("SAME:"))
-    return {Check::CheckSame, Rest};
-
-  if (Rest.consume_front("NOT:"))
-    return {Check::CheckNot, Rest};
-
-  if (Rest.consume_front("DAG:"))
-    return {Check::CheckDAG, Rest};
-
-  if (Rest.consume_front("LABEL:"))
-    return {Check::CheckLabel, Rest};
-
-  if (Rest.consume_front("EMPTY:"))
-    return {Check::CheckEmpty, Rest};
-
   // 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:") ||
       Rest.startswith("EMPTY-NOT:") || Rest.startswith("NOT-EMPTY:"))
     return {Check::CheckBadNot, Rest};
 
+  if (Rest.consume_front("NEXT"))
+    return ConsumeModifiers(Check::CheckNext);
+
+  if (Rest.consume_front("SAME"))
+    return ConsumeModifiers(Check::CheckSame);
+
+  if (Rest.consume_front("NOT"))
+    return ConsumeModifiers(Check::CheckNot);
+
+  if (Rest.consume_front("DAG"))
+    return ConsumeModifiers(Check::CheckDAG);
+
+  if (Rest.consume_front("LABEL"))
+    return ConsumeModifiers(Check::CheckLabel);
+
+  if (Rest.consume_front("EMPTY"))
+    return ConsumeModifiers(Check::CheckEmpty);
+
   return {Check::CheckNone, Rest};
 }
 
 // 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;

llvm/test/FileCheck/check-literal.txt

+; RUN: FileCheck -check-prefix=A -input-file %s %s
+
+;; This tests the LITERAL directive modifier.
+
+; A{LITERAL} is not incorrectly flagged. Neither is
+; A{LITERAL
+
+The result is "5371, 5372, 5373, 5374"
+
+The result is "[[[5371]], [[5372]], [[5373]], [[5374]]]"
+{{there you go.*}}
+
+[[10]]
+[[20]]
+[[50]]
+
+; A: 5371, 5372,
+; A-SAME: 5373, 5374
+; A{LITERAL}: [[[5371]], [[5372]],
+; A-SAME{LITERAL}: [[5373]], [[5374]]]
+; A-NEXT{LITERAL}: {{there you go.*}}
+; A-NOT{LITERAL}: [[50]]
+; A-DAG{LITERAL}: [[20]]
+; A-DAG{LITERAL}: [[10]]
+; A{LITERAL}: [[50]]
+
+; RUN: %ProtectFileCheckOutput \
+; RUN: not FileCheck %s --input-file %s --check-prefix=CHECK-ERRNOT 2>&1 | \
+; RUN:   FileCheck %s --check-prefix=ERRNOT
+
+;; This ensures LITERAL applied to not in error cases reports an error.
+
+[[a]]
+[[b]]
+[[c]]
+
+; CHECK-ERRNOT{LITERAL}: [[a]]
+; CHECK-ERRNOT-NOT{LITERAL}: [[b]]
+; CHECK-ERRNOT{LITERAL}: [[c]]
+; ERRNOT: no match expected