[FileCheck] Fix PR49531: invalid use of string var

[thopre]

Summary

FileCheck string substitution block parsing code only report an invalid
variable name in a string variable use if it starts with a forbidden
character. It does not report anything if there are unparsed characters
after the variable name, i.e. [[X-Y]] is parsed as [[X]] and no error is
returned. This commit fixes that.

[jhenderson]

llvm/lib/FileCheck/FileCheck.cpp

460

I wonder if it would make more sense to add the check to this function, somewhere around here. This is the "parseVariable" function after all, so seems like the logical place to reject invalid variable names, rather than the place it is currently detected. What do you think?

llvm/test/FileCheck/simple-var-capture.txt

22

I would suggest you also want an invalid variable name which is a definition (e.g. [[X-Y:.+]]) or something like that.

llvm/unittests/FileCheck/FileCheckTest.cpp

1347

Ditto - consider also checking for a definition with an invalid name.

[thopre]

llvm/lib/FileCheck/FileCheck.cpp

460

A trailer has to be allowed when parsing a variable use because it could be [[@LINE+1]]. If the variable is a pseudo variable, parsing is deferred to the numeric substitution block parsing. Otherwise any trailer is an error. Perhaps it's time to remove all uses of [[@LINE]] in favor of [[#@LINE]]

llvm/test/FileCheck/simple-var-capture.txt

22

The new code is only executed for string variable use. String variable definition and pseudo variable use different codepath.

llvm/unittests/FileCheck/FileCheckTest.cpp

1347

See line 1339 for a test for that already.

[jdenny]

LGTM if @jhenderson is happy.

[jhenderson]

llvm/lib/FileCheck/FileCheck.cpp

460

This function already knows whether it is a pseudo variable, so doesn't that mean you can do something like if(!IsPseudo) /*error if trailer*/?

[thopre]

llvm/lib/FileCheck/FileCheck.cpp

460

parseVariable is also called for numeric expression where it's fine to have a trailer after a variable ([[#FOO+2]]). I would need to distinguish 3 states:

• No trailer (e.g. string variable def and use)
• Trailer for pseudo only (Legacy @LINE expression)
• Trailer allowed (numeric expression)

That's why I suggested removing legacy @LINE expression, it would reduce this to 2 options which can be dealt with a AllowTrailer (false by default) parameter.

[jhenderson]

LGTM! I don't think there's a pressing need to get rid of the legacy pseudo variables, but I'm also not opposed to it, if you think it's worth doing.

llvm/lib/FileCheck/FileCheck.cpp

460

Ah, thanks. Didn't realise this was used elsewhere.

[thopre]

Sorry it seems I forgot to run the whole testsuite when I put this up for review. The patch found 3 issues:

https://reviews.llvm.org/D98852
https://reviews.llvm.org/D98853
https://reviews.llvm.org/D98854

[jdenny]

In D98691#2634161, @thopre wrote:

Sorry it seems I forgot to run the whole testsuite when I put this up for review. The patch found 3 issues:

https://reviews.llvm.org/D98852
https://reviews.llvm.org/D98853
https://reviews.llvm.org/D98854

Ah, it's good those are now being caught. Thanks for working on all this!

llvm/lib/FileCheck/FileCheck.cpp

if (!isValidVarNameStart(Str[I++]))
     return ErrorDiagnostic::get(SM, Str, "invalid variable name");
 
   for (size_t E = Str.size(); I != E; ++I)
     // Variable names are composed of alphanumeric characters and underscores.
     if (Str[I] != '_' && !isAlnum(Str[I]))
       break;
 
   StringRef Name = Str.take_front(I);
   Str = Str.substr(I);
   return VariableProperties {Name, IsPseudo};
 }
 
 // StringRef holding all characters considered as horizontal whitespaces by
 // FileCheck input canonicalization.
 constexpr StringLiteral SpaceChars = " \t";
 
 // Parsing helper function that strips the first character in S and returns it.
if (PatternStr.startswith("[[")) {
             return true;
           }
           DefName = Name;
           MatchRegexp = MatchStr.str();
         } else {
           if (IsPseudo) {
             MatchStr = OrigMatchStr;
             IsLegacyLineExpr = IsNumBlock = true;
-          } else
+          } else {
+            if (!MatchStr.empty()) {
+              SM.PrintMessage(SMLoc::getFromPointer(Name.data()),
+                              SourceMgr::DK_Error,
+                              "invalid name in string variable use");
+              return true;
+            }
             SubstStr = Name;
           }
         }
+      }
 
       // Parse numeric substitution block.
       std::unique_ptr<Expression> ExpressionPointer;
       Optional<NumericVariable *> DefinedNumericVariable;
       if (IsNumBlock) {
         Expected<std::unique_ptr<Expression>> ParseResult =
             parseNumericSubstitutionBlock(MatchStr, DefinedNumericVariable,
                                           IsLegacyLineExpr, LineNumber, Context,

llvm/test/FileCheck/simple-var-capture.txt

 // RUN: FileCheck -input-file %s %s
 
 op1 r1
 op2 r1, r2
 ; CHECK:        op1 [[REG:r[0-9]]]
 ; CHECK-NEXT:   op2 [[REG]]
 
 op3 r16, r18, r21
 op4 r30, r18, r21
 ; CHECK:        op3 {{r[0-9]+}}, [[REGa:r[0-9]+]], [[REGb:r[0-9]+]]
 ; CHECK-NEXT:   op4 {{r[0-9]+}}, [[REGa]], [[REGb]]
 
 
+// RUN: %ProtectFileCheckOutput \
+// RUN: not FileCheck --check-prefixes INVALID-VARNAME --input-file %s %s 2>&1 \
+// RUN:   | FileCheck --check-prefix INVALID-VARNAME-MSG --strict-whitespace %s
+
+5
+4
+; INVALID-VARNAME: [[X:]]
+; INVALID-VARNAME-NEXT: [[Y:]]
+; INVALID-VARNAME-NEXT: [[X-Y]]
+; INVALID-VARNAME-MSG: simple-var-capture.txt:[[#@LINE-1]]:27: error: invalid name in string variable use
+; INVALID-VARNAME-MSG-NEXT: ; {{I}}NVALID-VARNAME-NEXT: {{\[\[X-Y\]\]}}
+; INVALID-VARNAME-MSG-NEXT:    {{^}}                          ^{{$}}

llvm/unittests/FileCheck/FileCheckTest.cpp

TEST_F(FileCheckTest, ParsePattern) {
 
   // Invalid string variable definition.
   EXPECT_TRUE(Tester.parsePattern("[[@PAT:]]"));
   EXPECT_TRUE(Tester.parsePattern("[[PAT+2:]]"));
 
   // Collision with numeric variable.
   EXPECT_TRUE(Tester.parsePattern("[[FOO:]]"));
 
+  // Invalid use of string variable.
+  EXPECT_TRUE(Tester.parsePattern("[[FOO-BAR]]"));
+
   // Valid use of string variable.
   EXPECT_FALSE(Tester.parsePattern("[[BAR]]"));
 
   // Valid string variable definition.
   EXPECT_FALSE(Tester.parsePattern("[[PAT:[0-9]+]]"));
 
   // Invalid numeric substitution.
   EXPECT_TRUE(Tester.parsePattern("[[#42INVALID]]"));