This is an archive of the discontinued LLVM Phabricator instance.

[FileCheck] Fix -dump-input per-pattern diagnostic indexing
ClosedPublic

Authored by jdenny on Mar 2 2021, 3:16 PM.

Details

Summary

In input dump annotations, check:2'1 indicates diagnostic 1 for the
CHECK directive on check file line 2. Without this patch,
-dump-input computes the diagnostic index with the assumption that
FileCheck *consecutively* produces all diagnostics for the same
pattern. Already, that can be a false assumption, as in the examples
below. Moreover, it seems like a brittle assumption as FileCheck
evolves. Finally, it actually complicates the implementation even if
it makes it slightly more efficient.

This patch avoids that assumption. Examples below show results after
applying this patch. Before applying this patch, 'N is omitted
throughout these examples because the implementation doesn't notice
there's more than one diagnostic per pattern.

First, CHECK-LABEL violates the assumption because CHECK-LABEL
tries to match twice, and other directives can match in between:

$ cat check
CHECK: foobar
CHECK-LABEL: foobar

$ FileCheck -vv check < input |& tail -8
<<<<<<
           1: text
           2: foobar
label:2'0     ^~~~~~
check:1       ^~~~~~
label:2'1           X error: no match found
           3: text
>>>>>>

Second, --implicit-check-not is obviously processed many times among
other directives:

$ cat check
CHECK: foo
CHECK: foo

$ FileCheck -vv -dump-input=always -implicit-check-not=foo \
            check < input |& tail -16
<<<<<<
            1: text
not:imp1'0     X~~~~
            2: foo
check:1        ^~~
not:imp1'1        X
            3: text
not:imp1'1     ~~~~~
            4: foo
check:2        ^~~
not:imp1'2        X
            5: text
not:imp1'2     ~~~~~
            6:
eof:2          ^
>>>>>>

Diff Detail

Event Timeline

jdenny created this revision.Mar 2 2021, 3:16 PM
jdenny requested review of this revision.Mar 2 2021, 3:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2021, 3:16 PM
jdenny updated this revision to Diff 328237.Mar 4 2021, 10:45 AM
jdenny edited the summary of this revision. (Show Details)

Rebased and updated for D93341's effect on -dump-input.

Removed an unnecessary -allow-unused-prefixes.

The code change looks fine. If I'm honest, I don't really follow the testing, but that's likely due to a lack of familiarity of the dump-input output format. If @thopre gets a chance to review, it'd probably be best.

llvm/test/FileCheck/dump-input/annotations.txt
516–541

Are you aware of the split-file utility? It allows you to write these sorts of inputs without needing mass echo lines, all in the same test file.

LGTM otherwise.

llvm/test/FileCheck/dump-input/annotations.txt
556–593

That seems a bit brittle to assume there's always 2 irrelevant errors first. Also it's difficult to check for correctness without running the test manually to see what it produces. What's the actual output?

568

Why does labelB have only one annotation but there other labels have 2?

jdenny marked an inline comment as done.Mar 24 2021, 3:09 PM
jdenny added inline comments.
llvm/test/FileCheck/dump-input/annotations.txt
516–541

I should try that. Thanks!

However, that's a significant rewrite of this file. Do you mind if I tuck that away for NFC work at some unspecified future date?

556–593

As my comment on line 563 attempts to say, my goal was to check that "normal" error diagnostics are not suppressed by input dumps even though the latter repeats the same error diagnostics. To make the test easier to maintain, I figured it was sufficient to check that no diagnostics were omitted. I didn't check for extra diagnostics or incorrect text as all that should be tested elsewhere.

Maybe my testing strategy is wrong. Do you feel I should type out those error diagnostics? Or does the comment just need improvement?

And I botched the number: it should be 3 diagnostics. Maybe I should have a -implicit-check-not='error:' to make sure there are no extra diagnostics. And maybe I should use LAB-COUNT-3:.

568

Each CHECK-LABEL matches once to find the end of a section and then again as the last directive in the section, so matching occurs like this:

  • first section:
    • label:2'0 matches labelA to find the end of the section
    • check:1, the first directive in the section, matches text
    • label:2'1, the last directive in the section, matches labelA again
  • second section:
    • label:4 matches labelB to find the end of the section
    • check:3, the first directive in the section, fails to match, so no more directives in the section are processed
  • third section:
    • label:6'0 matches labelC
    • check:5 matches labelC
    • label:6'1 fails to match
  • fourth section:
    • label:8'0 matches labelD
    • not:7 requires the next directive in the section to be matched to establish its search range
    • thus, label:8'1 matches labelD
    • not:7 finds no match
  • fifth section:
    • label:10'0 matches labelE
    • not:9 requires the next to directive in the section to be matched to establish its search range
    • thus, label:10'1 matches labelE
    • not:9 unexpectedly matches textD
  • sixth section:
    • label:12'0 matches labelF
    • not:11 requires the next to directive in the section to be matched to establish its search range
    • thus, label:12'1 matches labelF
    • not:11 finds no match
jhenderson added inline comments.Mar 25 2021, 1:24 AM
llvm/test/FileCheck/dump-input/annotations.txt
516–541

Yes, that's fine. I imagine there are a number of FileCheck tests that might benefit from it.

thopre added inline comments.Mar 25 2021, 2:54 AM
llvm/test/FileCheck/dump-input/annotations.txt
556–593

I would spell them out then. For all I know, these two LAB directives could catch different errors than the one below (future FileCheck could emit some new error). If you don't want to hardcode the error message, some middle ground would be to match the line number of the error and perhaps a significant word in the error.

E.g.:

LAB: .chk:3:error:{{.*}}no match

For this case I thought the full error makes more sense.

568

Thank you for the detailed explanation. It's crystal clear now.

jdenny updated this revision to Diff 333461.Mar 25 2021, 4:37 PM
jdenny marked 4 inline comments as done.

Rebased, and addressed reviewer comments.

llvm/test/FileCheck/dump-input/annotations.txt
556–593

Maybe you're right. It at least makes it easier to understand.

I've adjusted the tests that this patch touches. I haven't adjusted other tests in this file. That can happen in another patch one day.

thopre accepted this revision.Mar 26 2021, 3:13 AM

LGTM, thanks!

This revision is now accepted and ready to land.Mar 26 2021, 3:13 AM
This revision was landed with ongoing or failed builds.Mar 27 2021, 7:44 AM
This revision was automatically updated to reflect the committed changes.