FileCheck [2/12]: Stricter parsing of -D option

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 -D option.

Prior to this change, parsing of -D option was very loose: it assumed
that there is an equal sign (which to be fair is now checked by the
FileCheck executable) and that the part on the left of the equal sign
was a valid variable name. This commit adds logic to ensure that this
is the case and gives diagnostic when it is not, making it clear that
the issue came from a command-line option error. This is achieved by
sharing the variable parsing code into a new function ParseVariable.

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)

Reviewers: jhenderson, chandlerc, jdenny, probinson, grimar, arichardson, rnk

Subscribers: hiraditya, llvm-commits, probinson, dblaikie, grimar, arichardson, tra, rnk, kristina, hfinkel, rogfer01, JonChesterfield

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D60382

llvm-svn: 359447

1 files changed, 112 insertions, 43 deletions

diff --git a/llvm/lib/Support/FileCheck.cpp b/llvm/lib/Support/FileCheck.cpp
index e1564a4..a80750d 100644
--- a/llvm/lib/Support/FileCheck.cpp
+++ b/llvm/lib/Support/FileCheck.cpp
@@ -24,6 +24,37 @@
 
 using namespace llvm;
 
+bool FileCheckPattern::isValidVarNameStart(char C) {
+  return C == '_' || isalpha(C);
+}
+
+bool FileCheckPattern::parseVariable(StringRef Str, bool &IsPseudo,
+                                     unsigned &TrailIdx) {
+  if (Str.empty())
+    return true;
+
+  bool ParsedOneChar = false;
+  unsigned I = 0;
+  IsPseudo = Str[0] == '@';
+
+  // Global vars start with '$'.
+  if (Str[0] == '$' || IsPseudo)
+    ++I;
+
+  for (unsigned E = Str.size(); I != E; ++I) {
+    if (!ParsedOneChar && !isValidVarNameStart(Str[I]))
+      return true;
+
+    // Variable names are composed of alphanumeric characters and underscores.
+    if (Str[I] != '_' && !isalnum(Str[I]))
+      break;
+    ParsedOneChar = true;
+  }
+
+  TrailIdx = I;
+  return false;
+}
+
 /// Parses the given string into the Pattern.
 ///
 /// \p Prefix provides which prefix is being matched, \p SM provides the
