From: Thomas Preud'homme Date: Mon, 2 Sep 2019 14:04:00 +0000 (+0000) Subject: [FileCheck] Forbid using var defined on same line X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=131f5690c1f1badc8dafd760c9acb28a4052a964;p=llvm [FileCheck] Forbid using var defined on same line 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. Reviewers: jhenderson, chandlerc, jdenny, probinson, grimar, arichardson, rnk Subscribers: JonChesterfield, rogfer01, hfinkel, kristina, rnk, tra, arichardson, grimar, dblaikie, probinson, llvm-commits, hiraditya Tags: #llvm Differential Revision: https://reviews.llvm.org/D66141 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@370663 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/docs/CommandGuide/FileCheck.rst b/docs/CommandGuide/FileCheck.rst index 429f3ad07d7..e8b324d080d 100644 --- a/docs/CommandGuide/FileCheck.rst +++ b/docs/CommandGuide/FileCheck.rst @@ -648,7 +648,7 @@ The ``--enable-var-scope`` option has the same effect on numeric variables as on string variables. Important note: In its current implementation, an expression cannot use a -numeric variable with a non-empty expression defined on the same line. +numeric variable defined earlier in the same CHECK directive. FileCheck Pseudo Numeric Variables ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/include/llvm/Support/FileCheck.h b/include/llvm/Support/FileCheck.h index 944ca1d1b4a..149666157a6 100644 --- a/include/llvm/Support/FileCheck.h +++ b/include/llvm/Support/FileCheck.h @@ -98,11 +98,6 @@ 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 Value; @@ -113,14 +108,10 @@ private: 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 DefLineNumber = None, - FileCheckExpressionAST *ExpressionAST = nullptr) - : Name(Name), ExpressionAST(ExpressionAST), DefLineNumber(DefLineNumber) { - } + Optional DefLineNumber = None) + : Name(Name), DefLineNumber(DefLineNumber) {} /// \returns name of this numeric variable. StringRef getName() const { return Name; } @@ -128,25 +119,12 @@ public: /// \returns this variable's value. Optional 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. @@ -609,8 +587,7 @@ private: /// should defining such a variable be invalid. static Expected parseNumericVariableDefinition( StringRef &Expr, FileCheckPatternContext *Context, - Optional LineNumber, FileCheckExpressionAST *ExpressionAST, - const SourceMgr &SM); + Optional LineNumber, const SourceMgr &SM); /// Parses \p Name as a (pseudo if \p IsPseudo is true) numeric variable use /// at line \p LineNumber, or before input is parsed if \p LineNumber is /// None. Parameter \p Context points to the class instance holding the live diff --git a/lib/Support/FileCheck.cpp b/lib/Support/FileCheck.cpp index 9310a7911a7..5c05af4c190 100644 --- a/lib/Support/FileCheck.cpp +++ b/lib/Support/FileCheck.cpp @@ -24,38 +24,12 @@ 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 FileCheckNumericVariableUse::eval() const { Optional Value = NumericVariable->getValue(); if (Value) return *Value; - FileCheckExpressionAST *ExpressionAST = NumericVariable->getExpressionAST(); - if (!ExpressionAST) - return make_error(Name); - - return ExpressionAST->eval(); + return make_error(Name); } Expected FileCheckASTBinop::eval() const { @@ -141,8 +115,7 @@ char FileCheckNotFoundError::ID = 0; Expected FileCheckPattern::parseNumericVariableDefinition( StringRef &Expr, FileCheckPatternContext *Context, - Optional LineNumber, FileCheckExpressionAST *ExpressionAST, - const SourceMgr &SM) { + Optional LineNumber, const SourceMgr &SM) { Expected ParseVarResult = parseVariable(Expr, SM); if (!ParseVarResult) return ParseVarResult.takeError(); @@ -169,8 +142,7 @@ FileCheckPattern::parseNumericVariableDefinition( if (VarTableIter != Context->GlobalNumericVariableTable.end()) DefinedNumericVariable = VarTableIter->second; else - DefinedNumericVariable = - Context->makeNumericVariable(Name, LineNumber, ExpressionAST); + DefinedNumericVariable = Context->makeNumericVariable(Name, LineNumber); return DefinedNumericVariable; } @@ -202,12 +174,11 @@ FileCheckPattern::parseNumericVariableUse(StringRef Name, bool IsPseudo, } Optional 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"); + "' defined earlier in the same CHECK directive"); return std::make_unique(Name, NumericVariable); } @@ -334,8 +305,7 @@ FileCheckPattern::parseNumericSubstitutionBlock( if (DefEnd != StringRef::npos) { DefExpr = DefExpr.ltrim(SpaceChars); Expected ParseResult = - parseNumericVariableDefinition(DefExpr, Context, LineNumber, - ExpressionAST.get(), SM); + parseNumericVariableDefinition(DefExpr, Context, LineNumber, SM); if (!ParseResult) return ParseResult.takeError(); diff --git a/test/FileCheck/numeric-expression.txt b/test/FileCheck/numeric-expression.txt index cb1e9f26928..8bbaca0a4ac 100644 --- a/test/FileCheck/numeric-expression.txt +++ b/test/FileCheck/numeric-expression.txt @@ -85,10 +85,10 @@ 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 @@ -189,3 +189,27 @@ 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 earlier in the same CHECK directive +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 earlier in the same CHECK directive +SAME-LINE-USE-MSG2-NEXT: {{S}}AME-LINE-USE2-NEXT: {{\[\[#VAR2:VAR1\+1\]\] \[\[#VAR2\+1\]\]}} +SAME-LINE-USE-MSG2-NEXT: {{^}} ^{{$}} diff --git a/unittests/Support/FileCheckTest.cpp b/unittests/Support/FileCheckTest.cpp index 375f8960c10..fd1df57ca73 100644 --- a/unittests/Support/FileCheckTest.cpp +++ b/unittests/Support/FileCheckTest.cpp @@ -58,12 +58,10 @@ static void expectUndefError(const Twine &ExpectedUndefVarName, Error 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()); @@ -73,9 +71,7 @@ TEST_F(FileCheckTest, NumericVariable) { 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 Value = FooVar.getValue(); ASSERT_TRUE(bool(Value)); EXPECT_EQ(42U, *Value); @@ -83,43 +79,13 @@ TEST_F(FileCheckTest, NumericVariable) { 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 FooVarUsePtr = - std::make_unique("FOO", &FooVar); - std::unique_ptr One = - std::make_unique(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) { @@ -332,12 +298,12 @@ TEST_F(FileCheckTest, ParseExpr) { // 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 @@ -354,8 +320,9 @@ TEST_F(FileCheckTest, ParseExpr) { // 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]]")); }