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

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

Prior to this change, parsing of -D option was very loose: it assumed
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)

[thopre]

Use camel casing for new functions

[thopre]

Rebase

[arsenm]

llvm/lib/Support/FileCheck.cpp

58–60 ↗

(On Diff #194330)

I think this should be split into a separate isValidVarNameStart function or something. I would also invert this to be a whitelist. We probably also need to at least disallow $, @, +, - and :

[thopre]

Rebase and fix Optional usage

[thopre]

llvm/lib/Support/FileCheck.cpp

58–60 ↗

(On Diff #194330)

I'm not sure what you mean by invert to a whitelist. Do you mean:

Name[0] == '_' || isalpha(Name[0])

? Regarding $, @, +, - and : I believe they are already rejected by the line above checking for isalnum or '_', isn't it?

[probinson]

Wouldn't it be preferable to put the command-line option validation in the driver, rather than here?
The division of responsibilities between the driver and the library is really not clear-cut; normally I'd expect the driver to do option parsing/validation and the library to simply accept the list. (I understand that FileCheck currently isn't always organized that way, but it's something I'd like to see happen.)

[thopre]

• Fix variable parsing logic
• Add unit testing

[thopre]

In D60382#1464197, @probinson wrote:

Wouldn't it be preferable to put the command-line option validation in the driver, rather than here?
The division of responsibilities between the driver and the library is really not clear-cut; normally I'd expect the driver to do option parsing/validation and the library to simply accept the list. (I understand that FileCheck currently isn't always organized that way, but it's something I'd like to see happen.)

If the library was only used by the FileCheck executable I would agree but since it is used as a library by some of the tests I believe it makes sense to test here. Some of the code could go away if the library was receiving global defines as a vector of pair of StringRef but the validation of variable name would still be necessary IMO.

[thopre]

Rebase

[probinson]

In D60382#1464412, @thopre wrote:

In D60382#1464197, @probinson wrote:

Wouldn't it be preferable to put the command-line option validation in the driver, rather than here?
The division of responsibilities between the driver and the library is really not clear-cut; normally I'd expect the driver to do option parsing/validation and the library to simply accept the list. (I understand that FileCheck currently isn't always organized that way, but it's something I'd like to see happen.)

If the library was only used by the FileCheck executable I would agree but since it is used as a library by some of the tests I believe it makes sense to test here. Some of the code could go away if the library was receiving global defines as a vector of pair of StringRef but the validation of variable name would still be necessary IMO.

IMO this is not a user-facing API. I think it's entirely reasonable to trust the unittests to call the API correctly, and have the user-facing driver validate the user-provided input. If you want extra checks in the library in Asserts mode, that would be fine.

[thopre]

In D60382#1464553, @probinson wrote:

In D60382#1464412, @thopre wrote:

In D60382#1464197, @probinson wrote:

Wouldn't it be preferable to put the command-line option validation in the driver, rather than here?
The division of responsibilities between the driver and the library is really not clear-cut; normally I'd expect the driver to do option parsing/validation and the library to simply accept the list. (I understand that FileCheck currently isn't always organized that way, but it's something I'd like to see happen.)

If the library was only used by the FileCheck executable I would agree but since it is used as a library by some of the tests I believe it makes sense to test here. Some of the code could go away if the library was receiving global defines as a vector of pair of StringRef but the validation of variable name would still be necessary IMO.

IMO this is not a user-facing API. I think it's entirely reasonable to trust the unittests to call the API correctly, and have the user-facing driver validate the user-provided input. If you want extra checks in the library in Asserts mode, that would be fine.

Actually, while this parsing code can easily be moved to the FileCheck executable by assuming the name is well formed, follow-on commits eventually allow to define a numeric variable from the command-line to a numeric expression so the parsing code is reused to avoid a lot of code duplication and the parsing code expect a SourceMgr to print the diagnostic to.

