FileCheck Enhancement - CHECK-WORD

[eklepilkina]

Summary

This patch adds new directives CHECK-WORD, CHECK-WORD-NEXT, CHECK-WORD-NOT, CHECK-WORD-SAME, CHECK-WORD-DAG, which are like standart directives, but they match only when string is a separate word.

[jyknight]

I don't really like adding a whole new set of CHECK-things for word-matching.

I think either users should use "\b" to check that the edge of the match is at the beginning/end of a word, or else make word matching the default and add \w* to the CHECKs that actually *WANT* to match subword. I'd expect there to be few which really ought to be doing that, but probably more that do accidentally.

[eklepilkina]

"I think either users should use "\b" to check that the edge of the match is at the beginning/end of a word" - Problem is that current regexp library doesn't support \b assertion. And question about changing library appears again.
I think, if change the meaning of CHECK there will be a lot of work of changing LLVM tests and private tests of FileCheck users.

[vsk]

In D22353#485146, @eklepilkina wrote:

"I think either users should use "\b" to check that the edge of the match is at the beginning/end of a word" - Problem is that current regexp library doesn't support \b assertion. And question about changing library appears again.
I think, if change the meaning of CHECK there will be a lot of work of changing LLVM tests and private tests of FileCheck users.

+1, I'd rather not have to go fix up tests which rely on the sub-word matching behavior of CHECK.

@eklepilkina: Thanks for working on this! There are a few spots in this patch that need clang-formatting. Also, please upload diffs with more context (e.g git diff -U1000 ...). I just have a few minor comments --

test/FileCheck/check-word.txt

15

Could you add:

// RUN: not FileCheck -input-file %s %s -check-prefix=FAILTEST
first
and
// FAILTEST-WORD: first
// FAILTEST-WORD-SAME: and

The failtest should probably be extended to cover more cases.

utils/FileCheck/FileCheck.cpp

1149

nit, capitalize 'if' and add a space before it.

1157

Why do Match{Pos,Len} need to be passed in by reference?

1165

Does this guarantee that there cannot be a newline between CHECK-SAME-WORD matches?

[probinson]

In D22353#485146, @eklepilkina wrote:

"I think either users should use "\b" to check that the edge of the match is at the beginning/end of a word" - Problem is that current regexp library doesn't support \b assertion. And question about changing library appears again.

Well, the regex implementation does live in the Support library, even if it did come from somewhere else originally; we can modify it. A word-break feature seems useful in more contexts than just CHECK-WORD.

I think, if change the meaning of CHECK there will be a lot of work of changing LLVM tests and private tests of FileCheck users.

We won't know until somebody tries. It should not be too hard to do the experiment, I've done that kind of thing twice already.

[eklepilkina]

utils/FileCheck/FileCheck.cpp

1165

Newline between CHECK-SAME-WORD matches checks separately as usual CHECK-SAME.
I added suggested test and it works. As I understood it checks such situation.

[eklepilkina]

In D22353#485513, @probinson wrote:

We won't know until somebody tries. It should not be too hard to do the experiment, I've done that kind of thing twice already.

Ok, I can do such experiment, but this experiment will be only with LLVM and clang tests. May be there are a few tests. But how can I guarantee that users have no a lot of such tests in private repositories?

[eklepilkina]

There is a suggestion to add new option, which will change standart behaviour to word-matchinf instead of addding new set of CHECKs directives. What about this idea?

[Meinersbur]

I am in support of such a switch. As already mentioned that once I got bitten that

; CHECK: br

to check for a branch instruction also matched unrelated lines eg.

%2 = add i32 %subregion, i32 5

Such an option would lower the probability of such mismatches. I'd prefer this mode for all my tests but introducing an opt-in switch for compatibility is a good idea.

[eklepilkina]

Replace set of new CHECK directives to new -check-word option

[eklepilkina]

I made new mode and made experiment turned it on for all LLVM tests.
Result:

Expected Passes    : 15810
Expected Failures  : 125
Unsupported Tests  : 195
Unexpected Passes  : 4
Unexpected Failures: 1128

In my opinion, there is quite big number of failed tests with new behavior. So doing this word-match behavior standart won't be simple task.

[aprantl]

