[FileCheck] Forbid using var defined on same line

[thopre]

Summary

Commit r366897 introduced the possibility to set a variable from an
expression, such as #VAR2:VAR1+3. While introducing this feature, it
introduced extra logic to allow using such a variable on the same line
later on. Unfortunately that extra logic is flawed as it relies on a
mapping from variable to expression defining it when the mapping is from
variable definition to expression. This flaw causes among other issues
PR42896.

This commit avoids the problem by forbidding all use of a variable
defined on the same line, and removes the now useless logic. Redesign
will be done in a later commit because it will require some amount of
refactoring first for the solution to be clean. One example is the need
for some sort of transaction mechanism to set a variable temporarily and
from an expression and rollback if the CHECK pattern does not match so
that diagnostics show the right variable values.

[jdenny]

llvm/test/FileCheck/numeric-expression.txt

188

After the earlier removal in this file, is this the only remaining check that you can use a numeric variable within another's pattern? There should probably be a positive check too. Sorry if I overlooked one.

[thopre]

llvm/test/FileCheck/numeric-expression.txt

188

There is a positive check removed above (see lines 85-92 in the before pane).

[jdenny]

llvm/test/FileCheck/numeric-expression.txt

188

That's the removal I was referring to. So, unless I overlooked one, there's now no positive check, but shouldn't there be one? For example:

