[PATCH] D86222: Fix PR46880: Fail CHECK-NOT with undefined variable

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

thopre added inline comments.


================
Comment at: llvm/test/FileCheck/dump-input-annotations.txt:626
+; SUBST-POS-NEXT:check:2'0     X~~~~~~~~~~~~~~~~~~~~~ error: match error
+; SUBST-POS-NEXT:check:2'1                            uses undefined variable: "UNDEF"
 ; SUBST-POS-NEXT:>>>>>>
----------------
jdenny wrote:
> thopre wrote:
> > jdenny wrote:
> > > thopre wrote:
> > > > jdenny wrote:
> > > > > This test used to cover printing of variable definitions when a positive directive fails.  It no longer does.
> > > > > 
> > > > > A separate test could be added to cover that case.  But do we need to drop that info when a variable is undefined?  It could be helpful for debugging.
> > > > We currently only print substitutions if there are no errors. I think if we want to print substitutions in case of undefined variable we should do it regardless of whether there are errors to be consistent. Obviously that means ignoring substitution errors in printSubstitutions
> > > What's an example of an error besides undefined variable for which we currently don't print substitutions but you're suggesting we would?
> > Overflow and underflow come to mind. There might be others.
> > 
> > ```
> > % cat overflow_undef.test
> > BIGVAR=10000000000000000
> > CHECK: BIGVAR: [[#BIGVAR:0x8000000000000000+0x8000000000000000]] [[#UNDEFVAR]]
> > % ./bin/FileCheck --dump-input=never --input-file overflow_undef.test overflow_undef.test
> > overflow_undef.test:2:19: error: unable to substitute variable or numeric expression: overflow error
> > CHECK: BIGVAR: [[#BIGVAR:0x8000000000000000+0x8000000000000000]] [[#UNDEFVAR]]
> >                   ^
> > zsh: exit 1     ./bin/FileCheck --dump-input=never --input-file overflow_undef.test
> > ```
> In the case of overflow/underflow, I'm in favor of printing used variable definitions.  It would be especially helpful if it would print any variable definitions used in the expression that overflowed/underflowed, but printing other used variable definitions could be helpful as well.
> 
> Your example is a bit different than the case I was pointing out originally: in your example, there are no used  variable definitions to report.  There is however an undefined variable.  I am also in favor of printing all errors (e.g., overflow and undefined variable) from a directive instead of just the first (overflow), where error recovery makes that feasible.
> 
> Again, if supporting all this info isn't feasible, then please at least add another test so we don't lose coverage of `-dump-input` printing used variable definitions when a positive directive with a well formed pattern fails to match.
> 
> (By the way, I just tried a few examples (without the current patch), and I'm noticing issues with overflow errors with respect to `-dump-input`.  The traditional error message doesn't print undefined variables, but `-dump-input` does.  `-dump-input` reports `error: no match found` twice and doesn't report the overflow.  My master is a couple of months old already, so ignore me if all this has been fixed.)
Does this updated patch solve your concern? Ignore all the tests failures, I still haven't gotten round to look at those.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86222