@@ -117,9 +148,10 @@ bool FileCheckPattern::ParsePattern(StringRef PatternStr, StringRef Prefix,
     // itself must be of the form "[a-zA-Z_][0-9a-zA-Z_]*", otherwise we reject
     // it.  This is to catch some common errors.
     if (PatternStr.startswith("[[")) {
+      StringRef MatchStr = PatternStr.substr(2);
       // Find the closing bracket pair ending the match.  End is going to be an
       // offset relative to the beginning of the match string.
-      size_t End = FindRegexVarEnd(PatternStr.substr(2), SM);
+      size_t End = FindRegexVarEnd(MatchStr, SM);
 
       if (End == StringRef::npos) {
         SM.PrintMessage(SMLoc::getFromPointer(PatternStr.data()),
@@ -128,55 +160,44 @@ bool FileCheckPattern::ParsePattern(StringRef PatternStr, StringRef Prefix,
         return true;
       }
 
-      StringRef MatchStr = PatternStr.substr(2, End);
+      MatchStr = MatchStr.substr(0, End);
       PatternStr = PatternStr.substr(End + 4);
 
-      // Get the regex name (e.g. "foo").
-      size_t NameEnd = MatchStr.find(':');
-      StringRef Name = MatchStr.substr(0, NameEnd);
+      // 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);
 
-      if (Name.empty()) {
-        SM.PrintMessage(SMLoc::getFromPointer(Name.data()), SourceMgr::DK_Error,
-                        "invalid name in named regex: empty name");
+      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, more strict check is performed in \c EvaluateExpression.
-      bool IsExpression = false;
-      for (unsigned i = 0, e = Name.size(); i != e; ++i) {
-        if (i == 0) {
-          if (Name[i] == '$')  // Global vars start with '$'
-            continue;
-          if (Name[i] == '@') {
-            if (NameEnd != StringRef::npos) {
-              SM.PrintMessage(SMLoc::getFromPointer(Name.data()),
-                              SourceMgr::DK_Error,
-                              "invalid name in named regex definition");
-              return true;
-            }
-            IsExpression = true;
-            continue;
+      // 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 (Name[i] != '_' && !isalnum(Name[i]) &&
-            (!IsExpression || (Name[i] != '+' && Name[i] != '-'))) {
-          SM.PrintMessage(SMLoc::getFromPointer(Name.data() + i),
-                          SourceMgr::DK_Error, "invalid name in named regex");
-          return true;
-        }
-      }
-
-      // Name can't start with a digit.
-      if (isdigit(static_cast<unsigned char>(Name[0]))) {
-        SM.PrintMessage(SMLoc::getFromPointer(Name.data()), SourceMgr::DK_Error,
-                        "invalid name in named regex");
-        return true;
       }
 
       // Handle [[foo]].
-      if (NameEnd == StringRef::npos) {
+      if (!IsVarDef) {
         // Handle variables that were defined earlier on the same line by
         // emitting a backreference.
         if (VariableDefs.find(Name) != VariableDefs.end()) {
@@ -189,7 +210,7 @@ bool FileCheckPattern::ParsePattern(StringRef PatternStr, StringRef Prefix,
           }
           AddBackrefToRegEx(VarParenNum);
         } else {
-          VariableUses.push_back(std::make_pair(Name, RegExStr.size()));
+          VariableUses.push_back(std::make_pair(MatchStr, RegExStr.size()));
         }
         continue;
       }
@@ -199,7 +220,7 @@ bool FileCheckPattern::ParsePattern(StringRef PatternStr, StringRef Prefix,
       RegExStr += '(';
       ++CurParen;
 
-      if (AddRegExToRegEx(MatchStr.substr(NameEnd + 1), CurParen, SM))
+      if (AddRegExToRegEx(Trailer, CurParen, SM))
         return true;
 
       RegExStr += ')';
@@ -755,7 +776,8 @@ FindFirstMatchingPrefix(Regex &PrefixRE, StringRef &Buffer,
 bool llvm::FileCheck::ReadCheckFile(
     SourceMgr &SM, StringRef Buffer, Regex &PrefixRE,
     std::vector<FileCheckString> &CheckStrings) {
-  PatternContext.defineCmdlineVariables(Req.GlobalDefines);
+  if (PatternContext.defineCmdlineVariables(Req.GlobalDefines, SM))
+    return true;
 
   std::vector<FileCheckPattern> ImplicitNegativeChecks;
   for (const auto &PatternString : Req.ImplicitCheckNot) {
@@ -1374,12 +1396,59 @@ Regex llvm::FileCheck::buildCheckPrefixRegex() {
   return Regex(PrefixRegexStr);
 }
 
-void FileCheckPatternContext::defineCmdlineVariables(
-    std::vector<std::string> &CmdlineDefines) {
+bool FileCheckPatternContext::defineCmdlineVariables(
+    std::vector<std::string> &CmdlineDefines, SourceMgr &SM) {
   assert(GlobalVariableTable.empty() &&
          "Overriding defined variable with command-line variable definitions");
+
+  if (CmdlineDefines.empty())
+    return false;
+
+  // Create a string representing the vector of command-line definitions. Each
+  // definition is on its own line and prefixed with a definition number to
+  // clarify which definition a given diagnostic corresponds to.
+  unsigned I = 0;
+  bool ErrorFound = false;
+  std::string CmdlineDefsDiag;
+  StringRef Prefix1 = "Global define #";
+  StringRef Prefix2 = ": ";
   for (StringRef CmdlineDef : CmdlineDefines)
-    GlobalVariableTable.insert(CmdlineDef.split('='));
+    CmdlineDefsDiag +=
+        (Prefix1 + Twine(++I) + Prefix2 + CmdlineDef + "\n").str();
+
+  std::unique_ptr<MemoryBuffer> CmdLineDefsDiagBuffer =
+      MemoryBuffer::getMemBufferCopy(CmdlineDefsDiag, "Global defines");
+  StringRef CmdlineDefsDiagRef = CmdLineDefsDiagBuffer->getBuffer();
+  SM.AddNewSourceBuffer(std::move(CmdLineDefsDiagBuffer), SMLoc());
+
+  SmallVector<StringRef, 4> CmdlineDefsDiagVec;
+  CmdlineDefsDiagRef.split(CmdlineDefsDiagVec, '\n', -1 /*MaxSplit*/,
+                           false /*KeepEmpty*/);
+  for (StringRef CmdlineDefDiag : CmdlineDefsDiagVec) {
+    unsigned NameStart = CmdlineDefDiag.find(Prefix2) + Prefix2.size();
+    if (CmdlineDefDiag.substr(NameStart).find('=') == StringRef::npos) {
+      SM.PrintMessage(SMLoc::getFromPointer(CmdlineDefDiag.data()),
+                      SourceMgr::DK_Error,
+                      "Missing equal sign in global definition");
+      ErrorFound = true;
+      continue;
+    }
+    std::pair<StringRef, StringRef> CmdlineNameVal =
+        CmdlineDefDiag.substr(NameStart).split('=');
+    StringRef Name = CmdlineNameVal.first;
+    bool IsPseudo;
+    unsigned TrailIdx;
+    if (FileCheckPattern::parseVariable(Name, IsPseudo, TrailIdx) || IsPseudo ||
+        TrailIdx != Name.size() || Name.empty()) {
+      SM.PrintMessage(SMLoc::getFromPointer(Name.data()), SourceMgr::DK_Error,
+                      "invalid name for variable definition '" + Name + "'");
+      ErrorFound = true;
+      continue;
+    }
+    GlobalVariableTable.insert(CmdlineNameVal);
+  }
+
+  return ErrorFound;
 }
 
 void FileCheckPatternContext::clearLocalVars() {