[PATCH] D98086: [FileCheck] Fix numeric error propagation

Wed Mar 10 17:18:38 PST 2021

jdenny marked an inline comment as done.
jdenny added inline comments.

================
Comment at: llvm/test/FileCheck/match-time-error-propagation/matched-excluded-pattern.txt:10-18
+; TODO: Capturing from an excluded pattern probably shouldn't be permitted
+; because it seems useless: it's captured only if the pattern matches, but then
+; FileCheck fails.  The helpfulness of reporting overflow from that capture is
+; perhaps questionable then, but it doesn't seem harmful either.  Anyway, the
+; goal of this test is simply to exercise the error propagation mechanism for a
+; matched excluded pattern.  In the future, if we have a more interesting error
+; to exercise in that case, we should instead use it in this test, and then we
----------------
jdenny wrote:
> thopre wrote:
> > jdenny wrote:
> > > thopre wrote:
> > > > FYI I'd like to make a patch to use APInt in FileCheck to allow numbers bigger than 64bits. I've found the need while working on __builtin_isnan for long double. No timeline and I don't expect to have time on it soon but that would remove the only post-match error unless a new one is added before that. On the other hand I haven't even started that work so I think this testcase should be kept.
> > > Thanks for mentioning it.  Inequality constraints will fit here, right?  If so, hopefully they'll come first so it's easy to maintain this handling.
> > Inequality will need code needed for using a variable defined on the same line: the ability of tentatively match a line, set variables etc. and discard if a constraint is not met. I have an old patch that does it but I wanted something more automatic (i.e. require a transaction object to perform a match with a commit method and the destructor would delete everything unless committed. That's the general idea but everytime I try to compartmentalize FileCheck more (e.g. creating a numeric expression parser object) I get into an encapsulation hell with info I need but not easily available. Sorry for renting
> > 
> > Anyway, perhaps it's time for me to get back to the use of var defined on the same line and get closure on that FileCheck numeric expression patchset.
> Actually, based on the way `==` works, I suppose not.
I agree that FileCheck internals could benefit from some refactoring.

I now see three potential uses cases for these after-match errors (represented by `MatchFoundErrorNote` and reported by `printMatch` in this patch):

1. FileCheck internal limitations: the pattern matches as specified but FileCheck cannot handle it.  The only limitation right now is overflow/underflow, but that could be eliminated in the future, as you have pointed out.

2. A user-visible two-stage check of a directive's constraints: instead of searching for a match that satisfies all constraints of the directive, it searches for a match that satisfies only some constraints and then fails if the remaining constraints aren't satisfied by that match.  This would be confusing in ways we've discussed before.  For example, we once discussed doing this for a variable captured and later used in an expression within the same directive.  Given the input `1 0 2`, the following directive would then fail (`0` isn't `1 + 1`), but it seems better for it to succeed because, intuitively, the pattern should match (`2` is `1 + 1`):

    `CHECK: [[%u,X:]] {{.*}} [[%u,X+1]]`

    It's especially confusing how CHECK-NOT should then behave: with the current error handling in this patch, CHECK-NOT would also fail on a match not satisfying the remaining constraints.  Is that really the meaning of CHECK-NOT?  So, use case 2 seems wrong, and your comments suggest you're not planning to pursue it for this example.  From the user's view, it will then seem like one stage.

3. What about division by zero for a variable captured and used in the same directive?  For example, given the input `0 / 0 = 0`, should the following produce an after-match error, or should FileCheck keep looking for another match?

    `CHECK: [[#%u,X:]] / [[#%u,Y:]] = [[%u:X/Y]]`

    What should CHECK-NOT do?  It seems to me FileCheck should fail in both cases because it cannot compute what should actually match.  What do you think?

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

https://reviews.llvm.org/D98086