I don't think making this the default behavior is a good idea. This may introduce hard-to-find bugs to every testcase (because they were written written with the old behavior in mind).
For example, how would we even find problems with CHECK-NOT; the new behavior will make the expressions stricter, but we wouldn't even realize it because we're expecting no match. The only way this could be made the default behavior would be to change all existing CHECKs to explicitly use the old behavior. This would still break every out-of-tree testcase, but that is admittedly a lesser concern.

[alexfh]

Do I understand correctly, that this patch makes writing single-word check lines easier? Why not add more words to check lines to make them more strict? BTW, does FileCheck support {{\b}}?

[alexfh]

In D22353#535691, @alexfh wrote:

Do I understand correctly, that this patch makes writing single-word check lines easier? Why not add more words to check lines to make them more strict? BTW, does FileCheck support {{\b}}?

Previous comments answer the last question. So is it feasible to add support for \b to LLVM regexps?

[eklepilkina]

In D22353#535691, @alexfh wrote:

Why not add more words to check lines to make them more strict? BTW, does FileCheck support {{\b}}?

This patch adds

// CHECK-WORD:
// CHECK-WORD-NEXT:
// CHECK-WORD-SAME:
// CHECK-WORD-DAG:
// CHECK-WORD-NOT:

What words do you suggest to add?

So is it feasible to add support for \b to LLVM regexps?

The regexp library is very old. I asked several times about changing it. But I haven't got any answer. There are a lot of other unsupported features of regexs in this library and which can be useful.

[aprantl]

In D22353#536766, @eklepilkina wrote:

In D22353#535691, @alexfh wrote:

Why not add more words to check lines to make them more strict? BTW, does FileCheck support {{\b}}?
So is it feasible to add support for \b to LLVM regexps?

The regexp library is very old. I asked several times about changing it. But I haven't got any answer. There are a lot of other unsupported features of regexs in this library and which can be useful.

I think it would be worthwhile to bring this up again. What are our options with the regex library? Is there a newer version of the one we are currently using that we could upgrade to (and that supports \b)? How much work would it be to implement support for \b ourselves?

[eklepilkina]

What are our options with the regex library? Is there a newer version of the one we are currently using that we could upgrade to (and that supports \b)?

As I understood used regexp library is version of OpenBSD library. I couldn't find some new versions.
I don't know a lot about modern C++ regexp library, but there are:

1. PCRE(pcre.h)
2. There is regex library in boost. But boost is too big
3. Also there is regex library in poco.

May be there are people who has some experience with C++ regexs libraries.

How much work would it be to implement support for \b ourselves?

I can't give exact estimate. I inly had a look on its source code. But I think that it's wrong to add features support. There are a lot of other useful feautures, for example complex assertions, conditional subexpressions and etc., that can be used. Then if somebody want them he should implement them by himself.

[alexfh]

In D22353#541171, @eklepilkina wrote:

What are our options with the regex library? Is there a newer version of the one we are currently using that we could upgrade to (and that supports \b)?

As I understood used regexp library is version of OpenBSD library. I couldn't find some new versions.
I don't know a lot about modern C++ regexp library, but there are:

1. PCRE(pcre.h)
2. There is regex library in boost. But boost is too big
3. Also there is regex library in poco.

FWIW, there's also re2 (https://github.com/google/re2).

May be there are people who has some experience with C++ regexs libraries.

How much work would it be to implement support for \b ourselves?

I can't give exact estimate. I inly had a look on its source code. But I think that it's wrong to add features support. There are a lot of other useful feautures, for example complex assertions, conditional subexpressions and etc., that can be used. Then if somebody want them he should implement them by himself.

In D22353#536766, @eklepilkina wrote:

In D22353#535691, @alexfh wrote:

Why not add more words to check lines to make them more strict?

This patch adds

// CHECK-WORD:
// CHECK-WORD-NEXT:
// CHECK-WORD-SAME:
// CHECK-WORD-DAG:
// CHECK-WORD-NOT:

What words do you suggest to add?

I'm suggesting to make patterns used in CHECK-lines stricter by adding more text to them instead of allowing to match on word boundaries using the new CHECK-directives. E.g. the line you want to check is

op1;

So instead of using CHECK-WORD: op1, you could make the pattern stricter by adding more context: CHECK: {{^}}op1;{{$}}. Do you have specific examples of output and patterns, where this solution is undesired for some reason?

