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

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added inline comments.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:107
+    /// Indicates an error during match.
+    MatchError,
     /// Indicates a good match for an expected pattern.
----------------
jdenny wrote:
> The name `MatchError` and its comment seem too general.  It could easily describe many of the following enumerators.  I suggest:
> 
> ```
>     /// Indicates a match that can never succeed because the pattern is found to
>     /// be undefined at match time (e.g., contains an undefined variable).
>     MatchUndefined,
> ```
> 
> Undefined variables are probably the only such case we have right now, and maybe that will always be true, but I'm trying to ensure this can handle more cases if needed in the future.
> 
> Also, the middle paragraph in the preceding comments is now incorrect.  Here's a possible revision:
> 
> ```
>   /// A directive's supplied pattern is said to be either undefined, expected,
>   /// or excluded depending on whether the pattern is well formed at match time
>   /// and whether it must have or must not have a match in order for the 
>   /// directive to succeed.  For example, a CHECK directive's pattern is 
>   /// normally expected, a CHECK-NOT directive's pattern is normally excluded, 
>   /// but either directive's pattern is undefined if it contains an undefined
>   /// variable at match time.  The match result type MatchUndefined is for 
>   /// undefined patterns.  All match result types whose names end with 
>   /// "Excluded" are for excluded patterns.  All other match result types are 
>   /// for expected patterns.
> ```
> 
> Here, I'm assuming you consistently use the new enumerator for undefined variables, regardless of whether a directive is positive or negative.  I haven't checked the implementation, but the test suite changes seems to confirm that.
Another possibility that might be clearer is `MatchPatternMalformed`.


================
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:>>>>>>
----------------
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.)


================
Comment at: llvm/utils/FileCheck/FileCheck.cpp:201
+  case FileCheckDiag::MatchError:
+    return MarkerStyle('X', raw_ostream::RED, "error: match error",
+                       /*FiltersAsError=*/true);
----------------
jdenny wrote:
> If you agree to changing `MatchError` to `MatchUndefined`, I suggest changing `error: match error` to `error: match undefined`.
And `MatchPatternMalformed` would obviously become something like `error: match pattern malformed`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86222