[FileCheck] Fix --dump-input implicit pattern location

[jdenny]

Summary

Currently, --dump-input implies that all --implicit-check-not
patterns appear on line 1 by printing annotations like:

       1: foo bar baz 
not:1         !~~     error: no match expected

This patch changes that to:

          1: foo bar baz 
not:imp1         !~~     error: no match expected

imp1 indicates the first --implicit-check-not pattern.

[jdenny]

Applied clang-format.

[jdenny]

I'm new to working with Harbormaster. Do I manually force it to re-run after updating a patch?

[thopre]

In D77605#1970014, @jdenny wrote:

I'm new to working with Harbormaster. Do I manually force it to re-run after updating a patch?

I've seen it do it automatically in other phabricator instances but that doesn't seem to be the case here. Might be configuration-dependent. Better force a rebuild to be sure.

llvm/utils/FileCheck/FileCheck.cpp

321–323

Is this going to be stable if a new option that require a buffer gets added? I know the tests would catch this if that's the case but I'd prefer to not have to update this formula.

I'm also confused at the comment. Does it match the formula? All I see in the formula is that the implicit buffer ID is the check file buffer minus one. Does the comment need updating? Actually repeating the formula does not seem useful, I'd rather have an explanation as to why the implicit buffer ID can be obtained with this formula.

[jdenny]

In D77605#1970617, @thopre wrote:

In D77605#1970014, @jdenny wrote:

I'm new to working with Harbormaster. Do I manually force it to re-run after updating a patch?

I've seen it do it automatically in other phabricator instances but that doesn't seem to be the case here. Might be configuration-dependent. Better force a rebuild to be sure.

Thanks. I clicked restart. We'll see what happens.

llvm/utils/FileCheck/FileCheck.cpp

321–323

Is this going to be stable if a new option that require a buffer gets added? I know the tests would catch this if that's the case but I'd prefer to not have to update this formula.

Good point. I suppose I could pass around the buffer ID range for implicit buffers.

Of course, if new buffers are added for patterns, this code likely has to be updated to process annotations for those new buffers anyway. But your suggestion would at least address the case where a new buffer is added for some other purpose.

I'm also confused at the comment. Does it match the formula?

It does.

All I see in the formula is that the implicit buffer ID is the check file buffer

It's not the CheckFileBufferID, it's the CheckBufferID. Perhaps I should rename the latter to BufferID so it's more distinct?

minus one.

I could rewrite it as - 2 + 1. That is, - 2 is for the input file buffer and check file buffer, and + 1 converts from zero-origin to one-origin indexing. Would that help?

Well, it's moot if I pass in the implicit buffer ID range.

[jdenny]

Addressed reviewer comments.

[jdenny]

clang-tidy (via Harbormaster) is saying BuildInputAnnotations should be buildInputAnnotations. However, this patch does not introduce that function, and it follows the style in the rest of the file, so any fix should be in a separate patch.

[thopre]

Much nicer with that change, thanks! LGTM

[jdenny]

In D77605#1984191, @thopre wrote:

Much nicer with that change, thanks!

Agreed. Thanks for the review!

llvm/include/llvm/Support/FileCheck.h

