Page MenuHomePhabricator

[FileCheck] Forbid using var defined on same line
ClosedPublic

Authored by thopre on Aug 13 2019, 7:23 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

thopre created this revision.Aug 13 2019, 7:23 AM
jdenny added inline comments.Aug 15 2019, 10:26 AM
llvm/test/FileCheck/numeric-expression.txt
180 ↗(On Diff #214832)

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 marked 2 inline comments as done.Aug 16 2019, 3:29 AM
thopre added inline comments.
llvm/test/FileCheck/numeric-expression.txt
180 ↗(On Diff #214832)

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

jdenny added inline comments.Aug 16 2019, 6:52 AM
llvm/test/FileCheck/numeric-expression.txt
180 ↗(On Diff #214832)

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 updated this revision to Diff 216096.Aug 20 2019, 3:52 AM
thopre marked an inline comment as done.
  • Keep a positive match test
  • Fix comment in unit tests
thopre marked 2 inline comments as done.Aug 20 2019, 3:52 AM
thopre added inline comments.
llvm/test/FileCheck/numeric-expression.txt
180 ↗(On Diff #214832)

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

LGTM except for one minor issue.

llvm/include/llvm/Support/FileCheck.h
114 ↗(On Diff #216096)

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

thopre updated this revision to Diff 216795.Fri, Aug 23, 3:45 AM
thopre marked 2 inline comments as done.

Remove unused ExpressionAST parameter in FileCheckNumericVariable constructor

thopre marked an inline comment as done.Fri, Aug 23, 3:47 AM
thopre added inline comments.
llvm/include/llvm/Support/FileCheck.h
114 ↗(On Diff #216096)

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

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.

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.

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 ↗(On Diff #216795)

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 marked an inline comment as done.Wed, Aug 28, 3:06 PM

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 ↗(On Diff #216795)

Indeed, it just needs adapting.

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 updated this revision to Diff 218057.Fri, Aug 30, 2:59 AM
thopre marked 2 inline comments as done.

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

llvm/unittests/Support/FileCheckTest.cpp
86–88 ↗(On Diff #216795)

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 added inline comments.Fri, Aug 30, 3:42 AM
llvm/lib/Support/FileCheck.cpp
118 ↗(On Diff #218057)

ExpressionAST looks like it's no longer used?

thopre updated this revision to Diff 218319.Mon, Sep 2, 2:48 AM
thopre marked an inline comment as done.
  • Remove unused ExpressionAST in parseNumericVariableDefinition
  • Fix numeric-expression.txt testcase after change of diagnostic message
  • Fix codestyle issues
This revision is now accepted and ready to land.Mon, Sep 2, 2:58 AM
This revision was automatically updated to reflect the committed changes.