Overall, I think the FileCheck utility is already confusing enough and many developers attribute some magic properties to it (that it can somehow implicitly connect position of a CHECK line to the position of the relevant line in the input, etc.). Adding more features like CHECK-WORD only makes this worse.

So is it feasible to add support for \b to LLVM regexps?

The regexp library is very old. I asked several times about changing it. But I haven't got any answer. There are a lot of other unsupported features of regexs in this library and which can be useful.

[Meinersbur]

In D22353#542382, @alexfh wrote:

So instead of using CHECK-WORD: op1, you could make the pattern stricter by adding more context: CHECK: {{^}}op1;{{$}}. Do you have specific examples of output and patterns, where this solution is undesired for some reason?

Most of the time it is not desirable to add the remainder of the line to be checked as well. It could be very long, or just contain irrelevant information (such as the branch sources following basic block names)

[eklepilkina]

As I understood most of users prefer to change library to have support of \b assert. But, I think, this decision is quite important and can be made only by he contributor of FileCheck. I don't know how this suggestion can be done. Or I can try to change library and create a patch with this changes?

[aprantl]

I think the two most compelling options are:

1. Implement support for \b in llvm's regex library.
2. Go with Duncan's suggestion and investigate whether the C++11 regex library can replace LLVM's posix regex library entirely so we can drop it from LLVM.

[eklepilkina]

Ok, I'll try to use STL regex.

[eklepilkina]

There is problem with STL, their realization of regular expressions doesn't match start and end of line in basic mode and there is no multiline mode. There is the issue 2343 in http://cplusplus.github.io/LWG/lwg-toc.html.
There is opportunity to split text by lines and match each line separately, but I think this is a hack, and I think it'll slow FileCheck.

[alexfh]

In D22353#541171, @eklepilkina wrote:

What are our options with the regex library? Is there a newer version of the one we are currently using that we could upgrade to (and that supports \b)?

As I understood used regexp library is version of OpenBSD library. I couldn't find some new versions.
I don't know a lot about modern C++ regexp library, but there are:

1. PCRE(pcre.h)
2. There is regex library in boost. But boost is too big
3. Also there is regex library in poco.

FWIW, there's also re2 (https://github.com/google/re2).

May be there are people who has some experience with C++ regexs libraries.

How much work would it be to implement support for \b ourselves?

I can't give exact estimate. I inly had a look on its source code. But I think that it's wrong to add features support. There are a lot of other useful feautures, for example complex assertions, conditional subexpressions and etc., that can be used. Then if somebody want them he should implement them by himself.

In D22353#536766, @eklepilkina wrote:

In D22353#535691, @alexfh wrote:

Why not add more words to check lines to make them more strict?

This patch adds

// CHECK-WORD:
// CHECK-WORD-NEXT:
// CHECK-WORD-SAME:
// CHECK-WORD-DAG:
// CHECK-WORD-NOT:

What words do you suggest to add?

I'm suggesting to make patterns used in CHECK-lines stricter by adding more text to them instead of allowing to match on word boundaries using the new CHECK-directives. E.g. the line you want to check is

op1;

So instead of using CHECK-WORD: op1, you could make the pattern stricter by adding more context: CHECK: {{^}}op1;{{$}}. Do you have specific examples of output and patterns, where this solution is undesired for some reason?

Overall, I think the FileCheck utility is already confusing enough and many developers attribute some magic properties to it (that it can somehow implicitly connect position of a CHECK line to the position of the relevant line in the input, etc.). Adding more features like CHECK-WORD only makes this worse.

So is it feasible to add support for \b to LLVM regexps?

The regexp library is very old. I asked several times about changing it. But I haven't got any answer. There are a lot of other unsupported features of regexs in this library and which can be useful.

In D22353#545957, @eklepilkina wrote:

There is problem with STL, their realization of regular expressions doesn't match start and end of line in basic mode and there is no multiline mode. There is the issue 2343 in http://cplusplus.github.io/LWG/lwg-toc.html.
There is opportunity to split text by lines and match each line separately, but I think this is a hack, and I think it'll slow FileCheck.

Not sure, whether adding \b support to the regex library used in LLVM is more effort, but there's another option here: take the lib++s regex header with the multiline option added (as per http://cplusplus.github.io/LWG/lwg-active.html#2503) and use a private copy of it (with proper namespace modifications and whatever else is needed to make it actually private. WDYT?

