FileCheck [3/12]: Stricter parsing of @LINE expressions

[thopre]

Summary

This patch is part of a patch series to add support for FileCheck
numeric expressions. This specific patch gives earlier and better
diagnostics for the @LINE expressions.

Rather than detect parsing errors at matching time, this commit adds
enhance parsing to detect issues with @LINE expressions at parse time
and diagnose them more accurately.

Copyright:

• Linaro (changes up to diff 183612 of revision D55940)
• GraphCore (changes in later versions of revision D55940 and in new revision created off D55940)

[thopre]

Use camel case for new and modified functions

[thopre]

Rebase

[thopre]

• Fix detection of unexpected whitespace in @LINE expressions
• Make numeric value constructor explicit

[arsenm]

llvm/lib/Support/FileCheck.cpp

88

I thought these used Twine, not std::string?

[thopre]

Rebase

[thopre]

llvm/lib/Support/FileCheck.cpp

88

It is but there is an implicit conversion from the latter to the former. Using Twine directly makes the diagnostic string less readable due to akward concatenation:

Twine("Unsupported numeric operation '").concat(Twine(Operator)).concat("'")

I feel the current code provides better readability.

[probinson]

llvm/lib/Support/FileCheck.cpp

60

I'd use normal comments not doxygen for a small static helper like this.

68