20
42
CHECK: [[# VAR20:]]
CHECK-NEXT: [[# VAR42: VAR20+22]]

[thopre]

• Keep a positive match test
• Fix comment in unit tests

[thopre]

llvm/test/FileCheck/numeric-expression.txt

188

Ah yes indeed, good catch. I misunderstood your sentence. Fixed.

[arichardson]

LGTM except for one minor issue.

llvm/include/llvm/Support/FileCheck.h

112–113

ExpressionAST is not used anymore?
Unrelated, but should this constructor be explicit to avoid implicit conversions from StringRef?

[thopre]

Remove unused ExpressionAST parameter in FileCheckNumericVariable constructor

[thopre]

llvm/include/llvm/Support/FileCheck.h

112–113

Err yes indeed. I really need to get used to that aspect of C++. Will schedule a separate patch.

[thopre]

In D66141#1641237, @arichardson wrote:

LGTM except for one minor issue.

Hi Alexander,

Was your intention to approve the patch or did you intentionally leave it to others to approve?

Best regards.

[arichardson]

In D66141#1647722, @thopre wrote:

In D66141#1641237, @arichardson wrote:

LGTM except for one minor issue.

Hi Alexander,

Was your intention to approve the patch or did you intentionally leave it to others to approve?

Best regards.

While I've made some changes to FileCheck in the past and I'm happy to review all the changes, I feel like someone more familiar with the code should probably give the final approval.

[jhenderson]

I just want to make sure I understand the aim here:

1. This partially reverts the behaviour introduced in rL366897 to prevent the issue with variables defined on the same line (but still allowing variables to be defined from an expression).
2. A later patch will fix and reintroduce this behaviour.

Is there anything in particular preventing you doing 2) straight away, making this patch redundant?

This is a minor point, but I wonder whether you should stop calling it "on the same line" and start saying "in the same check". If I'm not mistaken, a CHECK-SAME would allow for a usage of a variable in a different check from the one it was defined in, but matching on the same output line:

0
1 2
CHECK:      [[#VAR1:]]
CHECK-NEXT: [[#VAR2:VAR1+1]]
CHECK-SAME: [[#VAR2+1]]

llvm/unittests/Support/FileCheckTest.cpp

86–88

I'm not sure I follow why this case has been deleted, if it's still valid to define a variable via an expression.

[thopre]

In D66141#1648698, @jhenderson wrote:

I just want to make sure I understand the aim here:

1. This partially reverts the behaviour introduced in rL366897 to prevent the issue with variables defined on the same line (but still allowing variables to be defined from an expression).
2. A later patch will fix and reintroduce this behaviour.

Correct.

Is there anything in particular preventing you doing 2) straight away, making this patch redundant?

In short: having a reliable FileCheck ASAP.

I've spent some time already trying to come up with a staging mechanism to be able to set the value of a variable from an expression for the case where it's used later but revert if the line fails to match. I want a system that is both simple (less risk of bugs) and foolproof (cannot evaluate a variable without a staging area initialized and automatic rollback if not committing the result) which I didn't quite manage due to the current encapsulation. Since I also have less time to spend on FileCheck these days, that means the bug would stay there for longer that I would like. Since this bug can result in incorrect match and thus hide others, I'd really prefer to have a fix now, even if that means loosing a bit of (arguably corner case) feature.

This is a minor point, but I wonder whether you should stop calling it "on the same line" and start saying "in the same check". If I'm not mistaken, a CHECK-SAME would allow for a usage of a variable in a different check from the one it was defined in, but matching on the same output line:

0
1 2
CHECK:      [[#VAR1:]]
CHECK-NEXT: [[#VAR2:VAR1+1]]
CHECK-SAME: [[#VAR2+1]]

Indeed. Will fix that.

llvm/unittests/Support/FileCheckTest.cpp

86–88

Indeed, it just needs adapting.

[probinson]

I am okay with saying you cannot reference a variable in the same CHECK directive where it is defined.
There's the workaround of splitting such a directive into a CHECK and CHECK-SAME; it is not always identical (see my FileCheck Follies talk) but will usually work.

[thopre]

Clarify that a numeric variable cannot be used if defined in the same CHECK directive.

llvm/unittests/Support/FileCheckTest.cpp

86–88

Actually that removal is justified because the FileCheckNumericVariable that this testcase tests is no longer aware of where does the value of a variable comes from. Because this patch forbids using a variable defined on the same CHECK directive, the value is guaranteed to be available in the FileCheckNumericVariable when doing the subtitution. A variable therefore only needs to be set to a given value or its value retrieved (for subtitution).

[jhenderson]

llvm/lib/Support/FileCheck.cpp

118

ExpressionAST looks like it's no longer used?

[thopre]

• Remove unused ExpressionAST in parseNumericVariableDefinition
• Fix numeric-expression.txt testcase after change of diagnostic message
• Fix codestyle issues

[jhenderson]

LGTM.

llvm/include/llvm/Support/FileCheck.h

 };
 
 /// Class representing a numeric variable and its associated current value.
 class FileCheckNumericVariable {
 private:
   /// Name of the numeric variable.
   StringRef Name;
 
-  /// Pointer to expression defining this numeric variable. Null for pseudo
-  /// variable whose value is known at parse time (e.g. @LINE pseudo variable)
-  /// or cleared local variable.
-  FileCheckExpressionAST *ExpressionAST;
-
   /// Value of numeric variable, if defined, or None otherwise.
   Optional<uint64_t> Value;
 
   /// Line number where this variable is defined, or None if defined before
   /// input is parsed. Used to determine whether a variable is defined on the
   /// same line as a given use.
   Optional<size_t> DefLineNumber;
 
 public:
   /// Constructor for a variable \p Name defined at line \p DefLineNumber or
-  /// defined before input is parsed if \p DefLineNumber is None. If not null,
-  /// the value set with setValue must match the result of evaluating
-  /// \p ExpressionAST.
+  /// defined before input is parsed if \p DefLineNumber is None.
   FileCheckNumericVariable(StringRef Name,
-                           Optional<size_t> DefLineNumber = None,
-                           FileCheckExpressionAST *ExpressionAST = nullptr)
-      : Name(Name), ExpressionAST(ExpressionAST), DefLineNumber(DefLineNumber) {
+                           Optional<size_t> DefLineNumber = None)
+      : Name(Name), DefLineNumber(DefLineNumber) {
   }
 
   /// \returns name of this numeric variable.
   StringRef getName() const { return Name; }
 
   /// \returns this variable's value.
   Optional<uint64_t> getValue() const { return Value; }
 
-  /// \returns the pointer to the expression defining this numeric variable, if
-  /// any, or null otherwise.
-  FileCheckExpressionAST *getExpressionAST() const { return ExpressionAST; }
-
-  /// \returns whether this variable's value is known when performing the
-  /// substitutions of the line where it is defined.
-  bool isValueKnownAtMatchTime() const;
-
-  /// Sets value of this numeric variable to \p NewValue. Triggers an assertion
-  /// failure if the variable is defined by an expression and the expression
-  /// cannot be evaluated to be equal to \p NewValue.
-  void setValue(uint64_t NewValue);
+  /// Sets value of this numeric variable to \p NewValue.
+  void setValue(uint64_t NewValue) { Value = NewValue; }
 
   /// Clears value of this numeric variable, regardless of whether it is
   /// currently defined or not.
-  void clearValue() {
-    Value = None;
-    ExpressionAST = nullptr;
-  }
+  void clearValue() { Value = None; }
 
   /// \returns the line number where this variable is defined, if any, or None
   /// if defined before input is parsed.
   Optional<size_t> getDefLineNumber() { return DefLineNumber; }
 };
 
 /// Class representing the use of a numeric variable in the AST of an
 /// expression.

llvm/lib/Support/FileCheck.cpp

 #include "llvm/Support/FormatVariadic.h"
 #include <cstdint>
 #include <list>
 #include <tuple>
 #include <utility>
 
 using namespace llvm;
 
-bool FileCheckNumericVariable::isValueKnownAtMatchTime() const {
-  if (Value)
-    return true;
-
-  return ExpressionAST != nullptr;
-}
-
-void FileCheckNumericVariable::setValue(uint64_t NewValue) {
-  if (ExpressionAST != nullptr) {
-    // Caller is expected to call setValue only if substitution was successful.
-    assert(NewValue == cantFail(ExpressionAST->eval(),
-                                "Failed to evaluate associated expression when "
-                                "sanity checking value") &&
-           "Value being set to different from variable evaluation");
-  }
-  Value = NewValue;
-  // Clear pointer to AST to ensure it is not used after the numeric
-  // substitution defining this variable is processed since it's the
-  // substitution that owns the pointer.
-  ExpressionAST = nullptr;
-}
-
 Expected<uint64_t> FileCheckNumericVariableUse::eval() const {
   Optional<uint64_t> Value = NumericVariable->getValue();
   if (Value)
     return *Value;
 
-  FileCheckExpressionAST *ExpressionAST = NumericVariable->getExpressionAST();
-  if (!ExpressionAST)
-    return make_error<FileCheckUndefVarError>(Name);
-
-  return ExpressionAST->eval();
 }
 
 Expected<uint64_t> FileCheckASTBinop::eval() const {
   Expected<uint64_t> LeftOp = LeftOperand->eval();
   Expected<uint64_t> RightOp = RightOperand->eval();
 
   // Bubble up any error (e.g. undefined variables) in the recursive
   // evaluation.

 
 char FileCheckUndefVarError::ID = 0;
 char FileCheckErrorDiagnostic::ID = 0;
 char FileCheckNotFoundError::ID = 0;
 
 Expected<FileCheckNumericVariable *>
 FileCheckPattern::parseNumericVariableDefinition(
     StringRef &Expr, FileCheckPatternContext *Context,
     Optional<size_t> LineNumber, FileCheckExpressionAST *ExpressionAST,
     const SourceMgr &SM) {
   Expected<VariableProperties> ParseVarResult = parseVariable(Expr, SM);
   if (!ParseVarResult)
     return ParseVarResult.takeError();
   StringRef Name = ParseVarResult->Name;
 
   if (ParseVarResult->IsPseudo)
     return FileCheckErrorDiagnostic::get(
if (!Expr.empty())
     return FileCheckErrorDiagnostic::get(
         SM, Expr, "unexpected characters after numeric variable name");
 
   FileCheckNumericVariable *DefinedNumericVariable;
   auto VarTableIter = Context->GlobalNumericVariableTable.find(Name);
   if (VarTableIter != Context->GlobalNumericVariableTable.end())
     DefinedNumericVariable = VarTableIter->second;
   else
-    DefinedNumericVariable =
-        Context->makeNumericVariable(Name, LineNumber, ExpressionAST);
+    DefinedNumericVariable = Context->makeNumericVariable(Name, LineNumber);
 
   return DefinedNumericVariable;
 }
 
 Expected<std::unique_ptr<FileCheckNumericVariableUse>>
 FileCheckPattern::parseNumericVariableUse(StringRef Name, bool IsPseudo,
                                           Optional<size_t> LineNumber,
                                           FileCheckPatternContext *Context,
FileCheckPattern::parseNumericVariableUse(StringRef Name, bool IsPseudo,
   if (VarTableIter != Context->GlobalNumericVariableTable.end())
     NumericVariable = VarTableIter->second;
   else {
     NumericVariable = Context->makeNumericVariable(Name);
     Context->GlobalNumericVariableTable[Name] = NumericVariable;
   }
 
   Optional<size_t> DefLineNumber = NumericVariable->getDefLineNumber();
-  if (DefLineNumber && LineNumber && *DefLineNumber == *LineNumber &&
-      !NumericVariable->isValueKnownAtMatchTime())
+  if (DefLineNumber && LineNumber && *DefLineNumber == *LineNumber)
     return FileCheckErrorDiagnostic::get(
         SM, Name,
-        "numeric variable '" + Name +
-            "' defined from input on the same line as used");
+        "numeric variable '" + Name + "' defined on the same line as used");
 
   return llvm::make_unique<FileCheckNumericVariableUse>(Name, NumericVariable);
 }
 
 Expected<std::unique_ptr<FileCheckExpressionAST>>
 FileCheckPattern::parseNumericOperand(StringRef &Expr, AllowedOperand AO,
                                       Optional<size_t> LineNumber,
                                       FileCheckPatternContext *Context,

llvm/test/FileCheck/numeric-expression.txt

 42
 CHECK-LABEL: USE MULTI VAR
 CHECK-NEXT: [[#VAR2:]]
 CHECK-NEXT: [[#VAR1+VAR2]]
 
 ; Numeric expression using a variable defined from a numeric expression.
 DEF EXPR GOOD MATCH
 42
-41 43
+41
 ; CHECK-LABEL: DEF EXPR GOOD MATCH
 ; CHECK-NEXT: [[# VAR42:VAR1+31]]
-; CHECK-NEXT: [[# VAR41: VAR42-1]] [[# VAR41 + 2]]
+; CHECK-NEXT: [[# VAR42-1]]
 
 ; Empty numeric expression.
 EMPTY NUM EXPR
 foo 104 bar
 ; CHECK-LABEL: EMPTY NUM EXPR
 ; CHECK-NEXT: foo [[#]] bar
 
 ; Numeric expression using undefined variables.

 RUN: not FileCheck --check-prefix DEF-EXPR-FAIL --input-file %s %s 2>&1 \
 RUN:   | FileCheck --strict-whitespace --check-prefix DEF-EXPR-FAIL-MSG %s
 
 DEF EXPR WRONG MATCH
 20
 43
 DEF-EXPR-FAIL-LABEL: DEF EXPR WRONG MATCH
 DEF-EXPR-FAIL-NEXT: [[# VAR20:]]
 DEF-EXPR-FAIL-NEXT: [[# VAR42: VAR20+22]]
 DEF-EXPR-FAIL-MSG: numeric-expression.txt:[[#@LINE-1]]:21: error: {{D}}EF-EXPR-FAIL-NEXT: is not on the line after the previous match
 DEF-EXPR-FAIL-MSG-NEXT: {{D}}EF-EXPR-FAIL-NEXT: {{\[\[# VAR42: VAR20\+22\]\]}}
 DEF-EXPR-FAIL-MSG-NEXT:   {{^}}                    ^{{$}}
+
+; Verify that using a numeric variable defined on the same line (whether from
+; input or from an expression) is rejected.
+RUN: not FileCheck --check-prefix SAME-LINE-USE1 --input-file %s %s 2>&1 \
+RUN:   | FileCheck --strict-whitespace --check-prefix SAME-LINE-USE-MSG1 %s
+RUN: not FileCheck --check-prefix SAME-LINE-USE2 --input-file %s %s 2>&1 \
+RUN:   | FileCheck --strict-whitespace --check-prefix SAME-LINE-USE-MSG2 %s
+
+SAME LINE USE
+3
+4 5
+SAME-LINE-USE1-LABEL: SAME LINE USE
+SAME-LINE-USE1-NEXT: [[#]]
+SAME-LINE-USE1-NEXT: [[#VAR1:]] [[#VAR1+1]]
+SAME-LINE-USE-MSG1: numeric-expression.txt:[[#@LINE-1]]:36: error: numeric variable 'VAR1' defined on the same line as used
+SAME-LINE-USE-MSG1-NEXT: {{S}}AME-LINE-USE1-NEXT: {{\[\[#VAR1:\]\] \[\[#VAR1\+1\]\]}}
+SAME-LINE-USE-MSG1-NEXT:        {{^}}                                   ^{{$}}
+
+SAME-LINE-USE2-LABEL: SAME LINE USE
+SAME-LINE-USE2-NEXT: [[#VAR1:]]
+SAME-LINE-USE2-NEXT: [[#VAR2:VAR1+1]] [[#VAR2+1]]
+SAME-LINE-USE-MSG2: numeric-expression.txt:[[#@LINE-1]]:42: error: numeric variable 'VAR2' defined on the same line as used
+SAME-LINE-USE-MSG2-NEXT: {{S}}AME-LINE-USE2-NEXT: {{\[\[#VAR2:VAR1\+1\]\] \[\[#VAR2\+1\]\]}}
+SAME-LINE-USE-MSG2-NEXT:        {{^}}                                         ^{{$}}

llvm/unittests/Support/FileCheckTest.cpp

 // is not ExpectedUndefVarName.
 static void expectUndefError(const Twine &ExpectedUndefVarName, Error Err) {
   expectUndefErrors({ExpectedUndefVarName.str()}, std::move(Err));
 }
 
 uint64_t doAdd(uint64_t OpL, uint64_t OpR) { return OpL + OpR; }
 
 TEST_F(FileCheckTest, NumericVariable) {
-  // Undefined variable: isValueKnownAtMatchTime returns false, getValue and
-  // eval fail, error returned by eval holds the name of the undefined
-  // variable.
+  // Undefined variable: getValue and eval fail, error returned by eval holds
+  // the name of the undefined variable.
   FileCheckNumericVariable FooVar = FileCheckNumericVariable("FOO", 1);
   EXPECT_EQ("FOO", FooVar.getName());
-  EXPECT_FALSE(FooVar.isValueKnownAtMatchTime());
   FileCheckNumericVariableUse FooVarUse =
       FileCheckNumericVariableUse("FOO", &FooVar);
   EXPECT_FALSE(FooVar.getValue());
   Expected<uint64_t> EvalResult = FooVarUse.eval();
   ASSERT_FALSE(EvalResult);
   expectUndefError("FOO", EvalResult.takeError());
 
   FooVar.setValue(42);
 
-  // Defined variable: isValueKnownAtMatchTime returns true, getValue and eval
-  // return value set.
-  EXPECT_TRUE(FooVar.isValueKnownAtMatchTime());
+  // Defined variable: getValue and eval return value set.
   Optional<uint64_t> Value = FooVar.getValue();
   ASSERT_TRUE(bool(Value));
   EXPECT_EQ(42U, *Value);
   EvalResult = FooVarUse.eval();
   ASSERT_TRUE(bool(EvalResult));
   EXPECT_EQ(42U, *EvalResult);
 
-  // Variable defined by numeric expression: isValueKnownAtMatchTime
-  // returns true, getValue and eval return value of expression, setValue
-  // clears expression.
-  std::unique_ptr<FileCheckNumericVariableUse> FooVarUsePtr =
-      llvm::make_unique<FileCheckNumericVariableUse>("FOO", &FooVar);
-  std::unique_ptr<FileCheckExpressionLiteral> One =
-      llvm::make_unique<FileCheckExpressionLiteral>(1);
-  FileCheckASTBinop Binop =
-      FileCheckASTBinop(doAdd, std::move(FooVarUsePtr), std::move(One));
-  FileCheckNumericVariable FoobarExprVar =
-      FileCheckNumericVariable("FOOBAR", 2, &Binop);
-  EXPECT_TRUE(FoobarExprVar.isValueKnownAtMatchTime());
-  ASSERT_FALSE(FoobarExprVar.getValue());
-  FileCheckNumericVariableUse FoobarExprVarUse =
-      FileCheckNumericVariableUse("FOOBAR", &FoobarExprVar);
-  EvalResult = FoobarExprVarUse.eval();
-  ASSERT_TRUE(bool(EvalResult));
-  EXPECT_EQ(43U, *EvalResult);
-  EXPECT_TRUE(FoobarExprVar.getExpressionAST());
-  FoobarExprVar.setValue(43);
-  EXPECT_FALSE(FoobarExprVar.getExpressionAST());
-  FoobarExprVar = FileCheckNumericVariable("FOOBAR", 2, &Binop);
-  EXPECT_TRUE(FoobarExprVar.getExpressionAST());
-
   // Clearing variable: getValue and eval fail. Error returned by eval holds
   // the name of the cleared variable.
   FooVar.clearValue();
-  FoobarExprVar.clearValue();
-  EXPECT_FALSE(FoobarExprVar.getExpressionAST());
   EXPECT_FALSE(FooVar.getValue());
-  EXPECT_FALSE(FoobarExprVar.getValue());
   EvalResult = FooVarUse.eval();
   ASSERT_FALSE(EvalResult);
   expectUndefError("FOO", EvalResult.takeError());
-  EvalResult = FoobarExprVarUse.eval();
-  ASSERT_FALSE(EvalResult);
-  expectUndefError("FOOBAR", EvalResult.takeError());
 }
 
 TEST_F(FileCheckTest, Binop) {
   FileCheckNumericVariable FooVar = FileCheckNumericVariable("FOO", 1);
   FooVar.setValue(42);
   std::unique_ptr<FileCheckNumericVariableUse> FooVarUse =
       llvm::make_unique<FileCheckNumericVariableUse>("FOO", &FooVar);
   FileCheckNumericVariable BarVar = FileCheckNumericVariable("BAR", 2);
TEST_F(FileCheckTest, ParseExpr) {
   // Only valid variable.
   EXPECT_FALSE(Tester.parseSubstExpect("@LINE"));
   EXPECT_FALSE(Tester.parseSubstExpect("FOO"));
   EXPECT_FALSE(Tester.parseSubstExpect("UNDEF"));
 
   // Valid empty expression.
   EXPECT_FALSE(Tester.parseSubstExpect(""));
 
-  // Valid use of variable defined on the same line from expression. Note that
-  // the same pattern object is used for the parsePatternExpect and
+  // Invalid use of variable defined on the same line from expression. Note
+  // that the same pattern object is used for the parsePatternExpect and
   // parseSubstExpect since no initNextPattern is called, thus appearing as
   // being on the same line from the pattern's point of view.
   ASSERT_FALSE(Tester.parsePatternExpect("[[#LINE1VAR:FOO+1]]"));
-  EXPECT_FALSE(Tester.parseSubstExpect("LINE1VAR"));
+  EXPECT_TRUE(Tester.parseSubstExpect("LINE1VAR"));
 
   // Invalid use of variable defined on same line from input. As above, the
   // absence of a call to initNextPattern makes it appear to be on the same
   // line from the pattern's point of view.
   ASSERT_FALSE(Tester.parsePatternExpect("[[#LINE2VAR:]]"));
   EXPECT_TRUE(Tester.parseSubstExpect("LINE2VAR"));
 
   // Unsupported operator.
   EXPECT_TRUE(Tester.parseSubstExpect("@LINE/2"));
 
   // Missing offset operand.
   EXPECT_TRUE(Tester.parseSubstExpect("@LINE+"));
 
   // Valid expression.
   EXPECT_FALSE(Tester.parseSubstExpect("@LINE+5"));
   EXPECT_FALSE(Tester.parseSubstExpect("FOO+4"));
-  EXPECT_FALSE(Tester.parseSubstExpect("FOOBAR"));
   Tester.initNextPattern();
+  EXPECT_FALSE(Tester.parseSubstExpect("FOOBAR"));
+  EXPECT_FALSE(Tester.parseSubstExpect("LINE1VAR"));
   EXPECT_FALSE(Tester.parsePatternExpect("[[#FOO+FOO]]"));
   EXPECT_FALSE(Tester.parsePatternExpect("[[#FOO+3-FOO]]"));
 }
 
 TEST_F(FileCheckTest, ParsePattern) {
   PatternTester Tester;
 
   // Space in pattern variable expression.