[alexfh]

Looks like a few older comments were posted again. Please ignore duplicates.

test/FileCheck/check-word.txt

+// RUN: FileCheck -input-file %s %s
+
+op1;
+op2;
+op33;
+// CHECK-WORD: op{{[0-9]}};
+// CHECk-WORD-NEXT: op{{[0-9]}};
+// CHECk-WORD-NOT: op{{[0-9]}}
+
+first and
+// CHECK-WORD: first
+// CHECK-WORD-SAME: and
+This is second line
+// CHECK-WORD: second
+
+one two three
+// CHECK-WORD-DAG: two
+// CHECK-WORD-DAG: three
+// CHECK-WORD-DAG: one
+
+four!
+// CHECK-WORD: four
+dog
+line
+// CHECK-WORD-NOT: cat
+// CHECK: line
 No newline at end of file

utils/FileCheck/FileCheck.cpp

     CheckNot,
     CheckDAG,
     CheckLabel,
+    CheckWord,
+    CheckWordNext,
+    CheckWordSame,
+    CheckWordNot,
+    CheckWordDAG,
 
     /// MatchEOF - When set, this pattern only matches the end of file. This is
     /// used for trailing CHECK-NOTs.

                            StringRef Prefix,
                            SourceMgr &SM,
                            unsigned LineNumber) {
-  bool MatchFullLinesHere = MatchFullLines && CheckTy != Check::CheckNot;
+  bool MatchFullLinesHere = MatchFullLines && CheckTy != Check::CheckNot &&
+                            CheckTy != Check::CheckWordNot;
 
   this->LineNumber = LineNumber;
   PatternLoc = SMLoc::getFromPointer(PatternStr.data());

   size_t CheckDag(const SourceMgr &SM, StringRef Buffer,
                   std::vector<const Pattern *> &NotStrings,
                   StringMap<StringRef> &VariableTable) const;
+
+  /// CheckWord - Verify there is a single word.
+  bool CheckWord(const size_t &MatchPos, const size_t &MatchLen, StringRef Buffer) const;
 };
 
 /// Canonicalize whitespaces in the input file. Line endings are replaced

   case Check::CheckLabel:
     return sizeof("-LABEL:") - 1;
 
+  case Check::CheckWord:
+    return sizeof("-WORD:") - 1;
+
+  case Check::CheckWordNext:
+    return sizeof("-WORD-NEXT:") - 1;
+
+  case Check::CheckWordSame:
+    return sizeof("-WORD-SAME:") - 1;
+
+  case Check::CheckWordNot:
+    return sizeof("-WORD-NOT:") - 1;
+
+  case Check::CheckWordDAG:
+    return sizeof("-WORD-DAG:") - 1;
+
   case Check::CheckEOF:
     llvm_unreachable("Should not be using EOF size");
   }

   if (Rest.startswith("LABEL:"))
     return Check::CheckLabel;
 
+  if (Rest.startswith("WORD:"))
+    return Check::CheckWord;
+
+  if (Rest.startswith("WORD-NEXT:"))
+    return Check::CheckWordNext;
+
+  if (Rest.startswith("WORD-SAME:"))
+    return Check::CheckWordSame;
+
+  if (Rest.startswith("WORD-NOT:"))
+    return Check::CheckWordNot;
+
+  if (Rest.startswith("WORD-DAG:"))
+    return Check::CheckWordDAG;
+
   // 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:") ||

     Buffer = Buffer.substr(EOL);
 
     // Verify that CHECK-NEXT lines have at least one CHECK line before them.