Please put doxygen for new APIs in the header not the implementation file. (Eventually the doxygen for all the methods should be in the header, but that's for a later cleanup.)

[thopre]

Add unit testing

[arichardson]

llvm/lib/Support/FileCheck.cpp

59

I guess this should be S

88

I think StringRef("Unsupported numeric operation '") + Operator + "'" should also work and won't create a copy for the std::string.

[thopre]

Concatenate diagnostic message via StringRef and Twine rather than std::string

[thopre]

• Fix codestyle issues
• Avoid concatenation when printing diagnostics

[thopre]

Rebase on top of latest changes

[probinson]

LGTM

[jhenderson]

llvm/lib/Support/FileCheck.cpp

59

next is a very unspecific function name to me, and given this is quite a large file, a slightly more specific name might be clearer, e.g. "popFront" or similar.

llvm/unittests/Support/FileCheckTest.cpp

100

parseExpect?

[thopre]

• Rename next to popFront
• Rename parse_expect to parseExpect to follow naming convention

[rnk]

The newly added test fails on Windows:
http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/6381

This appears to be the fix:

bool parseExpect(std::string &VarName, std::string &Trailer) {

• StringRef NameTrailer = StringRef(VarName + Trailer);

+ std::string NameTrailer = VarName + Trailer;

[thopre]

In D60383#1482917, @rnk wrote:

The newly added test fails on Windows:
http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/6381

This appears to be the fix:

bool parseExpect(std::string &VarName, std::string &Trailer) {

• StringRef NameTrailer = StringRef(VarName + Trailer);

+ std::string NameTrailer = VarName + Trailer;

Sadly I cannot reproduce it. I tried a stage2 build just in case but I still don't hit it. I'm also a bit confused by what is happening. Does the temporary std::string created by the + operator dies before NameTrailer is used? Anyway, if your patch works could you try std::string &NameTrailer = VarName + Trailer instead? I believe that removes an unnecessary copy from the temporary std::string created by the + operator to the NameTrailer one.

Thanks for the patch!

[thopre]

In D60383#1483748, @thopre wrote:

In D60383#1482917, @rnk wrote:

The newly added test fails on Windows:
http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/6381

This appears to be the fix:

bool parseExpect(std::string &VarName, std::string &Trailer) {

• StringRef NameTrailer = StringRef(VarName + Trailer);

+ std::string NameTrailer = VarName + Trailer;

Sadly I cannot reproduce it. I tried a stage2 build just in case but I still don't hit it. I'm also a bit confused by what is happening. Does the temporary std::string created by the + operator dies before NameTrailer is used? Anyway, if your patch works could you try std::string &NameTrailer = VarName + Trailer instead? I believe that removes an unnecessary copy from the temporary std::string created by the + operator to the NameTrailer one.

Thanks for the patch!

Oh I'm always doing Debug builds so probably at -O0 the std::string is actually created and stays valid when it's copied in the memory buffer while in a release build the compiler might not even create the temporary concatenated string or destroys it quickly.

[thopre]

In D60383#1483748, @thopre wrote:

In D60383#1482917, @rnk wrote:

The newly added test fails on Windows:
http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/6381

This appears to be the fix:

bool parseExpect(std::string &VarName, std::string &Trailer) {

• StringRef NameTrailer = StringRef(VarName + Trailer);

+ std::string NameTrailer = VarName + Trailer;

Sadly I cannot reproduce it. I tried a stage2 build just in case but I still don't hit it. I'm also a bit confused by what is happening. Does the temporary std::string created by the + operator dies before NameTrailer is used? Anyway, if your patch works could you try std::string &NameTrailer = VarName + Trailer instead? I believe that removes an unnecessary copy from the temporary std::string created by the + operator to the NameTrailer one.

Thanks for the patch!

Oh sorry realized the patch is already committed and yes making it a reference does not work. Thanks Reid!

llvm/include/llvm/Support/FileCheck.h

explicit FileCheckPattern(Check::FileCheckType Ty,
       : Context(Context), CheckTy(Ty) {}
 
   /// Returns the location in source code.
   SMLoc getLoc() const { return PatternLoc; }
 
   /// Returns the pointer to the global state for all patterns in this
   /// FileCheck instance.
   FileCheckPatternContext *getContext() const { return Context; }
+
   /// Return whether \p is a valid first character for a variable name.
   static bool isValidVarNameStart(char C);
   /// Verify that the string at the start of \p Str is a well formed variable.
   /// Return false if it is and set \p IsPseudo to indicate if it is a pseudo
   /// variable and \p TrailIdx to the position of the last character that is
   /// part of the variable name. Otherwise, only return true.
   static bool parseVariable(StringRef Str, bool &IsPseudo, unsigned &TrailIdx);
+  /// Parse a numeric expression involving pseudo variable \p Name with the
+  /// string corresponding to the operation being performed in \p Trailer.
+  /// Return whether parsing failed in which case errors are reported on \p SM.
+  bool parseExpression(StringRef Name, StringRef Trailer,
+                       const SourceMgr &SM) const;
   bool ParsePattern(StringRef PatternStr, StringRef Prefix, SourceMgr &SM,
                     unsigned LineNumber, const FileCheckRequest &Req);
   size_t match(StringRef Buffer, size_t &MatchLen) const;
   /// Print value of successful substitutions or name of undefined pattern
   /// variables preventing such a successful substitution.
   void printVariableUses(const SourceMgr &SM, StringRef Buffer,
                          SMRange MatchRange = None) const;
   void printFuzzyMatch(const SourceMgr &SM, StringRef Buffer,
                        std::vector<FileCheckDiag> *Diags) const;
 
   bool hasVariable() const {
     return !(VariableUses.empty() && VariableDefs.empty());
   }
 
   Check::FileCheckType getCheckTy() const { return CheckTy; }
 
   int getCount() const { return CheckTy.getCount(); }
 
 private:
   bool AddRegExToRegEx(StringRef RS, unsigned &CurParen, SourceMgr &SM);
   void AddBackrefToRegEx(unsigned BackrefNum);
   unsigned computeMatchDistance(StringRef Buffer) const;
-  bool EvaluateExpression(StringRef Expr, std::string &Value) const;
+  void evaluateExpression(StringRef Expr, std::string &Value) const;
   size_t FindRegexVarEnd(StringRef Str, SourceMgr &SM);
 };
 
 //===----------------------------------------------------------------------===//
 /// Summary of a FileCheck diagnostic.
 //===----------------------------------------------------------------------===//
 
 struct FileCheckDiag {

llvm/lib/Support/FileCheck.cpp

if (Str[I] != '_' && !isalnum(Str[I]))
       break;
     ParsedOneChar = true;
   }
 
   TrailIdx = I;
   return false;
 }
 
+// Parsing helper function that strips the first character in S and returns it.
+static char popFront(StringRef &S) {
+  char C = S.front();
+  S = S.drop_front();
+  return C;
+}
+
+bool FileCheckPattern::parseExpression(StringRef Name, StringRef Trailer,
+                                       const SourceMgr &SM) const {
+  if (!Name.equals("@LINE")) {
+    SM.PrintMessage(SMLoc::getFromPointer(Name.data()), SourceMgr::DK_Error,
+                    "invalid pseudo variable '" + Name + "'");
+    return true;
+  }
+
+  // Check if this is a supported operation and select function to perform it.
+  if (Trailer.empty())
+    return false;
+  SMLoc OpLoc = SMLoc::getFromPointer(Trailer.data());
+  char Operator = popFront(Trailer);
+  switch (Operator) {
+  case '+':
+  case '-':
+    break;
+  default:
+    SM.PrintMessage(OpLoc, SourceMgr::DK_Error,
+                    Twine("unsupported numeric operation '") + Twine(Operator) +
+                        "'");
+    return true;
+  }
+
+  // Parse right operand.
+  if (Trailer.empty()) {
+    SM.PrintMessage(SMLoc::getFromPointer(Trailer.data()), SourceMgr::DK_Error,
+                    "missing operand in numeric expression '" + Trailer + "'");
+    return true;
+  }
+  uint64_t Offset;
+  if (Trailer.consumeInteger(10, Offset)) {
+    SM.PrintMessage(SMLoc::getFromPointer(Trailer.data()), SourceMgr::DK_Error,
+                    "invalid offset in numeric expression '" + Trailer + "'");
+    return true;
+  }
+  if (!Trailer.empty()) {
+    SM.PrintMessage(SMLoc::getFromPointer(Trailer.data()), SourceMgr::DK_Error,
+                    "unexpected characters at end of numeric expression '" +
+                        Trailer + "'");
+    return true;
+  }
+  return false;
+}
+
 /// Parses the given string into the Pattern.
 ///
 /// \p Prefix provides which prefix is being matched, \p SM provides the
 /// SourceMgr used for error reports, and \p LineNumber is the line number in
 /// the input file from which the pattern string was read. Returns true in
 /// case of an error, false otherwise.
 bool FileCheckPattern::ParsePattern(StringRef PatternStr, StringRef Prefix,
                            SourceMgr &SM, unsigned LineNumber,
if (PatternStr.startswith("[[")) {
                         SourceMgr::DK_Error,
                         "invalid named regex reference, no ]] found");
         return true;
       }
 
       MatchStr = MatchStr.substr(0, End);
       PatternStr = PatternStr.substr(End + 4);
 
+      size_t VarEndIdx = MatchStr.find(":");
+      size_t SpacePos = MatchStr.substr(0, VarEndIdx).find_first_of(" \t");
+      if (SpacePos != StringRef::npos) {
+        SM.PrintMessage(SMLoc::getFromPointer(MatchStr.data() + SpacePos),
+                        SourceMgr::DK_Error, "unexpected whitespace");
+        return true;
+      }
+
       // Get the regex name (e.g. "foo") and verify it is well formed.
       bool IsPseudo;
       unsigned TrailIdx;
       if (parseVariable(MatchStr, IsPseudo, TrailIdx)) {
         SM.PrintMessage(SMLoc::getFromPointer(MatchStr.data()),
                         SourceMgr::DK_Error, "invalid name in named regex");
         return true;
       }
 
       StringRef Name = MatchStr.substr(0, TrailIdx);
       StringRef Trailer = MatchStr.substr(TrailIdx);
-      bool IsVarDef = (Trailer.find(":") != StringRef::npos);
+      bool IsVarDef = (VarEndIdx != StringRef::npos);
 
       if (IsVarDef && (IsPseudo || !Trailer.consume_front(":"))) {
         SM.PrintMessage(SMLoc::getFromPointer(MatchStr.data()),
                         SourceMgr::DK_Error,
                         "invalid name in named regex definition");
         return true;
       }
 
-      // Verify that the name/expression is well formed. FileCheck currently
-      // supports @LINE, @LINE+number, @LINE-number expressions. The check here
-      // is relaxed. A stricter check is performed in \c EvaluateExpression.
-      if (IsPseudo) {
-        for (unsigned I = 0, E = Trailer.size(); I != E; ++I) {
-          if (!isalnum(Trailer[I]) && Trailer[I] != '+' && Trailer[I] != '-') {
-            SM.PrintMessage(SMLoc::getFromPointer(Name.data() + I),
-                            SourceMgr::DK_Error, "invalid name in named regex");
-            return true;
-          }
-        }
-      }
+      if (!IsVarDef && IsPseudo) {
+        if (parseExpression(Name, Trailer, SM))
 
       // Handle [[foo]].
       if (!IsVarDef) {
         // Handle variables that were defined earlier on the same line by
         // emitting a backreference.
         if (VariableDefs.find(Name) != VariableDefs.end()) {
           unsigned VarParenNum = VariableDefs[Name];
           if (VarParenNum < 1 || VarParenNum > 9) {

 
 void FileCheckPattern::AddBackrefToRegEx(unsigned BackrefNum) {
   assert(BackrefNum >= 1 && BackrefNum <= 9 && "Invalid backref number");
   std::string Backref = std::string("\\") + std::string(1, '0' + BackrefNum);
   RegExStr += Backref;
 }
 
 /// Evaluates expression and stores the result to \p Value.
-///
-/// Returns true on success and false when the expression has invalid syntax.
-bool FileCheckPattern::EvaluateExpression(StringRef Expr, std::string &Value) const {
-  // The only supported expression is @LINE([\+-]\d+)?
-  if (!Expr.startswith("@LINE"))
-    return false;
+void FileCheckPattern::evaluateExpression(StringRef Expr,
+                                          std::string &Value) const {
   Expr = Expr.substr(StringRef("@LINE").size());
   int Offset = 0;
   if (!Expr.empty()) {
     if (Expr[0] == '+')
       Expr = Expr.substr(1);
-    else if (Expr[0] != '-')
-      return false;
-    if (Expr.getAsInteger(10, Offset))
-      return false;
+    Expr.getAsInteger(10, Offset);
   }
   Value = llvm::itostr(LineNumber + Offset);
-  return true;
 }
 
 /// Matches the pattern string against the input buffer \p Buffer
 ///
 /// This returns the position that is matched or npos if there is no match. If
 /// there is a match, the size of the matched string is returned in \p
 /// MatchLen.
 ///
size_t FileCheckPattern::match(StringRef Buffer, size_t &MatchLen) const {
   if (!VariableUses.empty()) {
     TmpStr = RegExStr;
 
     unsigned InsertOffset = 0;
     for (const auto &VariableUse : VariableUses) {
       std::string Value;
 
       if (VariableUse.first[0] == '@') {
-        if (!EvaluateExpression(VariableUse.first, Value))
-          return StringRef::npos;
+        evaluateExpression(VariableUse.first, Value);
       } else {
         llvm::Optional<StringRef> ValueRef =
             Context->getVarValue(VariableUse.first);
         // If the variable is undefined, return an error.
         if (!ValueRef)
           return StringRef::npos;
 
         // Look up the value and escape it so that we can put it into the regex.
void FileCheckPattern::printVariableUses(const SourceMgr &SM, StringRef Buffer,
   // variable values.
   if (!VariableUses.empty()) {
     for (const auto &VariableUse : VariableUses) {
       SmallString<256> Msg;
       raw_svector_ostream OS(Msg);
       StringRef Var = VariableUse.first;
       if (Var[0] == '@') {
         std::string Value;
-        if (EvaluateExpression(Var, Value)) {
-          OS << "with expression \"";
-          OS.write_escaped(Var) << "\" equal to \"";
-          OS.write_escaped(Value) << "\"";
-        } else {
-          OS << "uses incorrect expression \"";
-          OS.write_escaped(Var) << "\"";
-        }
-      } else {
+        evaluateExpression(Var, Value);
         llvm::Optional<StringRef> VarValue = Context->getVarValue(Var);
 
         // Check for undefined variable references.
         if (!VarValue) {
           OS << "uses undefined variable \"";
           OS.write_escaped(Var) << "\"";
         } else {
           OS << "with variable \"";

llvm/test/FileCheck/line-count.txt

 ; RUN: FileCheck  -input-file %s %s
-; RUN: not FileCheck -check-prefix BAD -input-file %s %s
-3
-4 aaa
-5 bbb
-6 ccc
-7 CHECK: [[@LINE-3]] {{a}}aa
-8 CHECK: [[@LINE-3]] {{b}}bb
-9 CHECK: [[@LINE-3]] {{c}}cc
-10 foobar
-11 CHECK: [[@LINE-1]] {{foo}}bar
-12
-13 arst CHECK: [[@LINE]] {{a}}rst
-14
-15 BAD: [[@LINE:cant-have-regex]]
+; RUN: not FileCheck -check-prefix BAD1 -input-file %s %s 2>&1 | FileCheck -check-prefix ERR1 %s
+; RUN: not FileCheck -check-prefix BAD2 -input-file %s %s 2>&1 | FileCheck -check-prefix ERR2 %s
+; RUN: not FileCheck -check-prefix BAD3 -input-file %s %s 2>&1 | FileCheck -check-prefix ERR3 %s
+; RUN: not FileCheck -check-prefix BAD4 -input-file %s %s 2>&1 | FileCheck -check-prefix ERR4 %s
+; RUN: not FileCheck -check-prefix BAD5 -input-file %s %s 2>&1 | FileCheck -check-prefix ERR5 %s
+; RUN: not FileCheck -check-prefix BAD6 -input-file %s %s 2>&1 | FileCheck -check-prefix ERR6 %s
+; RUN: not FileCheck -check-prefix BAD7 -input-file %s %s 2>&1 | FileCheck -check-prefix ERR7 %s
+; RUN: not FileCheck -check-prefix BAD8 -input-file %s %s 2>&1 | FileCheck -check-prefix ERR8 %s
+; RUN: not FileCheck -check-prefix BAD9 -input-file %s %s 2>&1 | FileCheck -check-prefix ERR9 %s
+; RUN: not FileCheck -check-prefix BAD10 -input-file %s %s 2>&1 | FileCheck -check-prefix ERR10 %s
+; RUN: not FileCheck -check-prefix BAD10 -input-file %s %s 2>&1 | FileCheck -check-prefix ERR10 %s
+13
+14 aaa
+15 bbb
+16 ccc
+17 CHECK: [[@LINE-3]] {{a}}aa
+18 CHECK: [[@LINE-3]] {{b}}bb
+19 CHECK: [[@LINE-3]] {{c}}cc
+20 foobar
+21 CHECK: [[@LINE-1]] {{foo}}bar
+22
+23 arst CHECK: [[@LINE]] {{a}}rst
+24
+25 BAD1: [[@LINE:cant-have-regex]]
+26 ERR1: line-count.txt:[[@LINE-1]]:12: error: invalid name in named regex definition
+27
+28 BAD2: [[ @LINE]]
+29 ERR2: line-count.txt:[[@LINE-1]]:12: error: unexpected whitespace
+30
+31 BAD3: [[@LINE ]]
+32 ERR3: line-count.txt:[[@LINE-1]]:17: error: unexpected whitespace
+33
+34 BAD4: [[ @LINE-1]]
+35 ERR4: line-count.txt:[[@LINE-1]]:12: error: unexpected whitespace
+36
+37 BAD5: [[@LINE -1]]
+38 ERR5: line-count.txt:[[@LINE-1]]:17: error: unexpected whitespace
+39
+40 BAD6: [[@LINE- 1]]
+41 ERR6: line-count.txt:[[@LINE-1]]:18: error: unexpected whitespace
+42
+43 BAD7: [[@LINE-1 ]]
+44 ERR7: line-count.txt:[[@LINE-1]]:19: error: unexpected whitespace
+45
+46 BAD8: [[@LIN]]
+47 ERR8: line-count.txt:[[@LINE-1]]:12: error: invalid  pseudo variable '@LIN'
+48
+49 BAD9: [[@LINE*2]]
+50 ERR9: line-count.txt:[[@LINE-1]]:17: error: unsupported numeric operation '*'
+51
+52 BAD10: [[@LINE-x]]
+53 ERR10: line-count.txt:[[@LINE-1]]:19: error: invalid offset in numeric expression 'x'
+54
+55 BAD11: [[@LINE-1x]]
+56 ERR11: line-count.txt:[[@LINE-1]]:19: error: unexpected characters at end of numeric expression 'x'

llvm/unittests/Support/FileCheckTest.cpp

TEST_F(FileCheckTest, ParseVar) {
   VarName = "BadVar:";
   IsPseudo = true;
   TrailIdx = 0;
   EXPECT_FALSE(FileCheckPattern::parseVariable(VarName, IsPseudo, TrailIdx));
   EXPECT_FALSE(IsPseudo);
   EXPECT_EQ(TrailIdx, VarName.size() - 1);
 }
 
+class ExprTester {
+private:
+  SourceMgr SM;
+  FileCheckPatternContext Context;
+  FileCheckPattern P = FileCheckPattern(Check::CheckPlain, &Context);
+
+public:
+  bool parseExpect(std::string &VarName, std::string &Trailer) {
+    StringRef NameTrailer = StringRef(VarName + Trailer);
+    std::unique_ptr<MemoryBuffer> Buffer =
+        MemoryBuffer::getMemBufferCopy(NameTrailer, "TestBuffer");
+    StringRef NameTrailerRef = Buffer->getBuffer();
+    SM.AddNewSourceBuffer(std::move(Buffer), SMLoc());
+    StringRef VarNameRef = NameTrailerRef.substr(0, VarName.size());
+    StringRef TrailerRef = NameTrailerRef.substr(VarName.size());
+    return P.parseExpression(VarNameRef, TrailerRef, SM);
+  }
+};
+
+TEST_F(FileCheckTest, ParseExpr) {
+  ExprTester Tester;
+
+  // @LINE with offset.
+  std::string VarName = "@LINE";
+  std::string Trailer = "+3";
+  EXPECT_FALSE(Tester.parseExpect(VarName, Trailer));
+
+  // @LINE only.
+  Trailer = "";
+  EXPECT_FALSE(Tester.parseExpect(VarName, Trailer));
+
+  // Wrong Pseudovar.
+  VarName = "@FOO";
+  EXPECT_TRUE(Tester.parseExpect(VarName, Trailer));
+
+  // Unsupported operator.
+  VarName = "@LINE";
+  Trailer = "/2";
+  EXPECT_TRUE(Tester.parseExpect(VarName, Trailer));
+
+  // Missing offset operand.
+  VarName = "@LINE";
+  Trailer = "+";
+  EXPECT_TRUE(Tester.parseExpect(VarName, Trailer));
+
+  // Cannot parse offset operand.
+  VarName = "@LINE";
+  Trailer = "+x";
+  EXPECT_TRUE(Tester.parseExpect(VarName, Trailer));
+
+  // Unexpected string at end of numeric expression.
+  VarName = "@LINE";
+  Trailer = "+5x";
+  EXPECT_TRUE(Tester.parseExpect(VarName, Trailer));
+}
+
 TEST_F(FileCheckTest, FileCheckContext) {
   FileCheckPatternContext Cxt = FileCheckPatternContext();
   std::vector<std::string> GlobalDefines;
   SourceMgr SM;
 
   // Missing equal sign
   GlobalDefines.emplace_back(std::string("LocalVar"));
   EXPECT_TRUE(Cxt.defineCmdlineVariables(GlobalDefines, SM));