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

[jdenny]

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          ^
>>>>>>

[jdenny]

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

Removed an unnecessary -allow-unused-prefixes.

[jdenny]

Ping.

[jhenderson]

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.

[thopre]

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]

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]

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]

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]

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]

LGTM, thanks!

llvm/test/FileCheck/dump-input/annotations.txt

 ; DAG1L-VQ-NEXT:dag:5                     X error: no match found
 ; DAG1L-VV-NEXT:dag:5'2                   X error: no match found
 ;    DAG1L-NEXT:>>>>>>
 ;     DAG1L-NOT:{{.}}
 
 ;--------------------------------------------------
 ; CHECK-LABEL
 ;
-; FIXME: Labels sometimes produce redundant diagnostics for good matches.
-; That bug is independent of but affects -dump-input.
-;--------------------------------------------------
-
-; Good match and no match.
-
-; RUN: echo 'lab0' > %t.in
-; RUN: echo 'foo' >> %t.in
-; RUN: echo 'lab1' >> %t.in
-; RUN: echo 'bar' >> %t.in
-
-; RUN: echo 'CHECK-LABEL: lab0' > %t.chk
-; RUN: echo 'CHECK: foo' >> %t.chk
-; RUN: echo 'CHECK-LABEL: lab2' >> %t.chk
+; Each CHECK-LABEL is processed twice: once before other patterns in the
+; preceding section, and once afterward.
+;
+; As expected, the search range for a negative pattern preceding a CHECK-LABEL
+; ends at the start of the CHECK-LABEL match.  not:7 and not:11 below
+; demonstrate this behavior.
+;
+; The search range for a positive pattern preceding a CHECK-LABEL ends at the
+; end of the CHECK-LABEL match.  check:3 and check:5 below demonstrate this
+; behavior.  As in the case of check:5, an effect of this behavior is that the
+; second CHECK-LABEL match might fail even though the first succeeded.
+;
+; FIXME: It seems like the search range for such a positive pattern should be
+; the same as in the case of a negative pattern.  Note that -dump-input is
+; correct here.  It's the matching behavior that's strange.
+;--------------------------------------------------
+
+; RUN: echo 'text'   >  %t.in
+; RUN: echo 'labelA' >> %t.in
+; RUN: echo 'textA'  >> %t.in
+; RUN: echo 'labelB' >> %t.in
+; RUN: echo 'textB'  >> %t.in
+; RUN: echo 'labelC' >> %t.in
+; RUN: echo 'textC'  >> %t.in
+; RUN: echo 'labelD' >> %t.in
+; RUN: echo 'textD'  >> %t.in
+; RUN: echo 'labelE' >> %t.in
+; RUN: echo 'textE'  >> %t.in
+; RUN: echo 'labelF' >> %t.in
+
+; RUN: echo 'CHECK: text'         >  %t.chk
+; RUN: echo 'CHECK-LABEL: labelA' >> %t.chk
+; RUN: echo 'CHECK: foobar'       >> %t.chk
+; RUN: echo 'CHECK-LABEL: labelB' >> %t.chk
+; RUN: echo 'CHECK: labelC'       >> %t.chk
+; RUN: echo 'CHECK-LABEL: labelC' >> %t.chk
+; RUN: echo 'CHECK-NOT: foobar'   >> %t.chk
+; RUN: echo 'CHECK-LABEL: labelD' >> %t.chk
+; RUN: echo 'CHECK-NOT: textD'    >> %t.chk
+; RUN: echo 'CHECK-LABEL: labelE' >> %t.chk
+; RUN: echo 'CHECK-NOT: labelF'   >> %t.chk
+; RUN: echo 'CHECK-LABEL: labelF' >> %t.chk
 
 ; RUN: %ProtectFileCheckOutput \
 ; RUN: not FileCheck -dump-input=always -input-file %t.in %t.chk 2>&1 \
