https://github.com/arichardson updated https://github.com/llvm/llvm-project/pull/227567
>From a7f40d199db89c6024013dd0edbc4f3f8a6894f0 Mon Sep 17 00:00:00 2001 From: Alex Richardson <[email protected]> Date: Tue, 29 Sep 2026 22:49:12 -0700 Subject: [PATCH 1/3] [Support] Fix offset parsing and buffer caching in SourceMgrDiagnosticVerifier Fix three bugs inherited from the original MLIR implementation: - Parse `@+N` / `@-N` line offsets explicitly in base 10 so leading zeros like `@+010` are not interpreted as octal, and reject negative offsets whose magnitude exceeds the current line number instead of silently underflowing `unsigned`. - Make `computeExpectedDiags` idempotent when called multiple times on the same buffer so existing matched diagnostics are not wiped out. - Rescan all buffers currently registered in `SourceMgr` inside `verify()` so buffers that never emitted a diagnostic still have their `expected-*` directives checked. This commit was created with the help of AI tools --- .../Support/SourceMgrDiagnosticVerifier.cpp | 45 +++++++++++++++---- 1 file changed, 36 insertions(+), 9 deletions(-) diff --git a/llvm/lib/Support/SourceMgrDiagnosticVerifier.cpp b/llvm/lib/Support/SourceMgrDiagnosticVerifier.cpp index 44124c25c7d21..832f54332a955 100644 --- a/llvm/lib/Support/SourceMgrDiagnosticVerifier.cpp +++ b/llvm/lib/Support/SourceMgrDiagnosticVerifier.cpp @@ -88,6 +88,10 @@ SourceMgrDiagnosticVerifier::computeExpectedDiags(raw_ostream &OS, // If the buffer is invalid, return an empty list. if (!Buf) return {}; + // Idempotent: a buffer that's already been scanned is returned as-is, + // rather than being rescanned and appending duplicate entries. + if (auto MaybeDiags = getExpectedDiags(Buf->getBufferIdentifier())) + return *MaybeDiags; auto &ExpectedDiags = ExpectedDiagsPerFile[Buf->getBufferIdentifier()]; // The number of the last line that did not correlate to a designator. @@ -141,12 +145,30 @@ SourceMgrDiagnosticVerifier::computeExpectedDiags(raw_ostream &OS, // Get the integer value without the @ and +/- prefix. if (OffsetMatch[0] == '+' || OffsetMatch[0] == '-') { int Offset; - OffsetMatch.drop_front().getAsInteger(0, Offset); + // The regex only ever captures decimal digits here, so parse as + // decimal explicitly: a leading zero (e.g. '@+08') would otherwise + // be mis-parsed as octal by getAsInteger's radix auto-detection + // (and '08' isn't valid octal, so that already-broken corner case + // would silently discard the offset instead of reporting an error). + if (OffsetMatch.drop_front().getAsInteger(10, Offset)) { + OK = false; + Record.emitError(OS, Mgr, "invalid line offset '" + OffsetMatch + "'"); + continue; + } - if (OffsetMatch.front() == '+') + if (OffsetMatch.front() == '+') { Record.LineNo += Offset; - else + } else if (static_cast<unsigned>(Offset) >= Record.LineNo) { + // Line numbers are 1-based, so an offset that would take LineNo to + // 0 or below is out of range. + OK = false; + Record.emitError(OS, Mgr, + "line offset '" + OffsetMatch + + "' is before the start of the file"); + continue; + } else { Record.LineNo -= Offset; + } } else if (OffsetMatch.consume_front("unknown")) { // This is matching unknown locations. Record.FileLoc = SMLoc(); @@ -189,12 +211,8 @@ SourceMgrDiagnosticVerifier::process(raw_ostream &OS, SourceMgr &Mgr, if (HasLoc) { // If the buffer couldn't be resolved, `Diags` stays empty: a diagnostic // with a location in an unknown file can never match anything. - if (Buf) { - if (auto MaybeDiags = getExpectedDiags(Buf->getBufferIdentifier())) - Diags = *MaybeDiags; - else - Diags = computeExpectedDiags(OS, Mgr, Buf); - } + if (Buf) + Diags = computeExpectedDiags(OS, Mgr, Buf); } else { Diags = ExpectedUnknownLocDiags; } @@ -237,6 +255,15 @@ SourceMgrDiagnosticVerifier::process(raw_ostream &OS, SourceMgr &Mgr, } bool SourceMgrDiagnosticVerifier::verify(raw_ostream &OS, SourceMgr &Mgr) { + // Scan any buffers that process() never had a reason to look at (e.g. an + // included file where the diagnostic its 'expected-*' comment names never + // actually fired): otherwise that expectation would never be recorded, and + // so never reported as missing below, letting verification silently + // pass when it shouldn't. computeExpectedDiags is idempotent, so + // re-scanning an already-known buffer here is harmless. + for (unsigned I = 0, E = Mgr.getNumBuffers(); I != E; ++I) + (void)computeExpectedDiags(OS, Mgr, Mgr.getMemoryBuffer(I + 1)); + // Verify that all expected errors were seen. auto CheckExpectedDiags = [&](ExpectedDiag &Diag) { if (!Diag.Matched) { >From d3732b1dc3abd26d56360c1d06dd1dc52fc92041 Mon Sep 17 00:00:00 2001 From: Alex Richardson <[email protected]> Date: Sun, 18 Oct 2026 12:40:41 -0700 Subject: [PATCH 2/3] [nspr] update --- .../Support/SourceMgrDiagnosticVerifier.h | 141 ++++++------ .../Support/SourceMgrDiagnosticVerifier.cpp | 200 ++++++++++-------- llvm/unittests/Support/CMakeLists.txt | 1 + .../SourceMgrDiagnosticVerifierTest.cpp | 174 +++++++++++++++ .../secondary/llvm/unittests/Support/BUILD.gn | 1 + mlir/lib/IR/Diagnostics.cpp | 15 +- 6 files changed, 361 insertions(+), 171 deletions(-) create mode 100644 llvm/unittests/Support/SourceMgrDiagnosticVerifierTest.cpp diff --git a/llvm/include/llvm/Support/SourceMgrDiagnosticVerifier.h b/llvm/include/llvm/Support/SourceMgrDiagnosticVerifier.h index 675c1a781bdd4..0c1dc380065c1 100644 --- a/llvm/include/llvm/Support/SourceMgrDiagnosticVerifier.h +++ b/llvm/include/llvm/Support/SourceMgrDiagnosticVerifier.h @@ -17,25 +17,77 @@ #define LLVM_SUPPORT_SOURCEMGRDIAGNOSTICVERIFIER_H #include <optional> +#include <string> +#include <vector> #include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/DenseMap.h" #include "llvm/ADT/SmallVector.h" -#include "llvm/ADT/StringMap.h" #include "llvm/Support/Compiler.h" #include "llvm/Support/Regex.h" #include "llvm/Support/SourceMgr.h" +#include "llvm/Support/raw_ostream.h" namespace llvm { class MemoryBuffer; -class raw_ostream; /// Scans SourceMgr source buffers for 'expected-<kind>' comments (e.g. /// 'expected-error {{message}}') and verifies that the diagnostics reported /// to it via \c process match them, and that every expected diagnostic is -/// matched at least once. +/// matched at least once. At most one directive may appear on a single line, +/// and nothing (other than whitespace) may follow the closing '}}' on that +/// line. class LLVM_ABI SourceMgrDiagnosticVerifier { public: + /// The result of matching a single actual diagnostic against the expected + /// diagnostics recorded for its location. + enum class MatchResult { + /// The diagnostic matched an expected diagnostic (kind and text). + Matched, + /// The diagnostic's text matched an expected diagnostic, but its kind + /// didn't; a message about this has already been printed. + NearMiss, + /// The diagnostic did not match any expected diagnostic. The caller is + /// responsible for reporting it, if desired. + Unexpected, + /// The diagnostic did not match, but \p ReportUnexpected was false, so + /// nothing was printed and nothing needs to be done. + Ignored, + }; + + /// \param Mgr The \c SourceMgr whose buffers are scanned for expected + /// diagnostics. + /// \param OS The stream to which verifier diagnostics (near misses, + /// unmatched expected diagnostics, malformed directives) are printed. + explicit SourceMgrDiagnosticVerifier(const SourceMgr &Mgr, + raw_ostream &OS = errs()); + + /// Matches a single actual diagnostic against the expected diagnostics + /// recorded for \p Buf / \p LineNo, scanning \p Buf first if it hasn't been + /// scanned yet. If \p HasLoc is false, the diagnostic has no location and is + /// matched against the '@unknown' list instead (\p Buf / \p LineNo are + /// ignored). If \p HasLoc is true but \p Buf is null (e.g. the diagnostic's + /// file isn't a known buffer), the diagnostic is matched against an empty + /// list, i.e. it can never match and is always unexpected. On a near miss, + /// prints a message to \c OS. \p ReportUnexpected controls whether near + /// misses / unexpected diagnostics are reported at all. + MatchResult process(SourceMgr::DiagKind Kind, bool HasLoc, + const MemoryBuffer *Buf, unsigned LineNo, + StringRef Message, bool ReportUnexpected = true); + + /// Convenience overload for diagnostics already packaged as an + /// \c SMDiagnostic (e.g. from a \c SourceMgr diagnostic handler). Resolves + /// the source buffer and line from \p Diag, and if the diagnostic is + /// unexpected and \p ReportUnexpected is true, prints \p Diag to \c OS. + MatchResult process(const SMDiagnostic &Diag, bool ReportUnexpected = true); + + /// Reports (to \c OS) any expected diagnostic that was never matched by a + /// call to \p process. Returns whether verification succeeded overall, i.e. + /// no diagnostic mismatches were recorded either here or by \p process. + bool verify(); + +private: /// A single diagnostic expected via an 'expected-<kind>' comment. struct ExpectedDiag { ExpectedDiag(SourceMgr::DiagKind Kind, unsigned LineNo, SMLoc FileLoc, @@ -45,16 +97,6 @@ class LLVM_ABI SourceMgrDiagnosticVerifier { /// Returns true if this diagnostic matches the given message. LLVM_ABI bool match(StringRef Str) const; - /// Computes the regex matcher for a '-re' diagnostic's substring. - /// Returns false and prints a message through \p Mgr on error. - LLVM_ABI bool computeRegex(raw_ostream &OS, SourceMgr &Mgr); - - /// Prints \p Msg at this diagnostic's location and returns false, for - /// use as `return emitError(...);` in functions that report failure via - /// a bool return. - LLVM_ABI bool emitError(raw_ostream &OS, SourceMgr &Mgr, - const Twine &Msg) const; - /// The severity of the diagnostic expected. SourceMgr::DiagKind Kind; /// The line number the expected diagnostic should be on. @@ -70,68 +112,33 @@ class LLVM_ABI SourceMgrDiagnosticVerifier { std::optional<Regex> SubstringRegex; }; - /// The result of matching a single actual diagnostic against the expected - /// diagnostics recorded for its location. - enum class MatchResult { - /// The diagnostic matched an expected diagnostic (kind and text). - Matched, - /// The diagnostic's text matched an expected diagnostic, but its kind - /// didn't; a message about this has already been printed. - NearMiss, - /// The diagnostic did not match any expected diagnostic. The caller is - /// responsible for reporting it, if desired. - Unexpected, - /// The diagnostic did not match, but \p ReportUnexpected was false, so - /// nothing was printed and nothing needs to be done. - Ignored, - }; + /// Prints \p Diag to \c OS directly, bypassing \c Mgr.getDiagHandler() so + /// that a verifier installed as \c Mgr's diagnostic handler does not + /// re-enter itself when printing its own diagnostics. + void printDiag(const SMDiagnostic &Diag) const; + + /// Records a verification failure (`OK = false`), prints \p Msg at \p Diag's + /// location to \c OS, and returns false. + bool emitError(const ExpectedDiag &Diag, const Twine &Msg); - SourceMgrDiagnosticVerifier() = default; + /// Computes the regex matcher for a '-re' diagnostic's substring. Returns + /// false and prints a message to \c OS on error. + bool computeRegex(ExpectedDiag &Diag); /// Computes and caches the list of expected diagnostics for \p Buf, if not /// already cached. Returns the (mutable) cached list. - MutableArrayRef<ExpectedDiag> computeExpectedDiags(raw_ostream &OS, - SourceMgr &Mgr, - const MemoryBuffer *Buf); - - /// Returns the cached expected diagnostics for the buffer named \p BufName, - /// or std::nullopt if \p computeExpectedDiags hasn't been called for it. - std::optional<MutableArrayRef<ExpectedDiag>> - getExpectedDiags(StringRef BufName); + MutableArrayRef<ExpectedDiag> computeExpectedDiags(const MemoryBuffer *Buf); - /// Returns the expected diagnostics with an '@unknown' location. - MutableArrayRef<ExpectedDiag> getExpectedUnknownLocDiags() { - return ExpectedUnknownLocDiags; - } + const SourceMgr &Mgr; + raw_ostream &OS; - /// Matches a single actual diagnostic against the expected diagnostics - /// recorded for \p Buf / \p LineNo, computing them first via \p - /// computeExpectedDiags if they haven't been already. If \p HasLoc is - /// false, the diagnostic has no location and is matched against the - /// '@unknown' list instead (\p Buf / \p LineNo are ignored). If \p HasLoc - /// is true but \p Buf is null (e.g. the diagnostic's file isn't a known - /// buffer), the diagnostic is matched against an empty list, i.e. it can - /// never match and is always unexpected. On a near miss, prints a message - /// through \p Mgr. \p ReportUnexpected controls whether near misses / - /// unexpected diagnostics are reported at all. - MatchResult process(raw_ostream &OS, SourceMgr &Mgr, SourceMgr::DiagKind Kind, - bool HasLoc, const MemoryBuffer *Buf, unsigned LineNo, - StringRef Message, bool ReportUnexpected = true); - - /// Reports (through \p Mgr) any expected diagnostic that was never matched - /// by a call to \p process. Returns whether verification succeeded overall, - /// i.e. no diagnostic mismatches were recorded either here or by \p - /// process. - bool verify(raw_ostream &OS, SourceMgr &Mgr); - -private: /// Regex used to recognize 'expected-<kind>' comments. - Regex Expected{"expected-(error|note|remark|warning)(-re)? " - "*(@([+-][0-9]+|above|below|unknown))? *{{(.*)}}$"}; + Regex Expected{"expected-(error|note|remark|warning)(-re)?" + " *(@([+-][0-9]+|above|below|unknown))? *{{(.*)}}$"}; - /// The expected diagnostics for each buffer that has been scanned so far, - /// keyed by buffer identifier (i.e. file name). - StringMap<SmallVector<ExpectedDiag, 2>> ExpectedDiagsPerFile; + /// The expected diagnostics for each buffer that has been scanned so far. + DenseMap<const MemoryBuffer *, SmallVector<ExpectedDiag, 2>> + ExpectedDiagsPerBuffer; /// The expected diagnostics with an '@unknown' location. SmallVector<ExpectedDiag, 2> ExpectedUnknownLocDiags; diff --git a/llvm/lib/Support/SourceMgrDiagnosticVerifier.cpp b/llvm/lib/Support/SourceMgrDiagnosticVerifier.cpp index 832f54332a955..71f4b0e1c1c1b 100644 --- a/llvm/lib/Support/SourceMgrDiagnosticVerifier.cpp +++ b/llvm/lib/Support/SourceMgrDiagnosticVerifier.cpp @@ -8,11 +8,16 @@ #include "llvm/Support/SourceMgrDiagnosticVerifier.h" +#include "llvm/ADT/StringSwitch.h" #include "llvm/Support/MemoryBuffer.h" #include "llvm/Support/raw_ostream.h" using namespace llvm; +SourceMgrDiagnosticVerifier::SourceMgrDiagnosticVerifier(const SourceMgr &Mgr, + raw_ostream &OS) + : Mgr(Mgr), OS(OS) {} + /// Given a diagnostic kind, return a human readable string for it. static StringRef getDiagKindStr(SourceMgr::DiagKind Kind) { switch (Kind) { @@ -28,15 +33,22 @@ static StringRef getDiagKindStr(SourceMgr::DiagKind Kind) { llvm_unreachable("Unknown SourceMgr::DiagKind"); } -bool SourceMgrDiagnosticVerifier::ExpectedDiag::emitError( - raw_ostream &OS, SourceMgr &Mgr, const Twine &Msg) const { - if (FileLoc.isValid()) { - SMRange Range(FileLoc, SMLoc::getFromPointer(FileLoc.getPointer() + - Substring.size())); - Mgr.PrintMessage(OS, FileLoc, SourceMgr::DK_Error, Msg, Range); - } else { - Mgr.PrintMessage(OS, FileLoc, SourceMgr::DK_Error, Msg); +void SourceMgrDiagnosticVerifier::printDiag(const SMDiagnostic &Diag) const { + Mgr.printIncludeStackForDiagnostic(Diag.getLoc(), OS); + Diag.print(nullptr, OS); +} + +bool SourceMgrDiagnosticVerifier::emitError(const ExpectedDiag &Diag, + const Twine &Msg) { + OK = false; + SMRange Range; + ArrayRef<SMRange> Ranges; + if (Diag.FileLoc.isValid()) { + Range = + SMRange(Diag.FileLoc, SMLoc::getFromPointer(Diag.Substring.end() + 2)); + Ranges = Range; } + printDiag(Mgr.GetMessage(Diag.FileLoc, SourceMgr::DK_Error, Msg, Ranges)); return false; } @@ -48,11 +60,10 @@ bool SourceMgrDiagnosticVerifier::ExpectedDiag::match(StringRef Str) const { return Str.contains(Substring); } -bool SourceMgrDiagnosticVerifier::ExpectedDiag::computeRegex(raw_ostream &OS, - SourceMgr &Mgr) { +bool SourceMgrDiagnosticVerifier::computeRegex(ExpectedDiag &Diag) { std::string RegexStr; raw_string_ostream RegexOS(RegexStr); - StringRef StrToProcess = Substring; + StringRef StrToProcess = Diag.Substring; while (!StrToProcess.empty()) { // Find the next regex block. size_t RegexIt = StrToProcess.find("{{"); @@ -66,33 +77,30 @@ bool SourceMgrDiagnosticVerifier::ExpectedDiag::computeRegex(raw_ostream &OS, // Find the end of the regex block. size_t RegexEndIt = StrToProcess.find("}}"); if (RegexEndIt == StringRef::npos) - return emitError(OS, Mgr, "found start of regex with no end '}}'"); + return emitError(Diag, "found start of regex with no end '}}'"); StringRef RegexBlock = StrToProcess.take_front(RegexEndIt); // Validate that the regex is actually valid. std::string RegexError; if (!Regex(RegexBlock).isValid(RegexError)) - return emitError(OS, Mgr, "invalid regex: " + RegexError); + return emitError(Diag, "invalid regex: " + RegexError); RegexOS << '(' << RegexBlock << ')'; StrToProcess = StrToProcess.drop_front(RegexEndIt + 2); } - SubstringRegex = Regex(RegexStr); + Diag.SubstringRegex = Regex(RegexStr); return true; } MutableArrayRef<SourceMgrDiagnosticVerifier::ExpectedDiag> -SourceMgrDiagnosticVerifier::computeExpectedDiags(raw_ostream &OS, - SourceMgr &Mgr, - const MemoryBuffer *Buf) { +SourceMgrDiagnosticVerifier::computeExpectedDiags(const MemoryBuffer *Buf) { // If the buffer is invalid, return an empty list. if (!Buf) return {}; - // Idempotent: a buffer that's already been scanned is returned as-is, - // rather than being rescanned and appending duplicate entries. - if (auto MaybeDiags = getExpectedDiags(Buf->getBufferIdentifier())) - return *MaybeDiags; - auto &ExpectedDiags = ExpectedDiagsPerFile[Buf->getBufferIdentifier()]; + auto [It, Inserted] = ExpectedDiagsPerBuffer.try_emplace(Buf); + if (!Inserted) + return It->second; + auto &ExpectedDiags = It->second; // The number of the last line that did not correlate to a designator. unsigned LastNonDesignatorLine = 0; @@ -104,7 +112,7 @@ SourceMgrDiagnosticVerifier::computeExpectedDiags(raw_ostream &OS, SmallVector<StringRef, 100> Lines; Buf->getBuffer().split(Lines, '\n'); for (unsigned LineNo = 0, E = Lines.size(); LineNo < E; ++LineNo) { - SmallVector<StringRef, 4> Matches; + SmallVector<StringRef, 10> Matches; if (!Expected.match(Lines[LineNo].rtrim(), &Matches)) { // Check for designators that apply to this line. if (!DesignatorsForNextLine.empty()) { @@ -119,24 +127,16 @@ SourceMgrDiagnosticVerifier::computeExpectedDiags(raw_ostream &OS, // Point to the start of expected-*. SMLoc ExpectedStart = SMLoc::getFromPointer(Matches[0].data()); - SourceMgr::DiagKind Kind; - if (Matches[1] == "error") - Kind = SourceMgr::DK_Error; - else if (Matches[1] == "warning") - Kind = SourceMgr::DK_Warning; - else if (Matches[1] == "remark") - Kind = SourceMgr::DK_Remark; - else { - assert(Matches[1] == "note"); - Kind = SourceMgr::DK_Note; - } + SourceMgr::DiagKind Kind = StringSwitch<SourceMgr::DiagKind>(Matches[1]) + .Case("error", SourceMgr::DK_Error) + .Case("warning", SourceMgr::DK_Warning) + .Case("remark", SourceMgr::DK_Remark) + .Case("note", SourceMgr::DK_Note); ExpectedDiag Record(Kind, LineNo + 1, ExpectedStart, Matches[5]); // Check to see if this is a regex match, i.e. it includes the `-re`. - if (!Matches[2].empty() && !Record.computeRegex(OS, Mgr)) { - OK = false; + if (!Matches[2].empty() && !computeRegex(Record)) continue; - } StringRef OffsetMatch = Matches[3]; if (!OffsetMatch.empty()) { @@ -144,27 +144,26 @@ SourceMgrDiagnosticVerifier::computeExpectedDiags(raw_ostream &OS, // Get the integer value without the @ and +/- prefix. if (OffsetMatch[0] == '+' || OffsetMatch[0] == '-') { - int Offset; - // The regex only ever captures decimal digits here, so parse as - // decimal explicitly: a leading zero (e.g. '@+08') would otherwise - // be mis-parsed as octal by getAsInteger's radix auto-detection - // (and '08' isn't valid octal, so that already-broken corner case - // would silently discard the offset instead of reporting an error). + unsigned Offset; + // Parse as decimal explicitly so leading zeros (e.g. '@+08') are not + // treated as octal by getAsInteger's radix auto-detection. if (OffsetMatch.drop_front().getAsInteger(10, Offset)) { - OK = false; - Record.emitError(OS, Mgr, "invalid line offset '" + OffsetMatch + "'"); + emitError(Record, "invalid line offset '" + OffsetMatch + "'"); continue; } if (OffsetMatch.front() == '+') { + if (Offset > E - Record.LineNo) { + emitError(Record, "line offset '" + OffsetMatch + + "' is past the end of the file"); + continue; + } Record.LineNo += Offset; - } else if (static_cast<unsigned>(Offset) >= Record.LineNo) { - // Line numbers are 1-based, so an offset that would take LineNo to - // 0 or below is out of range. - OK = false; - Record.emitError(OS, Mgr, - "line offset '" + OffsetMatch + - "' is before the start of the file"); + } else if (Offset > Record.LineNo) { + // Allow LineNo == 0 (Offset == Record.LineNo), which MLIR uses for + // diagnostics on the implicit top-level module. + emitError(Record, "line offset '" + OffsetMatch + + "' is before the start of the file"); continue; } else { Record.LineNo -= Offset; @@ -194,17 +193,8 @@ SourceMgrDiagnosticVerifier::computeExpectedDiags(raw_ostream &OS, return ExpectedDiags; } -std::optional<MutableArrayRef<SourceMgrDiagnosticVerifier::ExpectedDiag>> -SourceMgrDiagnosticVerifier::getExpectedDiags(StringRef BufName) { - auto ExpectedDiags = ExpectedDiagsPerFile.find(BufName); - if (ExpectedDiags != ExpectedDiagsPerFile.end()) - return MutableArrayRef<ExpectedDiag>(ExpectedDiags->second); - return std::nullopt; -} - SourceMgrDiagnosticVerifier::MatchResult -SourceMgrDiagnosticVerifier::process(raw_ostream &OS, SourceMgr &Mgr, - SourceMgr::DiagKind Kind, bool HasLoc, +SourceMgrDiagnosticVerifier::process(SourceMgr::DiagKind Kind, bool HasLoc, const MemoryBuffer *Buf, unsigned LineNo, StringRef Message, bool ReportUnexpected) { MutableArrayRef<ExpectedDiag> Diags; @@ -212,31 +202,40 @@ SourceMgrDiagnosticVerifier::process(raw_ostream &OS, SourceMgr &Mgr, // If the buffer couldn't be resolved, `Diags` stays empty: a diagnostic // with a location in an unknown file can never match anything. if (Buf) - Diags = computeExpectedDiags(OS, Mgr, Buf); + Diags = computeExpectedDiags(Buf); } else { + for (unsigned I = 0, E = Mgr.getNumBuffers(); I != E; ++I) + (void)computeExpectedDiags(Mgr.getMemoryBuffer(I + 1)); Diags = ExpectedUnknownLocDiags; } - // Search for a matching expected diagnostic. - // If we find something that is close then emit a more specific error. + // Search for a matching expected diagnostic. Prefer an unmatched entry so + // that multiple expected-* directives on the same line with identical or + // overlapping substrings each get a chance to match before an already-matched + // entry is reused. + ExpectedDiag *AlreadyMatched = nullptr; ExpectedDiag *NearMiss = nullptr; - // If this was an expected error, remember that we saw it and return. for (auto &E : Diags) { // File line must match (unless it's an unknown location). if (HasLoc && E.LineNo != LineNo) continue; - if (E.match(Message)) { - if (E.Kind == Kind) { + if (!E.match(Message)) + continue; + if (E.Kind == Kind) { + if (!E.Matched) { E.Matched = true; return MatchResult::Matched; } - + AlreadyMatched = &E; + } else if (!E.Matched) { // If this only differs based on the diagnostic kind, then consider it // to be a near miss. NearMiss = &E; } } + if (AlreadyMatched) + return MatchResult::Matched; if (!ReportUnexpected) return MatchResult::Ignored; @@ -245,39 +244,52 @@ SourceMgrDiagnosticVerifier::process(raw_ostream &OS, SourceMgr &Mgr, // Otherwise, emit an error for the near miss. if (NearMiss) { - Mgr.PrintMessage(OS, NearMiss->FileLoc, SourceMgr::DK_Error, - "'" + getDiagKindStr(Kind) + - "' diagnostic emitted when expecting a '" + - getDiagKindStr(NearMiss->Kind) + "'"); + printDiag(Mgr.GetMessage(NearMiss->FileLoc, SourceMgr::DK_Error, + "'" + getDiagKindStr(Kind) + + "' diagnostic emitted when expecting a '" + + getDiagKindStr(NearMiss->Kind) + "'")); return MatchResult::NearMiss; } return MatchResult::Unexpected; } -bool SourceMgrDiagnosticVerifier::verify(raw_ostream &OS, SourceMgr &Mgr) { - // Scan any buffers that process() never had a reason to look at (e.g. an - // included file where the diagnostic its 'expected-*' comment names never - // actually fired): otherwise that expectation would never be recorded, and - // so never reported as missing below, letting verification silently - // pass when it shouldn't. computeExpectedDiags is idempotent, so - // re-scanning an already-known buffer here is harmless. - for (unsigned I = 0, E = Mgr.getNumBuffers(); I != E; ++I) - (void)computeExpectedDiags(OS, Mgr, Mgr.getMemoryBuffer(I + 1)); +SourceMgrDiagnosticVerifier::MatchResult +SourceMgrDiagnosticVerifier::process(const SMDiagnostic &Diag, + bool ReportUnexpected) { + bool HasLoc = Diag.getLoc().isValid(); + const MemoryBuffer *Buf = nullptr; + if (HasLoc) { + if (unsigned ID = Mgr.FindBufferContainingLoc(Diag.getLoc())) + Buf = Mgr.getMemoryBuffer(ID); + } + MatchResult Result = + process(Diag.getKind(), HasLoc, Buf, + HasLoc ? static_cast<unsigned>(Diag.getLineNo()) : 0, + Diag.getMessage(), ReportUnexpected); + if (Result == MatchResult::Unexpected) + printDiag(Diag); + return Result; +} - // Verify that all expected errors were seen. - auto CheckExpectedDiags = [&](ExpectedDiag &Diag) { - if (!Diag.Matched) { - Diag.emitError(OS, Mgr, - "expected " + getDiagKindStr(Diag.Kind) + " \"" + - Diag.Substring + "\" was not produced"); - OK = false; - } +bool SourceMgrDiagnosticVerifier::verify() { + // Check all buffers in SourceMgr registration order so diagnostics are + // reported deterministically and buffers that emitted no diagnostics (such as + // '.include'd files) are still scanned. Clear each buffer's vector rather + // than erasing map entries so a subsequent verify() call does not rescan + // buffers with Matched = false. + auto CheckExpectedDiags = [&](const ExpectedDiag &Diag) { + if (!Diag.Matched) + emitError(Diag, "expected " + getDiagKindStr(Diag.Kind) + " \"" + + Diag.Substring + "\" was not produced"); }; - for (auto &ExpectedDiagsPair : ExpectedDiagsPerFile) - for (auto &Diag : ExpectedDiagsPair.second) + for (unsigned I = 0, E = Mgr.getNumBuffers(); I != E; ++I) { + const MemoryBuffer *Buf = Mgr.getMemoryBuffer(I + 1); + for (const ExpectedDiag &Diag : computeExpectedDiags(Buf)) CheckExpectedDiags(Diag); - for (auto &Diag : ExpectedUnknownLocDiags) + ExpectedDiagsPerBuffer[Buf].clear(); + } + for (const ExpectedDiag &Diag : ExpectedUnknownLocDiags) CheckExpectedDiags(Diag); - ExpectedDiagsPerFile.clear(); + ExpectedUnknownLocDiags.clear(); return OK; } diff --git a/llvm/unittests/Support/CMakeLists.txt b/llvm/unittests/Support/CMakeLists.txt index f9732ff4e0e4b..c5db78950d21b 100644 --- a/llvm/unittests/Support/CMakeLists.txt +++ b/llvm/unittests/Support/CMakeLists.txt @@ -90,6 +90,7 @@ add_llvm_unittest(SupportTests SignalsTest.cpp SipHashTest.cpp SourceMgrTest.cpp + SourceMgrDiagnosticVerifierTest.cpp SpecialCaseListTest.cpp SuffixTreeTest.cpp SwapByteOrderTest.cpp diff --git a/llvm/unittests/Support/SourceMgrDiagnosticVerifierTest.cpp b/llvm/unittests/Support/SourceMgrDiagnosticVerifierTest.cpp new file mode 100644 index 0000000000000..1e7f65e2c0b79 --- /dev/null +++ b/llvm/unittests/Support/SourceMgrDiagnosticVerifierTest.cpp @@ -0,0 +1,174 @@ +//===- unittests/Support/SourceMgrDiagnosticVerifierTest.cpp --------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "llvm/Support/SourceMgrDiagnosticVerifier.h" +#include "llvm/Support/MemoryBuffer.h" +#include "llvm/Support/SourceMgr.h" +#include "llvm/Support/raw_ostream.h" +#include "gtest/gtest.h" + +using namespace llvm; +using MatchResult = SourceMgrDiagnosticVerifier::MatchResult; + +namespace { + +class SourceMgrDiagnosticVerifierTest : public testing::Test { +protected: + SourceMgr SM; + std::string Output; + raw_string_ostream OS{Output}; + + const MemoryBuffer *addBuffer(StringRef Text, + StringRef BufferName = "test.s") { + unsigned ID = SM.AddNewSourceBuffer( + MemoryBuffer::getMemBuffer(Text, BufferName), SMLoc()); + return SM.getMemoryBuffer(ID); + } +}; + +} // namespace + +TEST_F(SourceMgrDiagnosticVerifierTest, MatchAndVerifyIdempotent) { + const MemoryBuffer *Buf = + addBuffer("bad_op\n# expected-error@-1 {{invalid operand}}\n"); + SourceMgrDiagnosticVerifier Verifier(SM, OS); + + SMLoc Loc = SMLoc::getFromPointer(Buf->getBufferStart()); + SMDiagnostic Diag = + SM.GetMessage(Loc, SourceMgr::DK_Error, "invalid operand"); + EXPECT_EQ(Verifier.process(Diag), MatchResult::Matched); + EXPECT_TRUE(Verifier.verify()); + EXPECT_EQ(Output, ""); + + // Calling verify() a second time (e.g. from a destructor after an explicit + // verify() call) must not re-parse the buffer with Matched = false. + EXPECT_TRUE(Verifier.verify()); + EXPECT_EQ(Output, ""); +} + +TEST_F(SourceMgrDiagnosticVerifierTest, UnknownLocation) { + addBuffer("# expected-warning@unknown {{no location warning}}\n"); + SourceMgrDiagnosticVerifier Verifier(SM, OS); + + SMDiagnostic Diag = + SM.GetMessage(SMLoc(), SourceMgr::DK_Warning, "no location warning"); + EXPECT_EQ(Verifier.process(Diag), MatchResult::Matched); + EXPECT_TRUE(Verifier.verify()); + EXPECT_EQ(Output, ""); +} + +TEST_F(SourceMgrDiagnosticVerifierTest, UnmatchedUnknownLocation) { + addBuffer("# expected-error@unknown {{missing unknown error}}\n"); + SourceMgrDiagnosticVerifier Verifier(SM, OS); + + EXPECT_FALSE(Verifier.verify()); + EXPECT_NE( + StringRef(Output).find( + "error: expected error \"missing unknown error\" was not produced"), + StringRef::npos); +} + +TEST_F(SourceMgrDiagnosticVerifierTest, ReportUnexpectedFalse) { + const MemoryBuffer *Buf = + addBuffer("op\n# expected-error@-1 {{expected msg}}\n"); + SourceMgrDiagnosticVerifier Verifier(SM, OS); + + // An unexpected diagnostic with ReportUnexpected = false is ignored and does + // not fail verification. + SMLoc Loc = SMLoc::getFromPointer(Buf->getBufferStart()); + SMDiagnostic UnexpectedDiag = + SM.GetMessage(Loc, SourceMgr::DK_Warning, "unrelated warning"); + EXPECT_EQ(Verifier.process(UnexpectedDiag, /*ReportUnexpected=*/false), + MatchResult::Ignored); + + SMDiagnostic ExpectedDiag = + SM.GetMessage(Loc, SourceMgr::DK_Error, "expected msg"); + EXPECT_EQ(Verifier.process(ExpectedDiag, /*ReportUnexpected=*/false), + MatchResult::Matched); + EXPECT_TRUE(Verifier.verify()); + EXPECT_EQ(Output, ""); +} + +TEST_F(SourceMgrDiagnosticVerifierTest, NullBufferWithHasLocIsUnexpected) { + addBuffer("# expected-error {{anything}}\n"); + SourceMgrDiagnosticVerifier Verifier(SM, OS); + + EXPECT_EQ(Verifier.process(SourceMgr::DK_Error, /*HasLoc=*/true, + /*Buf=*/nullptr, 1, "anything"), + MatchResult::Unexpected); + EXPECT_FALSE(Verifier.verify()); +} + +TEST_F(SourceMgrDiagnosticVerifierTest, + MultipleExpectedDiagsOnSameLinePreferUnmatched) { + const MemoryBuffer *Buf = + addBuffer("op\n" + "# expected-error@-1 {{invalid operand}}\n" + "# expected-error@-2 {{invalid operand}}\n"); + SourceMgrDiagnosticVerifier Verifier(SM, OS); + + SMLoc Loc = SMLoc::getFromPointer(Buf->getBufferStart()); + SMDiagnostic Diag = + SM.GetMessage(Loc, SourceMgr::DK_Error, "invalid operand"); + EXPECT_EQ(Verifier.process(Diag), MatchResult::Matched); + EXPECT_EQ(Verifier.process(Diag), MatchResult::Matched); + EXPECT_TRUE(Verifier.verify()); + EXPECT_EQ(Output, ""); +} + +TEST_F(SourceMgrDiagnosticVerifierTest, LineZeroAndLeadingZeroOffsets) { + const MemoryBuffer *Buf = + addBuffer("// expected-error@+02 {{decimal leading zero}}\n" + "// expected-remark@-2 {{implicit module at line 0}}\n" + "op\n"); + SourceMgrDiagnosticVerifier Verifier(SM, OS); + + EXPECT_EQ(Verifier.process(SourceMgr::DK_Remark, /*HasLoc=*/true, Buf, + /*LineNo=*/0, "implicit module at line 0"), + MatchResult::Matched); + EXPECT_EQ(Verifier.process(SourceMgr::DK_Error, /*HasLoc=*/true, Buf, + /*LineNo=*/3, "decimal leading zero"), + MatchResult::Matched); + EXPECT_TRUE(Verifier.verify()); + EXPECT_EQ(Output, ""); +} + +TEST_F(SourceMgrDiagnosticVerifierTest, OutOfBoundsLineOffsets) { + addBuffer("op\n" + "# expected-error@-3 {{before start}}\n" + "# expected-error@+5 {{past end}}\n"); + SourceMgrDiagnosticVerifier Verifier(SM, OS); + + EXPECT_FALSE(Verifier.verify()); + EXPECT_NE(StringRef(Output).find( + "error: line offset '-3' is before the start of the file"), + StringRef::npos); + EXPECT_NE(StringRef(Output).find( + "error: line offset '+5' is past the end of the file"), + StringRef::npos); +} + +TEST_F(SourceMgrDiagnosticVerifierTest, AlreadyMatchedDoesNotTriggerNearMiss) { + const MemoryBuffer *Buf = addBuffer("op\n# expected-error@-1 {{operand}}\n"); + SourceMgrDiagnosticVerifier Verifier(SM, OS); + + SMLoc Loc = SMLoc::getFromPointer(Buf->getBufferStart()); + EXPECT_EQ( + Verifier.process(SM.GetMessage(Loc, SourceMgr::DK_Error, "bad operand")), + MatchResult::Matched); + // A subsequent unexpected note containing the same substring should be + // reported as Unexpected (not as a NearMiss against the already-satisfied + // expected-error). + EXPECT_EQ( + Verifier.process(SM.GetMessage(Loc, SourceMgr::DK_Note, "note operand")), + MatchResult::Unexpected); + EXPECT_FALSE(Verifier.verify()); + EXPECT_NE(StringRef(Output).find("note: note operand"), StringRef::npos); + EXPECT_EQ(StringRef(Output).find("diagnostic emitted when expecting"), + StringRef::npos); +} diff --git a/llvm/utils/gn/secondary/llvm/unittests/Support/BUILD.gn b/llvm/utils/gn/secondary/llvm/unittests/Support/BUILD.gn index 168b609279cab..7569f7e6fcbb9 100644 --- a/llvm/utils/gn/secondary/llvm/unittests/Support/BUILD.gn +++ b/llvm/utils/gn/secondary/llvm/unittests/Support/BUILD.gn @@ -93,6 +93,7 @@ unittest("SupportTests") { "SignalsTest.cpp", "SipHashTest.cpp", "SourceMgrTest.cpp", + "SourceMgrDiagnosticVerifierTest.cpp", "SpecialCaseListTest.cpp", "SuffixTreeTest.cpp", "SwapByteOrderTest.cpp", diff --git a/mlir/lib/IR/Diagnostics.cpp b/mlir/lib/IR/Diagnostics.cpp index ae678e1c44dfe..5f1a86e1ed495 100644 --- a/mlir/lib/IR/Diagnostics.cpp +++ b/mlir/lib/IR/Diagnostics.cpp @@ -640,8 +640,9 @@ namespace mlir { namespace detail { struct SourceMgrDiagnosticVerifierHandlerImpl { SourceMgrDiagnosticVerifierHandlerImpl( + llvm::SourceMgr &srcMgr, raw_ostream &out, SourceMgrDiagnosticVerifierHandler::Level level) - : level(level) {} + : verifier(srcMgr, out), level(level) {} /// The generic SourceMgr-based verifier core that this handler delegates /// scanning and matching of 'expected-*' diagnostics to. @@ -671,13 +672,7 @@ static StringRef getDiagKindStr(DiagnosticSeverity kind) { SourceMgrDiagnosticVerifierHandler::SourceMgrDiagnosticVerifierHandler( llvm::SourceMgr &srcMgr, MLIRContext *ctx, raw_ostream &out, Level level) : SourceMgrDiagnosticHandler(srcMgr, ctx, out), - impl(new SourceMgrDiagnosticVerifierHandlerImpl(level)) { - // Compute the expected diagnostics for each of the current files in the - // source manager. - for (unsigned i = 0, e = mgr.getNumBuffers(); i != e; ++i) - (void)impl->verifier.computeExpectedDiags(out, mgr, - mgr.getMemoryBuffer(i + 1)); - + impl(new SourceMgrDiagnosticVerifierHandlerImpl(srcMgr, out, level)) { // The base class registered a handler that prints every diagnostic. The // verifier takes its place: it consumes diagnostics and reports unexpected // ones itself. @@ -697,7 +692,7 @@ SourceMgrDiagnosticVerifierHandler::~SourceMgrDiagnosticVerifierHandler() { /// diagnostics were emitted. This return success if all diagnostics were /// verified correctly, failure otherwise. LogicalResult SourceMgrDiagnosticVerifierHandler::verify() { - return success(impl->verifier.verify(os, mgr)); + return success(impl->verifier.verify()); } std::unique_ptr<ScopedDiagnosticHandler> @@ -725,7 +720,7 @@ void SourceMgrDiagnosticVerifierHandler::process(LocationAttr loc, using MatchResult = llvm::SourceMgrDiagnosticVerifier::MatchResult; MatchResult result = impl->verifier.process( - os, mgr, getDiagKind(kind), /*hasLoc=*/static_cast<bool>(fileLoc), buf, + getDiagKind(kind), /*hasLoc=*/static_cast<bool>(fileLoc), buf, fileLoc ? fileLoc.getLine() : 0, msg, reportUnexpected); if (result == MatchResult::Unexpected) emitDiagnostic(loc, "unexpected " + getDiagKindStr(kind) + ": " + msg, >From 796ce0dd773390d789dc74d67f147984e2e7079f Mon Sep 17 00:00:00 2001 From: Alex Richardson <[email protected]> Date: Sun, 18 Oct 2026 15:09:07 -0700 Subject: [PATCH 3/3] [nspr] update --- .../Support/SourceMgrDiagnosticVerifier.h | 89 ++++++------- .../Support/SourceMgrDiagnosticVerifier.cpp | 123 +++++++----------- .../SourceMgrDiagnosticVerifierTest.cpp | 121 +++++------------ mlir/lib/IR/Diagnostics.cpp | 16 +-- 4 files changed, 135 insertions(+), 214 deletions(-) diff --git a/llvm/include/llvm/Support/SourceMgrDiagnosticVerifier.h b/llvm/include/llvm/Support/SourceMgrDiagnosticVerifier.h index 0c1dc380065c1..16d87d9e335d4 100644 --- a/llvm/include/llvm/Support/SourceMgrDiagnosticVerifier.h +++ b/llvm/include/llvm/Support/SourceMgrDiagnosticVerifier.h @@ -16,28 +16,23 @@ #ifndef LLVM_SUPPORT_SOURCEMGRDIAGNOSTICVERIFIER_H #define LLVM_SUPPORT_SOURCEMGRDIAGNOSTICVERIFIER_H -#include <optional> -#include <string> -#include <vector> - #include "llvm/ADT/ArrayRef.h" -#include "llvm/ADT/DenseMap.h" #include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/StringMap.h" #include "llvm/Support/Compiler.h" #include "llvm/Support/Regex.h" #include "llvm/Support/SourceMgr.h" -#include "llvm/Support/raw_ostream.h" +#include <optional> namespace llvm { class MemoryBuffer; +class raw_ostream; /// Scans SourceMgr source buffers for 'expected-<kind>' comments (e.g. /// 'expected-error {{message}}') and verifies that the diagnostics reported /// to it via \c process match them, and that every expected diagnostic is -/// matched at least once. At most one directive may appear on a single line, -/// and nothing (other than whitespace) may follow the closing '}}' on that -/// line. +/// matched at least once. class LLVM_ABI SourceMgrDiagnosticVerifier { public: /// The result of matching a single actual diagnostic against the expected @@ -48,43 +43,44 @@ class LLVM_ABI SourceMgrDiagnosticVerifier { /// The diagnostic's text matched an expected diagnostic, but its kind /// didn't; a message about this has already been printed. NearMiss, - /// The diagnostic did not match any expected diagnostic. The caller is - /// responsible for reporting it, if desired. + /// The diagnostic did not match any expected diagnostic. Unexpected, /// The diagnostic did not match, but \p ReportUnexpected was false, so /// nothing was printed and nothing needs to be done. Ignored, }; - /// \param Mgr The \c SourceMgr whose buffers are scanned for expected - /// diagnostics. - /// \param OS The stream to which verifier diagnostics (near misses, - /// unmatched expected diagnostics, malformed directives) are printed. - explicit SourceMgrDiagnosticVerifier(const SourceMgr &Mgr, - raw_ostream &OS = errs()); - - /// Matches a single actual diagnostic against the expected diagnostics - /// recorded for \p Buf / \p LineNo, scanning \p Buf first if it hasn't been - /// scanned yet. If \p HasLoc is false, the diagnostic has no location and is - /// matched against the '@unknown' list instead (\p Buf / \p LineNo are - /// ignored). If \p HasLoc is true but \p Buf is null (e.g. the diagnostic's - /// file isn't a known buffer), the diagnostic is matched against an empty - /// list, i.e. it can never match and is always unexpected. On a near miss, - /// prints a message to \c OS. \p ReportUnexpected controls whether near - /// misses / unexpected diagnostics are reported at all. - MatchResult process(SourceMgr::DiagKind Kind, bool HasLoc, - const MemoryBuffer *Buf, unsigned LineNo, + /// The location of an actual diagnostic. + struct Location { + /// The buffer containing the diagnostic, or null if it is in a file that + /// is not known to the SourceMgr (such a diagnostic never matches). + const MemoryBuffer *Buf; + /// The 1-based line number of the diagnostic. + unsigned LineNo; + }; + + /// Scans all buffers currently registered with \p Mgr for expected + /// diagnostics. Buffers added later are scanned the first time a + /// diagnostic is reported in them. Verifier errors (near misses, unmatched + /// expected diagnostics, malformed directives) are printed to \p OS, + /// bypassing \p Mgr's diagnostic handler. + SourceMgrDiagnosticVerifier(const SourceMgr &Mgr, raw_ostream &OS); + + /// Matches a diagnostic against the expected diagnostics for \p Loc, or + /// against the '@unknown' expected diagnostics if \p Loc is std::nullopt. + /// Prints a message on a near miss. Reporting an unexpected diagnostic is + /// left to the caller. If \p ReportUnexpected is false, a diagnostic that + /// does not match is ignored instead of failing verification. + MatchResult process(SourceMgr::DiagKind Kind, std::optional<Location> Loc, StringRef Message, bool ReportUnexpected = true); - /// Convenience overload for diagnostics already packaged as an - /// \c SMDiagnostic (e.g. from a \c SourceMgr diagnostic handler). Resolves - /// the source buffer and line from \p Diag, and if the diagnostic is - /// unexpected and \p ReportUnexpected is true, prints \p Diag to \c OS. + /// Like the overload above, but takes the location from \p Diag and also + /// prints \p Diag if it is unexpected. MatchResult process(const SMDiagnostic &Diag, bool ReportUnexpected = true); - /// Reports (to \c OS) any expected diagnostic that was never matched by a - /// call to \p process. Returns whether verification succeeded overall, i.e. - /// no diagnostic mismatches were recorded either here or by \p process. + /// Reports any expected diagnostic that was never matched by a call to + /// \c process. Returns whether verification succeeded overall, i.e. no + /// diagnostic mismatches were recorded either here or by \c process. bool verify(); private: @@ -112,21 +108,20 @@ class LLVM_ABI SourceMgrDiagnosticVerifier { std::optional<Regex> SubstringRegex; }; - /// Prints \p Diag to \c OS directly, bypassing \c Mgr.getDiagHandler() so - /// that a verifier installed as \c Mgr's diagnostic handler does not - /// re-enter itself when printing its own diagnostics. + /// Prints \p Diag to \c OS, bypassing the SourceMgr's diagnostic handler so + /// that a verifier installed as that handler does not re-enter itself. void printDiag(const SMDiagnostic &Diag) const; - /// Records a verification failure (`OK = false`), prints \p Msg at \p Diag's - /// location to \c OS, and returns false. + /// Records a verification failure, prints \p Msg at \p Diag's location and + /// returns false. bool emitError(const ExpectedDiag &Diag, const Twine &Msg); /// Computes the regex matcher for a '-re' diagnostic's substring. Returns - /// false and prints a message to \c OS on error. + /// false and prints a message on error. bool computeRegex(ExpectedDiag &Diag); - /// Computes and caches the list of expected diagnostics for \p Buf, if not - /// already cached. Returns the (mutable) cached list. + /// Scans \p Buf for expected diagnostics and returns the list for its + /// buffer identifier. MutableArrayRef<ExpectedDiag> computeExpectedDiags(const MemoryBuffer *Buf); const SourceMgr &Mgr; @@ -136,9 +131,9 @@ class LLVM_ABI SourceMgrDiagnosticVerifier { Regex Expected{"expected-(error|note|remark|warning)(-re)?" " *(@([+-][0-9]+|above|below|unknown))? *{{(.*)}}$"}; - /// The expected diagnostics for each buffer that has been scanned so far. - DenseMap<const MemoryBuffer *, SmallVector<ExpectedDiag, 2>> - ExpectedDiagsPerBuffer; + /// The expected diagnostics for each buffer that has been scanned so far, + /// keyed by buffer identifier (i.e. file name). + StringMap<SmallVector<ExpectedDiag, 2>> ExpectedDiagsPerFile; /// The expected diagnostics with an '@unknown' location. SmallVector<ExpectedDiag, 2> ExpectedUnknownLocDiags; diff --git a/llvm/lib/Support/SourceMgrDiagnosticVerifier.cpp b/llvm/lib/Support/SourceMgrDiagnosticVerifier.cpp index 71f4b0e1c1c1b..40c78e18ba74e 100644 --- a/llvm/lib/Support/SourceMgrDiagnosticVerifier.cpp +++ b/llvm/lib/Support/SourceMgrDiagnosticVerifier.cpp @@ -8,7 +8,6 @@ #include "llvm/Support/SourceMgrDiagnosticVerifier.h" -#include "llvm/ADT/StringSwitch.h" #include "llvm/Support/MemoryBuffer.h" #include "llvm/Support/raw_ostream.h" @@ -16,7 +15,10 @@ using namespace llvm; SourceMgrDiagnosticVerifier::SourceMgrDiagnosticVerifier(const SourceMgr &Mgr, raw_ostream &OS) - : Mgr(Mgr), OS(OS) {} + : Mgr(Mgr), OS(OS) { + for (unsigned I = 0, E = Mgr.getNumBuffers(); I != E; ++I) + (void)computeExpectedDiags(Mgr.getMemoryBuffer(I + 1)); +} /// Given a diagnostic kind, return a human readable string for it. static StringRef getDiagKindStr(SourceMgr::DiagKind Kind) { @@ -45,7 +47,8 @@ bool SourceMgrDiagnosticVerifier::emitError(const ExpectedDiag &Diag, ArrayRef<SMRange> Ranges; if (Diag.FileLoc.isValid()) { Range = - SMRange(Diag.FileLoc, SMLoc::getFromPointer(Diag.Substring.end() + 2)); + SMRange(Diag.FileLoc, SMLoc::getFromPointer(Diag.FileLoc.getPointer() + + Diag.Substring.size())); Ranges = Range; } printDiag(Mgr.GetMessage(Diag.FileLoc, SourceMgr::DK_Error, Msg, Ranges)); @@ -97,10 +100,7 @@ SourceMgrDiagnosticVerifier::computeExpectedDiags(const MemoryBuffer *Buf) { // If the buffer is invalid, return an empty list. if (!Buf) return {}; - auto [It, Inserted] = ExpectedDiagsPerBuffer.try_emplace(Buf); - if (!Inserted) - return It->second; - auto &ExpectedDiags = It->second; + auto &ExpectedDiags = ExpectedDiagsPerFile[Buf->getBufferIdentifier()]; // The number of the last line that did not correlate to a designator. unsigned LastNonDesignatorLine = 0; @@ -112,7 +112,7 @@ SourceMgrDiagnosticVerifier::computeExpectedDiags(const MemoryBuffer *Buf) { SmallVector<StringRef, 100> Lines; Buf->getBuffer().split(Lines, '\n'); for (unsigned LineNo = 0, E = Lines.size(); LineNo < E; ++LineNo) { - SmallVector<StringRef, 10> Matches; + SmallVector<StringRef, 4> Matches; if (!Expected.match(Lines[LineNo].rtrim(), &Matches)) { // Check for designators that apply to this line. if (!DesignatorsForNextLine.empty()) { @@ -127,11 +127,17 @@ SourceMgrDiagnosticVerifier::computeExpectedDiags(const MemoryBuffer *Buf) { // Point to the start of expected-*. SMLoc ExpectedStart = SMLoc::getFromPointer(Matches[0].data()); - SourceMgr::DiagKind Kind = StringSwitch<SourceMgr::DiagKind>(Matches[1]) - .Case("error", SourceMgr::DK_Error) - .Case("warning", SourceMgr::DK_Warning) - .Case("remark", SourceMgr::DK_Remark) - .Case("note", SourceMgr::DK_Note); + SourceMgr::DiagKind Kind; + if (Matches[1] == "error") + Kind = SourceMgr::DK_Error; + else if (Matches[1] == "warning") + Kind = SourceMgr::DK_Warning; + else if (Matches[1] == "remark") + Kind = SourceMgr::DK_Remark; + else { + assert(Matches[1] == "note"); + Kind = SourceMgr::DK_Note; + } ExpectedDiag Record(Kind, LineNo + 1, ExpectedStart, Matches[5]); // Check to see if this is a regex match, i.e. it includes the `-re`. @@ -144,30 +150,13 @@ SourceMgrDiagnosticVerifier::computeExpectedDiags(const MemoryBuffer *Buf) { // Get the integer value without the @ and +/- prefix. if (OffsetMatch[0] == '+' || OffsetMatch[0] == '-') { - unsigned Offset; - // Parse as decimal explicitly so leading zeros (e.g. '@+08') are not - // treated as octal by getAsInteger's radix auto-detection. - if (OffsetMatch.drop_front().getAsInteger(10, Offset)) { - emitError(Record, "invalid line offset '" + OffsetMatch + "'"); - continue; - } - - if (OffsetMatch.front() == '+') { - if (Offset > E - Record.LineNo) { - emitError(Record, "line offset '" + OffsetMatch + - "' is past the end of the file"); - continue; - } + int Offset; + OffsetMatch.drop_front().getAsInteger(0, Offset); + + if (OffsetMatch.front() == '+') Record.LineNo += Offset; - } else if (Offset > Record.LineNo) { - // Allow LineNo == 0 (Offset == Record.LineNo), which MLIR uses for - // diagnostics on the implicit top-level module. - emitError(Record, "line offset '" + OffsetMatch + - "' is before the start of the file"); - continue; - } else { + else Record.LineNo -= Offset; - } } else if (OffsetMatch.consume_front("unknown")) { // This is matching unknown locations. Record.FileLoc = SMLoc(); @@ -194,48 +183,44 @@ SourceMgrDiagnosticVerifier::computeExpectedDiags(const MemoryBuffer *Buf) { } SourceMgrDiagnosticVerifier::MatchResult -SourceMgrDiagnosticVerifier::process(SourceMgr::DiagKind Kind, bool HasLoc, - const MemoryBuffer *Buf, unsigned LineNo, +SourceMgrDiagnosticVerifier::process(SourceMgr::DiagKind Kind, + std::optional<Location> Loc, StringRef Message, bool ReportUnexpected) { MutableArrayRef<ExpectedDiag> Diags; - if (HasLoc) { + if (Loc) { // If the buffer couldn't be resolved, `Diags` stays empty: a diagnostic // with a location in an unknown file can never match anything. - if (Buf) - Diags = computeExpectedDiags(Buf); + if (Loc->Buf) { + auto It = ExpectedDiagsPerFile.find(Loc->Buf->getBufferIdentifier()); + if (It != ExpectedDiagsPerFile.end()) + Diags = It->second; + else + Diags = computeExpectedDiags(Loc->Buf); + } } else { - for (unsigned I = 0, E = Mgr.getNumBuffers(); I != E; ++I) - (void)computeExpectedDiags(Mgr.getMemoryBuffer(I + 1)); Diags = ExpectedUnknownLocDiags; } - // Search for a matching expected diagnostic. Prefer an unmatched entry so - // that multiple expected-* directives on the same line with identical or - // overlapping substrings each get a chance to match before an already-matched - // entry is reused. - ExpectedDiag *AlreadyMatched = nullptr; + // Search for a matching expected diagnostic. + // If we find something that is close then emit a more specific error. ExpectedDiag *NearMiss = nullptr; + // If this was an expected error, remember that we saw it and return. for (auto &E : Diags) { // File line must match (unless it's an unknown location). - if (HasLoc && E.LineNo != LineNo) + if (Loc && E.LineNo != Loc->LineNo) continue; - if (!E.match(Message)) - continue; - if (E.Kind == Kind) { - if (!E.Matched) { + if (E.match(Message)) { + if (E.Kind == Kind) { E.Matched = true; return MatchResult::Matched; } - AlreadyMatched = &E; - } else if (!E.Matched) { + // If this only differs based on the diagnostic kind, then consider it // to be a near miss. NearMiss = &E; } } - if (AlreadyMatched) - return MatchResult::Matched; if (!ReportUnexpected) return MatchResult::Ignored; @@ -256,40 +241,32 @@ SourceMgrDiagnosticVerifier::process(SourceMgr::DiagKind Kind, bool HasLoc, SourceMgrDiagnosticVerifier::MatchResult SourceMgrDiagnosticVerifier::process(const SMDiagnostic &Diag, bool ReportUnexpected) { - bool HasLoc = Diag.getLoc().isValid(); - const MemoryBuffer *Buf = nullptr; - if (HasLoc) { + std::optional<Location> Loc; + if (Diag.getLoc().isValid()) { + const MemoryBuffer *Buf = nullptr; if (unsigned ID = Mgr.FindBufferContainingLoc(Diag.getLoc())) Buf = Mgr.getMemoryBuffer(ID); + Loc = Location{Buf, static_cast<unsigned>(Diag.getLineNo())}; } MatchResult Result = - process(Diag.getKind(), HasLoc, Buf, - HasLoc ? static_cast<unsigned>(Diag.getLineNo()) : 0, - Diag.getMessage(), ReportUnexpected); + process(Diag.getKind(), Loc, Diag.getMessage(), ReportUnexpected); if (Result == MatchResult::Unexpected) printDiag(Diag); return Result; } bool SourceMgrDiagnosticVerifier::verify() { - // Check all buffers in SourceMgr registration order so diagnostics are - // reported deterministically and buffers that emitted no diagnostics (such as - // '.include'd files) are still scanned. Clear each buffer's vector rather - // than erasing map entries so a subsequent verify() call does not rescan - // buffers with Matched = false. + // Verify that all expected errors were seen. auto CheckExpectedDiags = [&](const ExpectedDiag &Diag) { if (!Diag.Matched) emitError(Diag, "expected " + getDiagKindStr(Diag.Kind) + " \"" + Diag.Substring + "\" was not produced"); }; - for (unsigned I = 0, E = Mgr.getNumBuffers(); I != E; ++I) { - const MemoryBuffer *Buf = Mgr.getMemoryBuffer(I + 1); - for (const ExpectedDiag &Diag : computeExpectedDiags(Buf)) + for (auto &ExpectedDiagsPair : ExpectedDiagsPerFile) + for (const ExpectedDiag &Diag : ExpectedDiagsPair.second) CheckExpectedDiags(Diag); - ExpectedDiagsPerBuffer[Buf].clear(); - } for (const ExpectedDiag &Diag : ExpectedUnknownLocDiags) CheckExpectedDiags(Diag); - ExpectedUnknownLocDiags.clear(); + ExpectedDiagsPerFile.clear(); return OK; } diff --git a/llvm/unittests/Support/SourceMgrDiagnosticVerifierTest.cpp b/llvm/unittests/Support/SourceMgrDiagnosticVerifierTest.cpp index 1e7f65e2c0b79..06e7f19ea2232 100644 --- a/llvm/unittests/Support/SourceMgrDiagnosticVerifierTest.cpp +++ b/llvm/unittests/Support/SourceMgrDiagnosticVerifierTest.cpp @@ -29,24 +29,24 @@ class SourceMgrDiagnosticVerifierTest : public testing::Test { MemoryBuffer::getMemBuffer(Text, BufferName), SMLoc()); return SM.getMemoryBuffer(ID); } + + SMDiagnostic getMessage(const MemoryBuffer *Buf, SourceMgr::DiagKind Kind, + StringRef Msg) { + return SM.GetMessage(SMLoc::getFromPointer(Buf->getBufferStart()), Kind, + Msg); + } }; } // namespace -TEST_F(SourceMgrDiagnosticVerifierTest, MatchAndVerifyIdempotent) { +TEST_F(SourceMgrDiagnosticVerifierTest, Match) { const MemoryBuffer *Buf = addBuffer("bad_op\n# expected-error@-1 {{invalid operand}}\n"); SourceMgrDiagnosticVerifier Verifier(SM, OS); - SMLoc Loc = SMLoc::getFromPointer(Buf->getBufferStart()); - SMDiagnostic Diag = - SM.GetMessage(Loc, SourceMgr::DK_Error, "invalid operand"); - EXPECT_EQ(Verifier.process(Diag), MatchResult::Matched); - EXPECT_TRUE(Verifier.verify()); - EXPECT_EQ(Output, ""); - - // Calling verify() a second time (e.g. from a destructor after an explicit - // verify() call) must not re-parse the buffer with Matched = false. + EXPECT_EQ( + Verifier.process(getMessage(Buf, SourceMgr::DK_Error, "invalid operand")), + MatchResult::Matched); EXPECT_TRUE(Verifier.verify()); EXPECT_EQ(Output, ""); } @@ -80,95 +80,44 @@ TEST_F(SourceMgrDiagnosticVerifierTest, ReportUnexpectedFalse) { // An unexpected diagnostic with ReportUnexpected = false is ignored and does // not fail verification. - SMLoc Loc = SMLoc::getFromPointer(Buf->getBufferStart()); - SMDiagnostic UnexpectedDiag = - SM.GetMessage(Loc, SourceMgr::DK_Warning, "unrelated warning"); - EXPECT_EQ(Verifier.process(UnexpectedDiag, /*ReportUnexpected=*/false), - MatchResult::Ignored); - - SMDiagnostic ExpectedDiag = - SM.GetMessage(Loc, SourceMgr::DK_Error, "expected msg"); - EXPECT_EQ(Verifier.process(ExpectedDiag, /*ReportUnexpected=*/false), - MatchResult::Matched); + EXPECT_EQ( + Verifier.process(getMessage(Buf, SourceMgr::DK_Warning, "unrelated"), + /*ReportUnexpected=*/false), + MatchResult::Ignored); + EXPECT_EQ( + Verifier.process(getMessage(Buf, SourceMgr::DK_Error, "expected msg"), + /*ReportUnexpected=*/false), + MatchResult::Matched); EXPECT_TRUE(Verifier.verify()); EXPECT_EQ(Output, ""); } -TEST_F(SourceMgrDiagnosticVerifierTest, NullBufferWithHasLocIsUnexpected) { +TEST_F(SourceMgrDiagnosticVerifierTest, UnknownFileNeverMatches) { addBuffer("# expected-error {{anything}}\n"); SourceMgrDiagnosticVerifier Verifier(SM, OS); - EXPECT_EQ(Verifier.process(SourceMgr::DK_Error, /*HasLoc=*/true, - /*Buf=*/nullptr, 1, "anything"), + SourceMgrDiagnosticVerifier::Location Loc{/*Buf=*/nullptr, /*LineNo=*/1}; + EXPECT_EQ(Verifier.process(SourceMgr::DK_Error, Loc, "anything"), MatchResult::Unexpected); EXPECT_FALSE(Verifier.verify()); } -TEST_F(SourceMgrDiagnosticVerifierTest, - MultipleExpectedDiagsOnSameLinePreferUnmatched) { - const MemoryBuffer *Buf = - addBuffer("op\n" - "# expected-error@-1 {{invalid operand}}\n" - "# expected-error@-2 {{invalid operand}}\n"); +TEST_F(SourceMgrDiagnosticVerifierTest, InstalledAsDiagHandler) { + addBuffer("op\n# expected-warning@-1 {{bad op}}\n"); SourceMgrDiagnosticVerifier Verifier(SM, OS); - - SMLoc Loc = SMLoc::getFromPointer(Buf->getBufferStart()); - SMDiagnostic Diag = - SM.GetMessage(Loc, SourceMgr::DK_Error, "invalid operand"); - EXPECT_EQ(Verifier.process(Diag), MatchResult::Matched); - EXPECT_EQ(Verifier.process(Diag), MatchResult::Matched); - EXPECT_TRUE(Verifier.verify()); - EXPECT_EQ(Output, ""); -} - -TEST_F(SourceMgrDiagnosticVerifierTest, LineZeroAndLeadingZeroOffsets) { - const MemoryBuffer *Buf = - addBuffer("// expected-error@+02 {{decimal leading zero}}\n" - "// expected-remark@-2 {{implicit module at line 0}}\n" - "op\n"); - SourceMgrDiagnosticVerifier Verifier(SM, OS); - - EXPECT_EQ(Verifier.process(SourceMgr::DK_Remark, /*HasLoc=*/true, Buf, - /*LineNo=*/0, "implicit module at line 0"), - MatchResult::Matched); - EXPECT_EQ(Verifier.process(SourceMgr::DK_Error, /*HasLoc=*/true, Buf, - /*LineNo=*/3, "decimal leading zero"), - MatchResult::Matched); - EXPECT_TRUE(Verifier.verify()); - EXPECT_EQ(Output, ""); -} - -TEST_F(SourceMgrDiagnosticVerifierTest, OutOfBoundsLineOffsets) { - addBuffer("op\n" - "# expected-error@-3 {{before start}}\n" - "# expected-error@+5 {{past end}}\n"); - SourceMgrDiagnosticVerifier Verifier(SM, OS); - - EXPECT_FALSE(Verifier.verify()); + SM.setDiagHandler( + [](const SMDiagnostic &Diag, void *Ctx) { + static_cast<SourceMgrDiagnosticVerifier *>(Ctx)->process(Diag); + }, + &Verifier); + + // The near-miss message must be printed to OS rather than re-entering the + // verifier through the SourceMgr's diagnostic handler. + SM.PrintMessage( + SMLoc::getFromPointer(SM.getMemoryBuffer(1)->getBufferStart()), + SourceMgr::DK_Error, "bad op"); EXPECT_NE(StringRef(Output).find( - "error: line offset '-3' is before the start of the file"), + "error: 'error' diagnostic emitted when expecting a 'warning'"), StringRef::npos); - EXPECT_NE(StringRef(Output).find( - "error: line offset '+5' is past the end of the file"), - StringRef::npos); -} - -TEST_F(SourceMgrDiagnosticVerifierTest, AlreadyMatchedDoesNotTriggerNearMiss) { - const MemoryBuffer *Buf = addBuffer("op\n# expected-error@-1 {{operand}}\n"); - SourceMgrDiagnosticVerifier Verifier(SM, OS); - - SMLoc Loc = SMLoc::getFromPointer(Buf->getBufferStart()); - EXPECT_EQ( - Verifier.process(SM.GetMessage(Loc, SourceMgr::DK_Error, "bad operand")), - MatchResult::Matched); - // A subsequent unexpected note containing the same substring should be - // reported as Unexpected (not as a NearMiss against the already-satisfied - // expected-error). - EXPECT_EQ( - Verifier.process(SM.GetMessage(Loc, SourceMgr::DK_Note, "note operand")), - MatchResult::Unexpected); EXPECT_FALSE(Verifier.verify()); - EXPECT_NE(StringRef(Output).find("note: note operand"), StringRef::npos); - EXPECT_EQ(StringRef(Output).find("diagnostic emitted when expecting"), - StringRef::npos); } diff --git a/mlir/lib/IR/Diagnostics.cpp b/mlir/lib/IR/Diagnostics.cpp index 5f1a86e1ed495..dd4494a6d1ba6 100644 --- a/mlir/lib/IR/Diagnostics.cpp +++ b/mlir/lib/IR/Diagnostics.cpp @@ -713,16 +713,16 @@ void SourceMgrDiagnosticVerifierHandler::process(Diagnostic &diag) { void SourceMgrDiagnosticVerifierHandler::process(LocationAttr loc, StringRef msg, DiagnosticSeverity kind) { - FileLineColLoc fileLoc = loc.findInstanceOf<FileLineColLoc>(); - const llvm::MemoryBuffer *buf = - fileLoc ? getBufferForFile(fileLoc.getFilename()) : nullptr; + using Verifier = llvm::SourceMgrDiagnosticVerifier; + std::optional<Verifier::Location> verifierLoc; + if (FileLineColLoc fileLoc = loc.findInstanceOf<FileLineColLoc>()) + verifierLoc = Verifier::Location{getBufferForFile(fileLoc.getFilename()), + fileLoc.getLine()}; bool reportUnexpected = impl->level != Level::OnlyExpected; - using MatchResult = llvm::SourceMgrDiagnosticVerifier::MatchResult; - MatchResult result = impl->verifier.process( - getDiagKind(kind), /*hasLoc=*/static_cast<bool>(fileLoc), buf, - fileLoc ? fileLoc.getLine() : 0, msg, reportUnexpected); - if (result == MatchResult::Unexpected) + Verifier::MatchResult result = impl->verifier.process( + getDiagKind(kind), verifierLoc, msg, reportUnexpected); + if (result == Verifier::MatchResult::Unexpected) emitDiagnostic(loc, "unexpected " + getDiagKindStr(kind) + ": " + msg, DiagnosticSeverity::Error); } _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
