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

95 ↗

(On Diff #194476)

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

[thopre]

Rebase

[thopre]

llvm/lib/Support/FileCheck.cpp

95 ↗

(On Diff #194476)

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

67 ↗

(On Diff #194780)

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

75 ↗

(On Diff #194780)

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

60 ↗

(On Diff #195131)

I guess this should be S

95 ↗

(On Diff #194476)

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

60 ↗

(On Diff #196265)

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 ↗

(On Diff #196265)

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/trunk/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/trunk/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/trunk/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/trunk/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));