[clang] Clang multi-match behavior of WarningSuppressionMappings

[vitalybuka]

"The longest wins" is confusing.
E.g. why here test/ should win?

src:*test/*
src:*lld/*=emit

For long suppression files length relationship can be hard to track.

Easier to understand rule is "the last wins".

We changed sanitizers to this approach way and
would be nice to have consistency hare.

Also consistency allows us avoid exposing
internals of SpecialCaseList in future.

[llvmbot]

@llvm/pr-subscribers-llvm-support

@llvm/pr-subscribers-clang

Author: Vitaly Buka (vitalybuka)

Changes

"The longest wins" is confusing.
E.g. why here test/ should win?

src:*test/*
src:*lld/*=emit

For long suppression files length relationship can be hard to track.

Easier to understand rule is "the last wins".

We changed sanitizers to this approach way and
would be nice to have consistency hare.

Also consistency allows us avoid exposing
internals of SpecialCaseList in future.

---

Full diff: https://github.com/llvm/llvm-project/pull/162237.diff

5 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+2)
• (modified) clang/docs/WarningSuppressionMappings.rst (+2-2)
• (modified) clang/include/clang/Basic/Diagnostic.h (+1-1)
• (modified) clang/lib/Basic/Diagnostic.cpp (+15-38)
• (modified) clang/unittests/Basic/DiagnosticTest.cpp (+3-5)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 390e0fa7a9a2b..84ad1ae64f06b 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -69,6 +69,8 @@ Potentially Breaking Changes
   call the member ``operator delete`` instead of the expected global
   delete operator. The old behavior is retained under ``-fclang-abi-compat=21``
   flag.
+- Clang warning suppressions file, ``--warning-suppression-mappings=``, now will
+  use the last matching entry instead of the longest one.
 
 C/C++ Language Potentially Breaking Changes
 -------------------------------------------
diff --git a/clang/docs/WarningSuppressionMappings.rst b/clang/docs/WarningSuppressionMappings.rst
index d96341ac6e563..d8af856f64ef0 100644
--- a/clang/docs/WarningSuppressionMappings.rst
+++ b/clang/docs/WarningSuppressionMappings.rst
@@ -63,7 +63,7 @@ Format
 Warning suppression mappings uses the same format as
 :doc:`SanitizerSpecialCaseList`.
 
-Sections describe which diagnostic group's behaviour to change, e.g.
+Sections describe which diagnostic group's behavior to change, e.g.
 ``[unused]``. When a diagnostic is matched by multiple sections, the latest
 section takes precedence.
 
@@ -76,7 +76,7 @@ Source files are matched against these globs either:
 - as paths relative to the current working directory
 - as absolute paths.
 
-When a source file matches multiple globs in a section, the longest one takes
+When a source file matches multiple globs in a section, the last one takes
 precedence.
 
 .. code-block:: bash
diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h
index e540040ddc524..bd3165f64f4a0 100644
--- a/clang/include/clang/Basic/Diagnostic.h
+++ b/clang/include/clang/Basic/Diagnostic.h
@@ -971,7 +971,7 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
   /// diagnostics in specific files.
   /// Mapping file is expected to be a special case list with sections denoting
   /// diagnostic groups and `src` entries for globs to suppress. `emit` category
-  /// can be used to disable suppression. Longest glob that matches a filepath
+  /// can be used to disable suppression. THe last glob that matches a filepath
   /// takes precedence. For example:
   ///   [unused]
   ///   src:clang/*
diff --git a/clang/lib/Basic/Diagnostic.cpp b/clang/lib/Basic/Diagnostic.cpp
index 2b89370a42d1b..b6dad86c038d3 100644
--- a/clang/lib/Basic/Diagnostic.cpp
+++ b/clang/lib/Basic/Diagnostic.cpp
@@ -517,12 +517,6 @@ class WarningsSpecialCaseList : public llvm::SpecialCaseList {
                         const SourceManager &SM) const;
 
 private:
-  // Find the longest glob pattern that matches FilePath amongst
-  // CategoriesToMatchers, return true iff the match exists and belongs to a
-  // positive category.
-  bool globsMatches(const llvm::StringMap<Matcher> &CategoriesToMatchers,
-                    StringRef FilePath) const;
-
   llvm::DenseMap<diag::kind, const Section *> DiagToSection;
 };
 } // namespace
@@ -584,43 +578,26 @@ void DiagnosticsEngine::setDiagSuppressionMapping(llvm::MemoryBuffer &Input) {
 bool WarningsSpecialCaseList::isDiagSuppressed(diag::kind DiagId,
                                                SourceLocation DiagLoc,
                                                const SourceManager &SM) const {
+  PresumedLoc PLoc = SM.getPresumedLoc(DiagLoc);
+  if (!PLoc.isValid())
+    return false;
   const Section *DiagSection = DiagToSection.lookup(DiagId);
   if (!DiagSection)
     return false;
-  const SectionEntries &EntityTypeToCategories = DiagSection->Entries;
-  auto SrcEntriesIt = EntityTypeToCategories.find("src");
-  if (SrcEntriesIt == EntityTypeToCategories.end())
+
+  StringRef F = llvm::sys::path::remove_leading_dotslash(PLoc.getFilename());
+
+  unsigned SuppressLineNo =
+      llvm::SpecialCaseList::inSectionBlame(DiagSection->Entries, "src", F, "");
+  if (!SuppressLineNo)
     return false;
-  const llvm::StringMap<llvm::SpecialCaseList::Matcher> &CategoriesToMatchers =
-      SrcEntriesIt->getValue();
-  // We also use presumed locations here to improve reproducibility for
-  // preprocessed inputs.
-  if (PresumedLoc PLoc = SM.getPresumedLoc(DiagLoc); PLoc.isValid())
-    return globsMatches(
-        CategoriesToMatchers,
-        llvm::sys::path::remove_leading_dotslash(PLoc.getFilename()));
-  return false;
-}
 
-bool WarningsSpecialCaseList::globsMatches(
-    const llvm::StringMap<Matcher> &CategoriesToMatchers,
-    StringRef FilePath) const {
-  StringRef LongestMatch;
-  bool LongestIsPositive = false;
-  for (const auto &Entry : CategoriesToMatchers) {
-    StringRef Category = Entry.getKey();
-    const llvm::SpecialCaseList::Matcher &Matcher = Entry.getValue();
-    bool IsPositive = Category != "emit";
-    for (const auto &Glob : Matcher.Globs) {
-      if (Glob->Name.size() < LongestMatch.size())
-        continue;
-      if (!Glob->Pattern.match(FilePath))
-        continue;
-      LongestMatch = Glob->Name;
-      LongestIsPositive = IsPositive;
-    }
-  }
-  return LongestIsPositive;
+  unsigned EmitLineNo = llvm::SpecialCaseList::inSectionBlame(
+      DiagSection->Entries, "src", F, "emit");
+  if (!EmitLineNo)
+    return true;
+
+  return SuppressLineNo > EmitLineNo;
 }
 
 bool DiagnosticsEngine::isSuppressedViaMapping(diag::kind DiagId,
diff --git a/clang/unittests/Basic/DiagnosticTest.cpp b/clang/unittests/Basic/DiagnosticTest.cpp
index de090864e5095..5492146f40fa9 100644
--- a/clang/unittests/Basic/DiagnosticTest.cpp
+++ b/clang/unittests/Basic/DiagnosticTest.cpp
@@ -294,7 +294,7 @@ TEST_F(SuppressionMappingTest, EmitCategoryIsExcluded) {
                                             locForFile("foo.cpp")));
 }
 
-TEST_F(SuppressionMappingTest, LongestMatchWins) {
+TEST_F(SuppressionMappingTest, LastMatchWins) {
   llvm::StringLiteral SuppressionMappingFile = R"(
   [unused]
   src:*clang/*
@@ -327,10 +327,8 @@ TEST_F(SuppressionMappingTest, LongShortMatch) {
 
   EXPECT_TRUE(Diags.isSuppressedViaMapping(diag::warn_unused_function,
                                            locForFile("test/t1.cpp")));
-
-  // FIXME: This is confusing.
-  EXPECT_TRUE(Diags.isSuppressedViaMapping(diag::warn_unused_function,
-                                           locForFile("lld/test/t2.cpp")));
+  EXPECT_FALSE(Diags.isSuppressedViaMapping(diag::warn_unused_function,
+                                            locForFile("lld/test/t2.cpp")));
 }
 
 TEST_F(SuppressionMappingTest, ShortLongMatch) {

[fmayer]

I don't know how many use this, and if we need some gentler way to change this behavior to avoid breaking people.

[vitalybuka]

I don't know how many use this, and if we need some gentler way to change this behavior to avoid breaking people.

This file is relatively new.
We did such change for sanitizer, no complains so far.

And it's very nuanced case ON/OFF with "emit". I see how it can be theoretically useful and good for completeness, but likely only suppression case is used to temporarily suppression.

Also I am not sure ignore/suppression list does deserve significant compatibility work.
We can do that, but seems unnecessary.

[fmayer]

I don't know how many use this, and if we need some gentler way to change this behavior to avoid breaking people.

This file is relatively new. We did such change for sanitizer, no complains so far.

And it's very nuanced case ON/OFF with "emit". I see how it can be theoretically useful and good for completeness, but likely only suppression case is used to temporarily suppression.

Also I am not sure ignore/suppression list does deserve significant compatibility work. We can do that, but seems unnecessary.

I'll defer to someone who has worked on the warning suppression list.