-; RUN: | FileCheck -match-full-lines %s -check-prefixes=LAB \
+; RUN: | FileCheck -match-full-lines %s -check-prefixes=LAB,LAB-Q \
 ; RUN:             -implicit-check-not='remark:'
 ; RUN: %ProtectFileCheckOutput \
 ; RUN: not FileCheck -dump-input=always -input-file %t.in %t.chk -v 2>&1 \
 ; RUN: | FileCheck -match-full-lines %s -check-prefixes=LAB,LAB-V \
 ; RUN:             -implicit-check-not='remark:'
 ; RUN: %ProtectFileCheckOutput \
 ; RUN: not FileCheck -dump-input=always -input-file %t.in %t.chk -vv 2>&1 \
 ; RUN: | FileCheck -match-full-lines %s -check-prefixes=LAB,LAB-V,LAB-VV \
 ; RUN:             -implicit-check-not='remark:' -allow-unused-prefixes
 
 ; Verbose diagnostics are suppressed but not errors.
-; LAB: {{.*}}error:{{.*}}
-; LAB: {{.*}}possible intended match{{.*}}
+; LAB:{{.*}}error:{{.*}}
+; LAB:{{.*}}error:{{.*}}
 
-; LAB:         <<<<<<
-; LAB-NEXT:               1: lab0
-; LAB-V-NEXT:  label:1'0     ^~~~
-; LAB-V-NEXT:  label:1'1     ^~~~
-; LAB-NEXT:               2: foo
-; LAB-NEXT:    label:3'0     X~~ error: no match found
-; LAB-NEXT:               3: lab1
-; LAB-NEXT:    label:3'0     ~~~~
-; LAB-NEXT:    label:3'1     ?    possible intended match
-; LAB-NEXT:               4: bar
-; LAB-NEXT:    label:3'0     ~~~
-; LAB-NEXT:    >>>>>>
-; LAB-NOT:     {{.}}
+;         LAB:<<<<<<
+;    LAB-NEXT:            1: text
+;  LAB-V-NEXT:check:1        ^~~~
+;    LAB-NEXT:            2: labelA
+;  LAB-V-NEXT:label:2'0      ^~~~~~
+;  LAB-V-NEXT:label:2'1      ^~~~~~
+;    LAB-NEXT:            3: textA
+;    LAB-NEXT:check:3        X~~~~ error: no match found
+;    LAB-NEXT:            4: labelB
+;  LAB-V-NEXT:label:4        ^~~~~~
+;    LAB-NEXT:check:3        ~~~~~~
+;    LAB-NEXT:            5: textB
+;    LAB-NEXT:            6: labelC
+;  LAB-V-NEXT:label:6'0      ^~~~~~
+;  LAB-V-NEXT:check:5        ^~~~~~
+;  LAB-Q-NEXT:label:6              X error: no match found
+;  LAB-V-NEXT:label:6'1            X error: no match found
+;    LAB-NEXT:            7: textC
+; LAB-VV-NEXT:not:7          X~~~~
+;    LAB-NEXT:            8: labelD
+;  LAB-V-NEXT:label:8'0      ^~~~~~
+;  LAB-V-NEXT:label:8'1      ^~~~~~
+;    LAB-NEXT:            9: textD
+;    LAB-NEXT:not:9          !~~~~ error: no match expected
+;    LAB-NEXT:           10: labelE
+;  LAB-V-NEXT:label:10'0     ^~~~~~
+;  LAB-V-NEXT:label:10'1     ^~~~~~
+;    LAB-NEXT:           11: textE
+; LAB-VV-NEXT:not:11         X~~~~
+;    LAB-NEXT:           12: labelF
+;  LAB-V-NEXT:label:12'0     ^~~~~~
+;  LAB-V-NEXT:label:12'1     ^~~~~~
+;    LAB-NEXT:>>>>>>
+;     LAB-NOT:{{.}}
 
 ;--------------------------------------------------
 ; --implicit-check-not
 ;
 ; The first two --implicit-check-not patterns have no match (success).  The
 ; third has an unexpected match (error).  To check per-input-line annotation
 ; sorting, all of those plus the CHECK directives have annotations on the same
 ; input line.
 ;--------------------------------------------------
 
 ; RUN: echo 'hello world again!' > %t.in
 
 ; RUN: echo 'CHECK: hel' > %t.chk
 ; RUN: echo 'CHECK: wor' >> %t.chk
 ; RUN: echo 'CHECK: !' >> %t.chk
 
