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

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

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

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).

177

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

184

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.

190

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

196–197

Nit: is relaxed -> is a relaxed

1418

Keep the StringRef here.

1419

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

177

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?

1418

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?

1419

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

[jhenderson]

llvm/lib/Support/FileCheck.cpp

177

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

196–197

Stricter -> A stricter

1418

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.

1419

See above.

[probinson]

llvm/lib/Support/FileCheck.cpp

1419

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

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

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

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

1419

"which definition a given diagnostic corresponds to."

1422

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

llvm/test/FileCheck/defines.txt

38

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/include/llvm/Support/FileCheck.h

 
 public:
   /// Return the value of variable \p VarName or nothing if no such variable has
   /// been defined.
   llvm::Optional<StringRef> getPatternVarValue(StringRef VarName);
 
   /// Define pattern and numeric 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);
+  /// 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), ie remove them from GlobalVariableTable.
   void clearLocalVars(void);
 };
 
 struct FileCheckDiag;
 
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; }
+  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;
   void PrintVariableUses(const SourceMgr &SM, StringRef Buffer,
                          SMRange MatchRange = None) const;
   void PrintFuzzyMatch(const SourceMgr &SM, StringRef Buffer,
                        std::vector<FileCheckDiag> *Diags) const;
 

llvm/lib/Support/FileCheck.cpp

 #include <cstdint>
 #include <list>
 #include <map>
 #include <tuple>
 #include <utility>
 
 using namespace llvm;
 
+// Verify that the string in \p Str or at the start of \p Str (if \p
+// AllowTrailer is true) is a well formed variable name. Also set IsPseudo to
+// true if it is a pseudo variable.
+bool FileCheckPattern::parseVariable(StringRef Str, bool &IsPseudo,
+                                     unsigned &TrailIdx) {
+  bool ParsedOneChar = false;
+  unsigned i = 0;
+  IsPseudo = false;
+  for (unsigned e = Str.size(); i != e; ++i) {
+    if (i == 0) {
+      // Global vars start with '$'.
+      if (Str[i] == '$')
+        continue;
+      else if (Str[i] == '@') {
+        IsPseudo = true;
+        continue;
+      }
+    }
+    // Variable names are composed of alphanumeric characters and underscores.
+    if (Str[i] != '_' && !isalnum(Str[i]))
+      break;
+    ParsedOneChar = true;
+  }
+
+  // Empty name.
+  if (!ParsedOneChar)
+    return true;
+
+  TrailIdx = i;
+  StringRef Name = Str.substr(0, i);
+
+  // Name can't start with a digit.
+  if (isdigit(static_cast<unsigned char>(Name[0])))
+    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,
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] != '-'))) {
+      StringRef Name = MatchStr.substr(0, TrailIdx);
+      StringRef Trailer = MatchStr.substr(TrailIdx);
+      size_t DefSepIdx = Trailer.find(":");
+      bool IsVarDef = (DefSepIdx != StringRef::npos);
+
+      if (IsVarDef && (IsPseudo || !Trailer.consume_front(":"))) {
+        SM.PrintMessage(SMLoc::getFromPointer(MatchStr.data()),
+                        "Invalid name in named regex definition");
+
+      // 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.
+      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) {
   auto *PatternContext = new FileCheckPatternContext();
-  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(
for (StringRef Prefix : Req.CheckPrefixes) {
 
     PrefixRegexStr.append(Prefix);
   }
 
   return Regex(PrefixRegexStr);
 }
 
 /// Define pattern and numeric variables from definitions given on the command
-/// line passed as a vector of VAR=VAL strings in \p CmdlineDefines.
-void FileCheckPatternContext::defineCmdlineVariables(
-    std::vector<std::string> &CmdlineDefines) {
+/// line passed as a vector of VAR=VAL strings in \p CmdlineDefines. Report any
+/// error to \p SM and return whether an error occured.
+bool FileCheckPatternContext::defineCmdlineVariables(
+    std::vector<std::string> &CmdlineDefines, SourceMgr &SM) {
   static bool CmdlineVariablesDefined = false;
 
   if (CmdlineVariablesDefined)
-    return;
+    return true;
   CmdlineVariablesDefined = true;
 
-  for (const auto &CmdlineDef : CmdlineDefines)
-    GlobalVariableTable.insert(StringRef(CmdlineDef).split('='));
+  bool ErrorFound = false;
+  for (const auto &CmdlineDef : CmdlineDefines) {
+    std::string Prefix = "-D";
+    std::string DiagCmdline = Prefix + CmdlineDef;
+    std::unique_ptr<MemoryBuffer> CmdLine =
+        MemoryBuffer::getMemBufferCopy(DiagCmdline, "command line");
+    StringRef CmdlineDefRef = CmdLine->getBuffer().substr(Prefix.size());
+    SM.AddNewSourceBuffer(std::move(CmdLine), SMLoc());
+
+    bool IsPseudo;
+    unsigned TrailIdx;
+    std::pair<StringRef, StringRef> CmdlineNameVal = CmdlineDefRef.split('=');
+    StringRef Name = CmdlineNameVal.first;
+    if (FileCheckPattern::parseVariable(Name, IsPseudo, TrailIdx) || IsPseudo ||
+        TrailIdx != Name.size()) {
+      SM.PrintMessage(SMLoc::getFromPointer(CmdlineDefRef.data()),
+                      SourceMgr::DK_Error,
+                      "Invalid name for variable definition '" + Name + "'");
+      ErrorFound = true;
+      continue;
+    }
+    GlobalVariableTable.insert(CmdlineNameVal);
+  }
+
+  return ErrorFound;
 }
 
 /// Undefine local variables (variables whose name does not start with a '$'
 /// sign), ie remove them from GlobalVariableTable.
 void FileCheckPatternContext::clearLocalVars(void) {
   SmallVector<StringRef, 16> LocalPatternVars, LocalNumericVars;
   for (const auto &Var : GlobalVariableTable)
     if (Var.first()[0] != '$')

llvm/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 -check-prefix ERRCLIFMT
+; RUN: not FileCheck -D@VALUE=10 -input-file %s %s 2>&1 | FileCheck %s -check-prefix ERRCLIPSEUDO
+; RUN: not FileCheck -D'VALUE + 2=10' -input-file %s %s 2>&1 | FileCheck %s -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: command line:1:3: error: Invalid name for variable definition '10VALUE'
+; ERRCLIFMT-NEXT: -D10VALUE=10
+; ERRCLIFMT-NEXT: {{^  \^$}}
+
+; ERRCLIPSEUDO: command line:1:3: error: Invalid name for variable definition '@VALUE'
+; ERRCLIPSEUDO-NEXT: -D@VALUE=10
+; ERRCLIPSEUDO-NEXT: {{^  \^$}}
+
+; ERRCLITRAIL: command line:1:3: error: Invalid name for variable definition 'VALUE + 2'
+; ERRCLITRAIL-NEXT: -DVALUE + 2=10
+; ERRCLITRAIL-NEXT: {{^  \^$}}