public:
   std::string getDescription(StringRef Prefix) const;
 };
 } // namespace Check
 
 struct FileCheckDiag {
   /// What is the FileCheck directive for this diagnostic?
   Check::FileCheckType CheckTy;
   /// Where is the FileCheck directive for this diagnostic?
-  unsigned CheckLine, CheckCol;
+  SMLoc CheckLoc;
   /// What type of match result does this diagnostic describe?
   ///
   /// A directive's supplied pattern is said to be either expected or excluded
   /// depending on whether the pattern must have or must not have a match in
   /// order for the directive to succeed.  For example, a CHECK directive's
   /// pattern is expected, and a CHECK-NOT directive's pattern is excluded.
   /// All match result types whose names end with "Excluded" are for excluded
   /// patterns, and all others are for expected patterns.
public:
   // library.
   Regex buildCheckPrefixRegex();
 
   /// Reads the check file from \p Buffer and records the expected strings it
   /// contains. Errors are reported against \p SM.
   ///
   /// Only expected strings whose prefix is one of those listed in \p PrefixRE
   /// are recorded. \returns true in case of an error, false otherwise.
-  bool readCheckFile(SourceMgr &SM, StringRef Buffer, Regex &PrefixRE);
+  ///
+  /// If \p ImpPatBufferIDRange, then the range (inclusive start, exclusive end)
+  /// of IDs for source buffers added to \p SM for implicit patterns are
+  /// recorded in it.  The range is empty if there are none.
+  bool
+  readCheckFile(SourceMgr &SM, StringRef Buffer, Regex &PrefixRE,
+                std::pair<unsigned, unsigned> *ImpPatBufferIDRange = nullptr);
 
   bool ValidateCheckPrefixes();
 
   /// Canonicalizes whitespaces in the file. Line endings are replaced with
   /// UNIX-style '\n'.
   StringRef CanonicalizeFile(MemoryBuffer &MB,
                              SmallVectorImpl<char> &OutputBuffer);
 

llvm/lib/Support/FileCheck.cpp

StringRef FileCheck::CanonicalizeFile(MemoryBuffer &MB,
   OutputBuffer.push_back('\0');
   return StringRef(OutputBuffer.data(), OutputBuffer.size() - 1);
 }
 
 FileCheckDiag::FileCheckDiag(const SourceMgr &SM,
                              const Check::FileCheckType &CheckTy,
                              SMLoc CheckLoc, MatchType MatchTy,
                              SMRange InputRange)
-    : CheckTy(CheckTy), MatchTy(MatchTy) {
+    : CheckTy(CheckTy), CheckLoc(CheckLoc), MatchTy(MatchTy) {
   auto Start = SM.getLineAndColumn(InputRange.Start);
   auto End = SM.getLineAndColumn(InputRange.End);
   InputStartLine = Start.first;
   InputStartCol = Start.second;
   InputEndLine = End.first;
   InputEndCol = End.second;
-  Start = SM.getLineAndColumn(CheckLoc);
-  CheckLine = Start.first;
-  CheckCol = Start.second;
 }
 
 static bool IsPartOfWord(char c) {
   return (isalnum(c) || c == '-' || c == '_');
 }
 
 Check::FileCheckType &Check::FileCheckType::setCount(int C) {
   assert(Count > 0 && "zero and negative counts are not supported");

 }
 
 FileCheck::FileCheck(FileCheckRequest Req)
     : Req(Req), PatternContext(std::make_unique<FileCheckPatternContext>()),
       CheckStrings(std::make_unique<std::vector<FileCheckString>>()) {}
 
 FileCheck::~FileCheck() = default;
 
-bool FileCheck::readCheckFile(SourceMgr &SM, StringRef Buffer,
-                              Regex &PrefixRE) {
+bool FileCheck::readCheckFile(
+    SourceMgr &SM, StringRef Buffer, Regex &PrefixRE,
+    std::pair<unsigned, unsigned> *ImpPatBufferIDRange) {
+  if (ImpPatBufferIDRange)
+    ImpPatBufferIDRange->first = ImpPatBufferIDRange->second = 0;
+
   Error DefineError =
       PatternContext->defineCmdlineVariables(Req.GlobalDefines, SM);
   if (DefineError) {
     logAllUnhandledErrors(std::move(DefineError), errs());
     return true;
   }
 
   PatternContext->createLineVariable();
 
   std::vector<Pattern> ImplicitNegativeChecks;
   for (const auto &PatternString : Req.ImplicitCheckNot) {
     // Create a buffer with fake command line content in order to display the
     // command line option responsible for the specific implicit CHECK-NOT.
     std::string Prefix = "-implicit-check-not='";
     std::string Suffix = "'";
     std::unique_ptr<MemoryBuffer> CmdLine = MemoryBuffer::getMemBufferCopy(
         Prefix + PatternString + Suffix, "command line");
 
     StringRef PatternInBuffer =
         CmdLine->getBuffer().substr(Prefix.size(), PatternString.size());
-    SM.AddNewSourceBuffer(std::move(CmdLine), SMLoc());
+    unsigned BufferID = SM.AddNewSourceBuffer(std::move(CmdLine), SMLoc());
+    if (ImpPatBufferIDRange) {
+      if (ImpPatBufferIDRange->first == ImpPatBufferIDRange->second) {
+        ImpPatBufferIDRange->first = BufferID;
+        ImpPatBufferIDRange->second = BufferID + 1;
+      } else {
+        assert(BufferID == ImpPatBufferIDRange->second &&
+               "expected consecutive source buffer IDs");
+        ++ImpPatBufferIDRange->second;
+      }
+    }
 
     ImplicitNegativeChecks.push_back(
         Pattern(Check::CheckNot, PatternContext.get()));
     ImplicitNegativeChecks.back().parsePattern(PatternInBuffer,
                                                "IMPLICIT-CHECK", SM, Req);
   }
 
   std::vector<Pattern> DagNotMatches = ImplicitNegativeChecks;

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

 ; 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:     {{.}}
+
+;--------------------------------------------------
+; --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
+
+; 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:             -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:             -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:{{.*}}
+
+; FIXME: All occurrences of imp1, imp2, and imp3 are sorting after the first
+; directive.  They should instead be sorted by when they execute.
+
+;         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-VV-NEXT:not:imp1        X~~
+; IMPNOT-VV-NEXT:not:imp2        X~~
+; IMPNOT-VV-NEXT:not:imp3        X~~
+; IMPNOT-VV-NEXT:not:imp1              X~~~~~~~
+; IMPNOT-VV-NEXT:not:imp2              X~~~~~~~
+;    IMPNOT-NEXT:not:imp3                 !~~~~  error: no match expected
+;  IMPNOT-V-NEXT:check:2            ^~~
+;  IMPNOT-V-NEXT:check:3                       ^
+;    IMPNOT-NEXT:>>>>>>
+;     IMPNOT-NOT:{{.}}

llvm/utils/FileCheck/FileCheck.cpp

static void DumpInputAnnotationHelp(raw_ostream &OS) {
   // Labels for input lines.
   OS << "  - ";
   WithColor(OS, raw_ostream::SAVEDCOLOR, true) << "L:";
   OS << "     labels line number L of the input file\n";
 
   // Labels for annotation lines.
   OS << "  - ";
   WithColor(OS, raw_ostream::SAVEDCOLOR, true) << "T:L";
-  OS << "    labels the only match result for a pattern of type T from "
-     << "line L of\n"
-     << "           the check file\n";
+  OS << "    labels the only match result for either (1) a pattern of type T"
+     << " from\n"
+     << "           line L of the check file if L is an integer or (2) the"
+     << " I-th implicit\n"
+     << "           pattern if L is \"imp\" followed by an integer "
+     << "I (index origin one)\n";
   OS << "  - ";
   WithColor(OS, raw_ostream::SAVEDCOLOR, true) << "T:L'N";
-  OS << "  labels the Nth match result for a pattern of type T from line "
-     << "L of\n"
-     << "           the check file\n";
+  OS << "  labels the Nth match result for such a pattern\n";
 
   // Markers on annotation lines.
   OS << "  - ";
   WithColor(OS, raw_ostream::SAVEDCOLOR, true) << "^~~";
   OS << "    marks good match (reported if -v)\n"
      << "  - ";
   WithColor(OS, raw_ostream::SAVEDCOLOR, true) << "!~~";
   OS << "    marks bad match, such as:\n"
std::string GetCheckTypeAbbreviation(Check::FileCheckType Ty) {
   case Check::CheckBadCount:
     return "bad-count";
   case Check::CheckNone:
     llvm_unreachable("invalid FileCheckType");
   }
   llvm_unreachable("unknown FileCheckType");
 }
 
-static void BuildInputAnnotations(const std::vector<FileCheckDiag> &Diags,
-                                  std::vector<InputAnnotation> &Annotations,
-                                  unsigned &LabelWidth) {
+static void
+BuildInputAnnotations(const SourceMgr &SM, unsigned CheckFileBufferID,
+                      const std::pair<unsigned, unsigned> &ImpPatBufferIDRange,
+                      const std::vector<FileCheckDiag> &Diags,
   // How many diagnostics has the current check seen so far?
   unsigned CheckDiagCount = 0;
   // What's the widest label?
   LabelWidth = 0;
   for (auto DiagItr = Diags.begin(), DiagEnd = Diags.end(); DiagItr != DiagEnd;
        ++DiagItr) {
     InputAnnotation A;
 
     // Build label, which uniquely identifies this check result.
-    A.CheckLine = DiagItr->CheckLine;
+    unsigned CheckBufferID = SM.FindBufferContainingLoc(DiagItr->CheckLoc);
+    auto CheckLineAndCol =
+        SM.getLineAndColumn(DiagItr->CheckLoc, CheckBufferID);
+    A.CheckLine = CheckLineAndCol.first;
     llvm::raw_string_ostream Label(A.Label);
-    Label << GetCheckTypeAbbreviation(DiagItr->CheckTy) << ":"
-          << DiagItr->CheckLine;
+    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");
     A.CheckDiagIndex = UINT_MAX;
     auto DiagNext = std::next(DiagItr);
     if (DiagNext != DiagEnd && DiagItr->CheckTy == DiagNext->CheckTy &&
-        DiagItr->CheckLine == DiagNext->CheckLine)
+        DiagItr->CheckLoc == DiagNext->CheckLoc)
       A.CheckDiagIndex = CheckDiagCount++;
     else if (CheckDiagCount) {
       A.CheckDiagIndex = CheckDiagCount;
       CheckDiagCount = 0;
     }
     if (A.CheckDiagIndex != UINT_MAX)
       Label << "'" << A.CheckDiagIndex;
     else
errs() << "Could not open check file '" << CheckFilename
            << "': " << EC.message() << '\n';
     return 2;
   }
   MemoryBuffer &CheckFile = *CheckFileOrErr.get();
 
   SmallString<4096> CheckFileBuffer;
   StringRef CheckFileText = FC.CanonicalizeFile(CheckFile, CheckFileBuffer);
 
+  unsigned CheckFileBufferID =
       SM.AddNewSourceBuffer(MemoryBuffer::getMemBuffer(
                                 CheckFileText, CheckFile.getBufferIdentifier()),
                             SMLoc());
 
-  if (FC.readCheckFile(SM, CheckFileText, PrefixRE))
+  std::pair<unsigned, unsigned> ImpPatBufferIDRange;
+  if (FC.readCheckFile(SM, CheckFileText, PrefixRE, &ImpPatBufferIDRange))
     return 2;
 
   // Open the file to check and add it to SourceMgr.
   ErrorOr<std::unique_ptr<MemoryBuffer>> InputFileOrErr =
       MemoryBuffer::getFileOrSTDIN(InputFilename);
   if (InputFilename == "-")
     InputFilename = "<stdin>"; // Overwrite for improved diagnostic messages
   if (std::error_code EC = InputFileOrErr.getError()) {
errs() << "\n"
            << InputFilename
            << "\n"
            << "Check file: " << CheckFilename << "\n"
            << "\n"
            << "-dump-input=help describes the format of the following dump.\n"
            << "\n";
     std::vector<InputAnnotation> Annotations;
     unsigned LabelWidth;
-    BuildInputAnnotations(Diags, Annotations, LabelWidth);
+    BuildInputAnnotations(SM, CheckFileBufferID, ImpPatBufferIDRange, Diags,
+                          Annotations, LabelWidth);
     DumpAnnotatedInput(errs(), Req, InputFileText, Annotations, LabelWidth);
   }
 
   return ExitCode;
 }