+; Prefixes used here:
+; IMPNOT    = quiet, -v, or -vv
+; IMPNOT-Q  = quiet
+; IMPNOT-V  = -v or -vv (-vv implies -v)
+; IMPNOT-VQ = -v and not -vv
+; IMPNOT-VV = -vv
+
 ; RUN: %ProtectFileCheckOutput \
 ; RUN: not FileCheck -dump-input=always -input-file=%t.in %t.chk 2>&1 \
 ; RUN:               --implicit-check-not='goodbye' \
 ; RUN:               --implicit-check-not='world' \
 ; RUN:               --implicit-check-not='again' \
-; RUN: | FileCheck -match-full-lines %s -check-prefix=IMPNOT \
+; RUN: | FileCheck -match-full-lines %s -check-prefixes=IMPNOT,IMPNOT-Q \
 ; RUN:             -implicit-check-not='remark:'
 ; RUN: %ProtectFileCheckOutput \
 ; RUN: not FileCheck -dump-input=always -input-file=%t.in %t.chk -v 2>&1 \
 ; RUN:               --implicit-check-not='goodbye' \
 ; RUN:               --implicit-check-not='world' \
 ; RUN:               --implicit-check-not='again' \
-; RUN: | FileCheck -match-full-lines %s -check-prefixes=IMPNOT,IMPNOT-V \
+; RUN: | FileCheck -match-full-lines %s \
+; RUN:             -check-prefixes=IMPNOT,IMPNOT-V,IMPNOT-VQ \
 ; RUN:             -implicit-check-not='remark:'
 ; RUN: %ProtectFileCheckOutput \
 ; RUN: not FileCheck -dump-input=always -input-file=%t.in %t.chk -vv 2>&1 \
 ; RUN:               --implicit-check-not='goodbye' \
 ; RUN:               --implicit-check-not='world' \
 ; RUN:               --implicit-check-not='again' \
 ; RUN: | FileCheck -match-full-lines %s \
 ; RUN:             -check-prefixes=IMPNOT,IMPNOT-V,IMPNOT-VV \
 ; RUN:             -implicit-check-not='remark:'
 
 ; Verbose diagnostics are suppressed but not errors.
 ; IMPNOT:{{.*}}error:{{.*}}
 
 ;         IMPNOT:<<<<<<
-;    IMPNOT-NEXT:          1: hello world again!
-;  IMPNOT-V-NEXT:check:1      ^~~
-; IMPNOT-VV-NEXT:not:imp1     X
-; IMPNOT-VV-NEXT:not:imp2     X
-; IMPNOT-VV-NEXT:not:imp3     X
-;  IMPNOT-V-NEXT:check:2            ^~~
-; IMPNOT-VV-NEXT:not:imp1        X~~
-; IMPNOT-VV-NEXT:not:imp2        X~~
-; IMPNOT-VV-NEXT:not:imp3        X~~
-;  IMPNOT-V-NEXT:check:3                       ^
-; IMPNOT-VV-NEXT:not:imp1              X~~~~~~~
-; IMPNOT-VV-NEXT:not:imp2              X~~~~~~~
-;    IMPNOT-NEXT:not:imp3                 !~~~~  error: no match expected
+;    IMPNOT-NEXT:            1: hello world again!
+;  IMPNOT-V-NEXT:check:1        ^~~
+; IMPNOT-VV-NEXT:not:imp1'0     X
+; IMPNOT-VV-NEXT:not:imp2'0     X
+; IMPNOT-VV-NEXT:not:imp3'0     X
+;  IMPNOT-V-NEXT:check:2              ^~~
+; IMPNOT-VV-NEXT:not:imp1'1        X~~
+; IMPNOT-VV-NEXT:not:imp2'1        X~~
+; IMPNOT-VV-NEXT:not:imp3'1        X~~
+;  IMPNOT-V-NEXT:check:3                         ^
+; IMPNOT-VV-NEXT:not:imp1'2              X~~~~~~~
+; IMPNOT-VV-NEXT:not:imp2'2              X~~~~~~~
+;  IMPNOT-Q-NEXT:not:imp3                   !~~~~  error: no match expected
+; IMPNOT-VQ-NEXT:not:imp3                   !~~~~  error: no match expected
+; IMPNOT-VV-NEXT:not:imp3'2                 !~~~~  error: no match expected
 ;    IMPNOT-NEXT:>>>>>>
 ;     IMPNOT-NOT:{{.}}
 
 ;--------------------------------------------------
 ; Substitutions: successful and failed positive directives.
 ;--------------------------------------------------
 
 ; RUN: echo 'def-match1 def-match2'  >  %t.in

