[PATCH] D60392: FileCheck [12/12]: Support use of var defined on same line

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre added a comment.

Hi Joel,

Thanks for your feedback, good job catching that!

In D60392#3181763 <https://reviews.llvm.org/D60392#3181763>, @jdenny wrote:

> I understand the desire to be able to define and reference a variable within the same directive.  However, as I understand it, this patch adds that ability in a way that introduces a new subtlety to FileCheck: the semantics of numeric variable references are then not always consistent.  If you assume the wrong semantics, you can end up with a false pass of a test.  I understand you've disabled the case of `CHECK-NOT`, but it does not appear to be the only case that can lead to false passes.
>
> Example
> -------
>
> Let's say we want to check that the very last value of y on an input line is equal to the first value of y plus 3.  Thus, the following input line is then erroneous:
>
>   x=20, y=30, z=40, x=21, y=31, z=41, x=22, y=33, z=42, x=23, y=32, z=43,
>
>
>
> Check version 1
> ---------------
>
>   CHECK: y=[[#VAR:]],{{.*}}y=[[#VAR+3]],
>
> The behavior is:
>
> - `y=[[#VAR:]],` matches `y=30,`.
> - `{{.*}}` is maximal munch.
> - So `y=[[#VAR+3]],` matches the last `y=` expression, which is `y=32,`.
>
> The directive then fails because `32` is not `30+3`.  OK, that's the exact behavior I was going for.  And now I think I understand how numeric variable references work.
>
> (If the above is not the behavior you expect after applying this patch, let me know.  Every example I tried after applying this patch produced seg faults or assertion failures, so I couldn't verify.)

I think the real issue here is using a wildcard pattern. I feel most often that not the user actually intended that. I remember someone asking for being able to use variables defined on the same line because using CHECK-SAME was different as pointed in your example below. See I think this is the behaviour people usually expect, judging from https://bugs.llvm.org/show_bug.cgi?id=30198 also about wildcard being a risky business.

> Check version 2
> ---------------
>
> Let's say I later realize I'd rather use `VAR` as defined to `30` by some earlier directive, so I rewrite check version 1 as follows:
>
>   CHECK: y=[[#VAR]],{{.*}}y=[[#VAR+3]],
>
> The behavior is:
>
> - `y=[[#VAR]],` is visibly differently than `y=[[#VAR:]],`, and its semantics are different too: it now matches a previously defined value instead of capturing a new value.  Nevertheless, it still matches `y=30,`.  So far, so good.
> - `{{.*}}` is still maximal munch.
> - Given that previous parts of the pattern match as before and `VAR` ends  up with the same value as before, shouldn't the semantics of `y=[[#VAR+3]],` be the same as before?  No.  It can only match `y=33,` now, and so that's what it does.
>
> The directive then accepts the erroneous input.  That's pretty subtle.

I think the main issue is the regex was badly written in the first place but I agree this patch is making things worse by having inconsistent behaviour. Someone ought to have written either of those two regex instead depending on intended pattern to be matched:

  CHECK: y=[[#VAR:]],{{.*}}y=[[#VAR+3]],{{[^y]*$}}

or

  CHECK: y=[[#VAR:]],{{[^y]*}}y=[[#VAR+3]],

I like the suggestion of CHECK-ASSERT or a new syntax. I'll think about it and in the meantime anyone with an opinion on this feel free to chime in.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60392/new/

https://reviews.llvm.org/D60392