Therefore, if we want to remove the diagnostic from that function we need to remove the ability to define a numeric variable from a numeric expression (which allows stuff like -D#NUMVAR1=10 -D#NUMVAR2=NUMVAR1+4. How useful do you think this ability is?

[jhenderson]

llvm/lib/Support/FileCheck.cpp

27 ↗

(On Diff #194896)

Here and elsewhere below, I don't believe the style has changed yet and this should be upper-case 'C' (also 'I', 'E' etc below).

171 ↗

(On Diff #194896)

Does this print the actual specified string in the error message?

178 ↗

(On Diff #194896)

DefSepIdx doesn't read well to me (as in I'm not sure what is meant by it at first glance). Perhaps a slightly more verbose name would be clearer.

184 ↗

(On Diff #194896)

Lower-case 'i' I think is more in keeping with elsewhere, so I don't think you should change this.

190 ↗

(On Diff #194896)

Nit: is relaxed -> is a relaxed

1407 ↗

(On Diff #194896)

Keep the StringRef here.

1408 ↗

(On Diff #194896)

Use a StringRef here.

llvm/unittests/Support/FileCheckTest.cpp

55–63 ↗

(On Diff #194896)

These should probably be a separate test since they test a different function.

70 ↗

(On Diff #194896)

Here and below, it might be worthwhile using VarName.size() (with appropriate - 1 as required) to make the test more robust, in case the input name changes ever.

[thopre]

• Fix one letter variable casing
• Do not capitalize error message
• Split new unit tests in 2 separate functions

[thopre]

Express expected result in unit test in terms of variable size

[probinson]

In D60382#1466257, @thopre wrote:

Therefore, if we want to remove the diagnostic from that function we need to remove the ability to define a numeric variable from a numeric expression (which allows stuff like -D#NUMVAR1=10 -D#NUMVAR2=NUMVAR1+4. How useful do you think this ability is?

I have no idea how useful this would be, but you're making a reasonable argument that in general the library needs to be where the validation occurs, because the acceptable syntax is determined by the library.
Just make it clear to the user where the problem is and what needs fixing.

[thopre]

llvm/lib/Support/FileCheck.cpp

171 ↗

(On Diff #194896)

It'll print the line, followed by a caret pointing to the variable followed by the error message. I thought mentioning the variable name in the error message was superfluous given the caret. Do you want me to include it?

1407 ↗

(On Diff #194896)

Should I rather use std::string instead? I need to prefix each string with "-D" (see DiagCmdline = Prefix + CmdlineDef) which cannot be done with a reference. Is there anything wrong with using std::string?

1408 ↗

(On Diff #194896)

Likewise, I need this to be std::string to allow for the concatenation on next line.

[jhenderson]

llvm/lib/Support/FileCheck.cpp

171 ↗

(On Diff #194896)

No, as long as it is clear from the context, there's no need.

190 ↗

(On Diff #194896)

Stricter -> A stricter

1407 ↗

(On Diff #194896)

Concatenation of StringRefs is supported via Twines. You can choose to make DiagCmdline a Twine itself, or do .str() on the result of the concatenation to get the final std::string a bit earlier, I don't really mind.

1408 ↗

(On Diff #194896)

See above.

[probinson]

llvm/lib/Support/FileCheck.cpp

1407 ↗

(On Diff #195374)

Why does the library know the name of the option passed to the driver? That's not a good separation of concerns.

[thopre]

• Make diagnostic for command-line global define parsing more neutral wrt. user of the FileCheck library
• Use StringRef in the loop for command-line global define parsing
• Fix wording of comment for @LINE parsing

[jhenderson]

I have only one more minor comment. Happy for this to go in with or without it, but @probinson should give the final thumbs up.

llvm/test/FileCheck/defines.txt

38 ↗

(On Diff #195525)

The line and column numbers like a bit weird to me, but isn't a big deal. If there's a way to not have them though, it would be better. Alternatively, if they have some actual meaning, please have a test case that produces different values for them.

[thopre]

llvm/test/FileCheck/defines.txt

38 ↗

(On Diff #195525)

Not have them in the output or in this expected text?

If the former yes currently it's not meaningful because the current code create a separate buffer with same name (Global defines) for each definition, so all definition would be a line 1, and the name would be a column 1 (I think it's good to test that bit since it shows we point to the variable name rather than the value).
I could change the code to have one line per definition which could be useful if a variable is defined twice from the command-line (i.e. -DFOO=10 -DFOO=12). Alternatively I could make the definition one after the other on the same line, perhaps separated by space (i.e. output would be FOO=10 FOO=12 with the caret pointing to the right FOO). Both of these approaches will require to loop over definition twice (once to construct the buffer, the second to parse it).

If the latter, I cannot introduce variations because the name will always be first but I think it's worth testing that the location is accurate.

[thopre]

Enumerate global defines in diagnostic

[thopre]

Remove debugging assert and add comment

[thopre]

• Add unit test for updated -D parsing
• Move unit tests to be in the same order as in FileCheck.h and FileCheck.cpp

[probinson]

llvm/lib/Support/FileCheck.cpp

47 ↗

(On Diff #196254)

A whole lot of this loop appears to be handling just the optional initial $/@ and then the initial isValidVarNameStart test. I'd think it would be more readable to factor out the handling of the initial one or two characters, then loop over the rest.

[probinson]

LGTM after the comment on the parseVariable loop is addressed.

[thopre]

Extract prefix characters checks out of loop in parseVariable.

[jhenderson]

LGTM, with a couple of nits.

llvm/lib/Support/FileCheck.cpp

1409 ↗

(On Diff #197088)

"which definition a given diagnostic corresponds to."

1412 ↗

(On Diff #197088)

You don't need = std::string() here.

llvm/test/FileCheck/defines.txt

38 ↗

(On Diff #195525)

I was referring to the output, but I think this looks somewhat better.

[thopre]

Fix english in comment in defineCmdlineVariables and remove unecessary call to String() constructor

llvm/trunk/include/llvm/Support/FileCheck.h

private:
   StringMap<StringRef> GlobalVariableTable;
 
 public:
   /// Return the value of variable \p VarName or None if no such variable has
   /// been defined.
   llvm::Optional<StringRef> getVarValue(StringRef VarName);
 
   /// Define variables from definitions given on the command line passed as a
-  /// vector of VAR=VAL strings in \p CmdlineDefines.
-  void defineCmdlineVariables(std::vector<std::string> &CmdlineDefines);
+  /// vector of VAR=VAL strings in \p CmdlineDefines. Report any error to \p SM
+  /// and return whether an error occured.
+  bool defineCmdlineVariables(std::vector<std::string> &CmdlineDefines,
+                              SourceMgr &SM);
 
   /// Undefine local variables (variables whose name does not start with a '$'
   /// sign), i.e. remove them from GlobalVariableTable.
   void clearLocalVars();
 };
 
 class FileCheckPattern {
   SMLoc PatternLoc;
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);
   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,

llvm/trunk/lib/Support/FileCheck.cpp

 #include <cstdint>
 #include <list>
 #include <map>
 #include <tuple>
 #include <utility>
 
 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
 /// 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,
while (!PatternStr.empty()) {
     }
 
     // Named RegEx matches.  These are of two forms: [[foo:.*]] which matches .*
     // (or some other regex) and assigns it to the FileCheck variable 'foo'. The
     // second form is [[foo]] which is a reference to foo.  The variable name
     // 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()),
                         SourceMgr::DK_Error,
                         "invalid named regex reference, no ]] found");
         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);
-
-      if (Name.empty()) {
-        SM.PrintMessage(SMLoc::getFromPointer(Name.data()), SourceMgr::DK_Error,
-                        "invalid name in named regex: empty name");
+      // 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;
       }
 
-      // 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;
-          }
-        }
-        if (Name[i] != '_' && !isalnum(Name[i]) &&
-            (!IsExpression || (Name[i] != '+' && Name[i] != '-'))) {
-          SM.PrintMessage(SMLoc::getFromPointer(Name.data() + i),
+      StringRef Name = MatchStr.substr(0, TrailIdx);
+      StringRef Trailer = MatchStr.substr(TrailIdx);
+      bool IsVarDef = (Trailer.find(":") != StringRef::npos);
+
+      if (IsVarDef && (IsPseudo || !Trailer.consume_front(":"))) {
+        SM.PrintMessage(SMLoc::getFromPointer(MatchStr.data()),
+
+      // 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;
           }
         }
-
-      // 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()) {
           unsigned VarParenNum = VariableDefs[Name];
           if (VarParenNum < 1 || VarParenNum > 9) {
             SM.PrintMessage(SMLoc::getFromPointer(Name.data()),
                             SourceMgr::DK_Error,
                             "Can't back-reference more than 9 variables");
             return true;
           }
           AddBackrefToRegEx(VarParenNum);
         } else {
-          VariableUses.push_back(std::make_pair(Name, RegExStr.size()));
+          VariableUses.push_back(std::make_pair(MatchStr, RegExStr.size()));
         }
         continue;
       }
 
       // Handle [[foo:.*]].
       VariableDefs[Name] = CurParen;
       RegExStr += '(';
       ++CurParen;
 
-      if (AddRegExToRegEx(MatchStr.substr(NameEnd + 1), CurParen, SM))
+      if (AddRegExToRegEx(Trailer, CurParen, SM))
         return true;
 
       RegExStr += ')';
     }
 
     // Handle fixed string matches.
     // Find the end, which is the start of the next regex.
     size_t FixedMatchEnd = PatternStr.find("{{");

 
 /// Read the check file, which specifies the sequence of expected strings.
 ///
 /// The strings are added to the CheckStrings vector. Returns true in case of
 /// an error, false otherwise.
 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) {
     // Create a buffer with fake command line content in order to display the
     // command line option responsible for the specific implicit CHECK-NOT.
     std::string Prefix = "-implicit-check-not='";
     std::string Suffix = "'";
     std::unique_ptr<MemoryBuffer> CmdLine = MemoryBuffer::getMemBufferCopy(
if (Prefix != Req.CheckPrefixes.front())
       PrefixRegexStr.push_back('|');
 
     PrefixRegexStr.append(Prefix);
   }
 
   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() {
   SmallVector<StringRef, 16> LocalPatternVars, LocalNumericVars;
   for (const StringMapEntry<StringRef> &Var : GlobalVariableTable)
     if (Var.first()[0] != '$')
       LocalPatternVars.push_back(Var.first());
 

llvm/trunk/test/FileCheck/defines.txt

 ; RUN: FileCheck -DVALUE=10 -input-file %s %s
 ; RUN: not FileCheck -DVALUE=20 -input-file %s %s 2>&1 | FileCheck %s -check-prefix ERRMSG
-;
 ; RUN: not FileCheck -DVALUE=10 -check-prefix NOT -input-file %s %s 2>&1 | FileCheck %s -check-prefix NOT-ERRMSG
 ; RUN: FileCheck -DVALUE=20 -check-prefix NOT -input-file %s %s
 ; RUN: not FileCheck -DVALUE10 -input-file %s %s 2>&1 | FileCheck %s -check-prefix ERRCLIEQ1
 ; RUN: not FileCheck -D -input-file %s %s 2>&1 | FileCheck %s -check-prefix ERRCLIEQ2
 ; RUN: not FileCheck -D=10 -input-file %s %s 2>&1 | FileCheck %s -check-prefix ERRCLIVAR1
 ; RUN: not FileCheck -D= -input-file %s %s 2>&1 | FileCheck %s -check-prefix ERRCLIVAR2
 ; RUN: FileCheck -DVALUE= -check-prefix EMPTY -input-file %s %s 2>&1
 
+; RUN: not FileCheck -D10VALUE=10 -input-file %s %s 2>&1 | FileCheck %s --strict-whitespace -check-prefix ERRCLIFMT
+; RUN: not FileCheck -D@VALUE=10 -input-file %s %s 2>&1 | FileCheck %s --strict-whitespace -check-prefix ERRCLIPSEUDO
+; RUN: not FileCheck -D'VALUE + 2=10' -input-file %s %s 2>&1 | FileCheck %s --strict-whitespace -check-prefix ERRCLITRAIL
 Value = 10
 ; CHECK: Value = [[VALUE]]
 ; NOT-NOT: Value = [[VALUE]]
 
 ; ERRMSG: defines.txt:[[@LINE-3]]:10: error: CHECK: expected string not found in input
 ; ERRMSG: defines.txt:1:1: note: scanning from here
 ; ERRMSG: defines.txt:1:1: note: with variable "VALUE" equal to "20"
 ; ERRMSG: defines.txt:[[@LINE-7]]:1: note: possible intended match here
 
 ; NOT-ERRMSG: defines.txt:[[@LINE-7]]:12: error: {{NOT}}-NOT: excluded string found in input
 ; NOT-ERRMSG: defines.txt:[[@LINE-10]]:1: note: found here
 ; NOT-ERRMSG: defines.txt:[[@LINE-11]]:1: note: with variable "VALUE" equal to "10"
 
 ; ERRCLIEQ1: Missing equal sign in command-line definition '-DVALUE10'
 
 ; ERRCLIEQ2: {{F|f}}ile{{C|c}}heck{{[^:]*}}: for the -D option: requires a value!
 
 ; ERRCLIVAR1: Missing pattern variable name in command-line definition '-D=10'
 
 ; ERRCLIVAR2: Missing pattern variable name in command-line definition '-D='
 
 Empty value = @@
 ; EMPTY: Empty value = @[[VALUE]]@
+
+; ERRCLIFMT: Global defines:1:19: error: invalid name for variable definition '10VALUE'
+; ERRCLIFMT-NEXT: Global define #1: 10VALUE=10
+; ERRCLIFMT-NEXT: {{^                  \^$}}
+
+; ERRCLIPSEUDO: Global defines:1:19: error: invalid name for variable definition '@VALUE'
+; ERRCLIPSEUDO-NEXT: Global define #1: @VALUE=10
+; ERRCLIPSEUDO-NEXT: {{^                  \^$}}
+
+; ERRCLITRAIL: Global defines:1:19: error: invalid name for variable definition 'VALUE + 2'
+; ERRCLITRAIL-NEXT: Global define #1: VALUE + 2=10
+; ERRCLITRAIL-NEXT: {{^                  \^$}}

llvm/trunk/unittests/Support/FileCheckTest.cpp

 //===- llvm/unittest/Support/FileCheckTest.cpp - FileCheck tests --===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/FileCheck.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 namespace {
 
 class FileCheckTest : public ::testing::Test {};
 
+TEST_F(FileCheckTest, ValidVarNameStart) {
+  EXPECT_TRUE(FileCheckPattern::isValidVarNameStart('a'));
+  EXPECT_TRUE(FileCheckPattern::isValidVarNameStart('G'));
+  EXPECT_TRUE(FileCheckPattern::isValidVarNameStart('_'));
+  EXPECT_FALSE(FileCheckPattern::isValidVarNameStart('2'));
+  EXPECT_FALSE(FileCheckPattern::isValidVarNameStart('$'));
+  EXPECT_FALSE(FileCheckPattern::isValidVarNameStart('@'));
+  EXPECT_FALSE(FileCheckPattern::isValidVarNameStart('+'));
+  EXPECT_FALSE(FileCheckPattern::isValidVarNameStart('-'));
+  EXPECT_FALSE(FileCheckPattern::isValidVarNameStart(':'));
+}
+
+TEST_F(FileCheckTest, ParseVar) {
+  StringRef VarName = "GoodVar42";
+  bool IsPseudo = true;
+  unsigned TrailIdx = 0;
+  EXPECT_FALSE(FileCheckPattern::parseVariable(VarName, IsPseudo, TrailIdx));
+  EXPECT_FALSE(IsPseudo);
+  EXPECT_EQ(TrailIdx, VarName.size());
+
+  VarName = "$GoodGlobalVar";
+  IsPseudo = true;
+  TrailIdx = 0;
+  EXPECT_FALSE(FileCheckPattern::parseVariable(VarName, IsPseudo, TrailIdx));
+  EXPECT_FALSE(IsPseudo);
+  EXPECT_EQ(TrailIdx, VarName.size());
+
+  VarName = "@GoodPseudoVar";
+  IsPseudo = true;
+  TrailIdx = 0;
+  EXPECT_FALSE(FileCheckPattern::parseVariable(VarName, IsPseudo, TrailIdx));
+  EXPECT_TRUE(IsPseudo);
+  EXPECT_EQ(TrailIdx, VarName.size());
+
+  VarName = "42BadVar";
+  EXPECT_TRUE(FileCheckPattern::parseVariable(VarName, IsPseudo, TrailIdx));
+
+  VarName = "$@";
+  EXPECT_TRUE(FileCheckPattern::parseVariable(VarName, IsPseudo, TrailIdx));
+
+  VarName = "B@dVar";
+  IsPseudo = true;
+  TrailIdx = 0;
+  EXPECT_FALSE(FileCheckPattern::parseVariable(VarName, IsPseudo, TrailIdx));
+  EXPECT_FALSE(IsPseudo);
+  EXPECT_EQ(TrailIdx, 1U);
+
+  VarName = "B$dVar";
+  IsPseudo = true;
+  TrailIdx = 0;
+  EXPECT_FALSE(FileCheckPattern::parseVariable(VarName, IsPseudo, TrailIdx));
+  EXPECT_FALSE(IsPseudo);
+  EXPECT_EQ(TrailIdx, 1U);
+
+  VarName = "BadVar+";
+  IsPseudo = true;
+  TrailIdx = 0;
+  EXPECT_FALSE(FileCheckPattern::parseVariable(VarName, IsPseudo, TrailIdx));
+  EXPECT_FALSE(IsPseudo);
+  EXPECT_EQ(TrailIdx, VarName.size() - 1);
+
+  VarName = "BadVar-";
+  IsPseudo = true;
+  TrailIdx = 0;
+  EXPECT_FALSE(FileCheckPattern::parseVariable(VarName, IsPseudo, TrailIdx));
+  EXPECT_FALSE(IsPseudo);
+  EXPECT_EQ(TrailIdx, VarName.size() - 1);
+
+  VarName = "BadVar:";
+  IsPseudo = true;
+  TrailIdx = 0;
+  EXPECT_FALSE(FileCheckPattern::parseVariable(VarName, IsPseudo, TrailIdx));
+  EXPECT_FALSE(IsPseudo);
+  EXPECT_EQ(TrailIdx, VarName.size() - 1);
+}
+
 TEST_F(FileCheckTest, FileCheckContext) {
-  FileCheckPatternContext Cxt;
+  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));
+
+  // Empty variable
+  GlobalDefines.clear();
+  GlobalDefines.emplace_back(std::string("=18"));
+  EXPECT_TRUE(Cxt.defineCmdlineVariables(GlobalDefines, SM));
+
+  // Invalid variable
+  GlobalDefines.clear();
+  GlobalDefines.emplace_back(std::string("18LocalVar=18"));
+  EXPECT_TRUE(Cxt.defineCmdlineVariables(GlobalDefines, SM));
 
-  // Define local and global variables from command-line.
+  // Define local variables from command-line.
+  GlobalDefines.clear();
   GlobalDefines.emplace_back(std::string("LocalVar=FOO"));
-  Cxt.defineCmdlineVariables(GlobalDefines);
+  GlobalDefines.emplace_back(std::string("EmptyVar="));
+  bool GotError = Cxt.defineCmdlineVariables(GlobalDefines, SM);
+  EXPECT_FALSE(GotError);
 
   // Check defined variables are present and undefined is absent.
   StringRef LocalVarStr = "LocalVar";
+  StringRef EmptyVarStr = "EmptyVar";
   StringRef UnknownVarStr = "UnknownVar";
   llvm::Optional<StringRef> LocalVar = Cxt.getVarValue(LocalVarStr);
+  llvm::Optional<StringRef> EmptyVar = Cxt.getVarValue(EmptyVarStr);
   llvm::Optional<StringRef> UnknownVar = Cxt.getVarValue(UnknownVarStr);
   EXPECT_TRUE(LocalVar);
   EXPECT_EQ(*LocalVar, "FOO");
+  EXPECT_TRUE(EmptyVar);
+  EXPECT_EQ(*EmptyVar, "");
   EXPECT_FALSE(UnknownVar);
 
   // Clear local variables and check they become absent.
   Cxt.clearLocalVars();
   LocalVar = Cxt.getVarValue(LocalVarStr);
   EXPECT_FALSE(LocalVar);
+  EmptyVar = Cxt.getVarValue(EmptyVarStr);
+  EXPECT_FALSE(EmptyVar);
 
   // Redefine global variables and check variables are defined again.
   GlobalDefines.emplace_back(std::string("$GlobalVar=BAR"));
-  Cxt.defineCmdlineVariables(GlobalDefines);
+  GotError = Cxt.defineCmdlineVariables(GlobalDefines, SM);
+  EXPECT_FALSE(GotError);
   StringRef GlobalVarStr = "$GlobalVar";
   llvm::Optional<StringRef> GlobalVar = Cxt.getVarValue(GlobalVarStr);
   EXPECT_TRUE(GlobalVar);
   EXPECT_EQ(*GlobalVar, "BAR");
 
   // Clear local variables and check global variables remain defined.
   Cxt.clearLocalVars();
   GlobalVar = Cxt.getVarValue(GlobalVarStr);
   EXPECT_TRUE(GlobalVar);
 }
 } // namespace