-    if ((CheckTy == Check::CheckNext || CheckTy == Check::CheckSame) &&
+    if ((CheckTy == Check::CheckNext || CheckTy == Check::CheckSame ||
+         CheckTy == Check::CheckWordNext || CheckTy == Check::CheckWordSame) &&
         CheckStrings.empty()) {
-      StringRef Type = CheckTy == Check::CheckNext ? "NEXT" : "SAME";
+      StringRef Type = CheckTy == Check::CheckNext || Check::CheckWordNext ? "NEXT" : "SAME";
       SM.PrintMessage(SMLoc::getFromPointer(UsedPrefixStart),
                       SourceMgr::DK_Error,
                       "found '" + UsedPrefix + "-" + Type + "' without previous '"

     }
 
     // Handle CHECK-DAG/-NOT.
-    if (CheckTy == Check::CheckDAG || CheckTy == Check::CheckNot) {
+    if (CheckTy == Check::CheckDAG || CheckTy == Check::CheckNot ||
+          CheckTy == Check::CheckWordDAG || CheckTy == Check::CheckWordNot) {
       DagNotMatches.push_back(P);
       continue;
     }

     // skipped region.
     if (CheckNot(SM, SkippedRegion, NotStrings, VariableTable))
       return StringRef::npos;
+
+    //if this check is a "CHECK-WORD", verify the bounds of string.
+    if (CheckWord(LastPos + MatchPos, MatchLen, Buffer))
+      return StringRef::npos;
   }
 
   return LastPos + MatchPos;
 }
 
+bool CheckString::CheckWord(const size_t &MatchPos, const size_t &MatchLen, StringRef Buffer) const {
+  bool isWord = true;
+  if (Pat.getCheckTy() != Check::CheckWord && Pat.getCheckTy() != Check::CheckWordNext &&
+      Pat.getCheckTy() != Check::CheckWordSame && Pat.getCheckTy() != Check::CheckWordDAG &&
+      Pat.getCheckTy() != Check::CheckWordNot)
+    return false;
+  // Check previous and next characters to understand word or not.
+  if (MatchPos != 0) {
+    if (!isspace(Buffer[MatchPos - 1]) && !ispunct(Buffer[MatchPos - 1])) {
+      isWord = false;
+    }
+  }
+  if (MatchPos + MatchLen < Buffer.size() && 
+      !isspace(Buffer[MatchPos + MatchLen ]) && !ispunct(Buffer[MatchPos + MatchLen])) {
+    isWord = false;
+  }
+  return (Pat.getCheckTy() == Check::CheckWordNot && isWord) ||
+         (Pat.getCheckTy() != Check::CheckWordNot && !isWord);
+}
+
 bool CheckString::CheckNext(const SourceMgr &SM, StringRef Buffer) const {
-  if (Pat.getCheckTy() != Check::CheckNext)
+  if (Pat.getCheckTy() != Check::CheckNext && Pat.getCheckTy() != Check::CheckWordNext)
     return false;
 
   // Count the number of newlines between the previous match and this one.

 }
 
 bool CheckString::CheckSame(const SourceMgr &SM, StringRef Buffer) const {
-  if (Pat.getCheckTy() != Check::CheckSame)
+  if (Pat.getCheckTy() != Check::CheckSame && Pat.getCheckTy() != Check::CheckWordSame)
     return false;
 
   // Count the number of newlines between the previous match and this one.

                            const std::vector<const Pattern *> &NotStrings,
                            StringMap<StringRef> &VariableTable) const {
   for (const Pattern *Pat : NotStrings) {
-    assert((Pat->getCheckTy() == Check::CheckNot) && "Expect CHECK-NOT!");
+    assert((Pat->getCheckTy() == Check::CheckNot || Pat->getCheckTy() == Check::CheckWordNot) &&
+            "Expect CHECK-NOT!");
 
     size_t MatchLen = 0;
     size_t Pos = Pat->Match(Buffer, MatchLen, VariableTable);

 
   for (const Pattern &Pat : DagNotStrings) {
     assert((Pat.getCheckTy() == Check::CheckDAG ||
-            Pat.getCheckTy() == Check::CheckNot) &&
+            Pat.getCheckTy() == Check::CheckNot ||
+            Pat.getCheckTy() == Check::CheckWordDAG ||
+            Pat.getCheckTy() == Check::CheckWordNot) &&
            "Invalid CHECK-DAG or CHECK-NOT!");
 
-    if (Pat.getCheckTy() == Check::CheckNot) {
+    if (Pat.getCheckTy() == Check::CheckNot || Pat.getCheckTy() == Check::CheckWordNot) {
       NotStrings.push_back(&Pat);
       continue;
     }
 
-    assert((Pat.getCheckTy() == Check::CheckDAG) && "Expect CHECK-DAG!");
+    assert((Pat.getCheckTy() == Check::CheckDAG || Pat.getCheckTy() == Check::CheckWordDAG) && 
+            "Expect CHECK-DAG!");
 
     size_t MatchLen = 0, MatchPos;