llvm/utils/FileCheck/FileCheck.cpp

 
 #include "llvm/FileCheck/FileCheck.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cmath>
+#include <map>
 using namespace llvm;
 
 static cl::extrahelp FileCheckOptsEnv(
     "\nOptions are parsed from the environment variable FILECHECK_OPTS and\n"
     "from the command line.\n");
 
 static cl::opt<std::string>
     CheckFilename(cl::Positional, cl::desc("<check-file>"), cl::Optional);

 }
 
 static void
 BuildInputAnnotations(const SourceMgr &SM, unsigned CheckFileBufferID,
                       const std::pair<unsigned, unsigned> &ImpPatBufferIDRange,
                       const std::vector<FileCheckDiag> &Diags,
                       std::vector<InputAnnotation> &Annotations,
                       unsigned &LabelWidth) {
-  // How many diagnostics have we seen so far?
-  unsigned DiagCount = 0;
-  // How many diagnostics has the current check seen so far?
-  unsigned CheckDiagCount = 0;
+  struct CompareSMLoc {
+    bool operator()(const SMLoc &LHS, const SMLoc &RHS) {
+      return LHS.getPointer() < RHS.getPointer();
+    }
+  };
+  // How many diagnostics does each pattern have?
+  std::map<SMLoc, unsigned, CompareSMLoc> DiagCountPerPattern;
+  for (auto Diag : Diags)
+    ++DiagCountPerPattern[Diag.CheckLoc];
+  // How many diagnostics have we seen so far per pattern?
+  std::map<SMLoc, unsigned, CompareSMLoc> DiagIndexPerPattern;
+  // How many total diagnostics have we seen so far?
+  unsigned DiagIndex = 0;
   // What's the widest label?
   LabelWidth = 0;
   for (auto DiagItr = Diags.begin(), DiagEnd = Diags.end(); DiagItr != DiagEnd;
        ++DiagItr) {
     InputAnnotation A;
-    A.DiagIndex = DiagCount++;
+    A.DiagIndex = DiagIndex++;
 
     // Build label, which uniquely identifies this check result.
     unsigned CheckBufferID = SM.FindBufferContainingLoc(DiagItr->CheckLoc);
     auto CheckLineAndCol =
         SM.getLineAndColumn(DiagItr->CheckLoc, CheckBufferID);
     llvm::raw_string_ostream Label(A.Label);
     Label << GetCheckTypeAbbreviation(DiagItr->CheckTy) << ":";
     if (CheckBufferID == CheckFileBufferID)
       Label << CheckLineAndCol.first;
     else if (ImpPatBufferIDRange.first <= CheckBufferID &&
              CheckBufferID < ImpPatBufferIDRange.second)
       Label << "imp" << (CheckBufferID - ImpPatBufferIDRange.first + 1);
     else
       llvm_unreachable("expected diagnostic's check location to be either in "
                        "the check file or for an implicit pattern");
-    unsigned CheckDiagIndex = UINT_MAX;
-    auto DiagNext = std::next(DiagItr);
-    if (DiagNext != DiagEnd && DiagItr->CheckTy == DiagNext->CheckTy &&
-        DiagItr->CheckLoc == DiagNext->CheckLoc)
-      CheckDiagIndex = CheckDiagCount++;
-    else if (CheckDiagCount) {
-      CheckDiagIndex = CheckDiagCount;
-      CheckDiagCount = 0;
-    }
-    if (CheckDiagIndex != UINT_MAX)
-      Label << "'" << CheckDiagIndex;
+    if (DiagCountPerPattern[DiagItr->CheckLoc] > 1)
+      Label << "'" << DiagIndexPerPattern[DiagItr->CheckLoc]++;
     Label.flush();
     LabelWidth = std::max((std::string::size_type)LabelWidth, A.Label.size());
 
     A.Marker = GetMarker(DiagItr->MatchTy);
     if (!DiagItr->Note.empty()) {
       A.Marker.Note = DiagItr->Note;
       // It's less confusing if notes that don't actually have ranges don't have
       // markers.  For example, a marker for 'with "VAR" equal to "5"' would