[𝘀𝗽𝗿] initial version

Created using spr 1.3.6

Author: vitalybuka

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

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

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/*

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,

clang/lib/Basic/SanitizerSpecialCaseList.cpp

@@ -42,7 +42,7 @@ void SanitizerSpecialCaseList::createSanitizerSections() {
     SanitizerMask Mask;
 
 #define SANITIZER(NAME, ID)                                                    \
-  if (S.SectionMatcher->match(NAME))                                           \
+  if (S.SectionMatcher.match(NAME))                                            \
     Mask |= SanitizerKind::ID;
 #define SANITIZER_GROUP(NAME, ID, ALIAS) SANITIZER(NAME, ID)
 

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) {

llvm/include/llvm/Support/SpecialCaseList.h

@@ -13,12 +13,14 @@
 #define LLVM_SUPPORT_SPECIALCASELIST_H
 
 #include "llvm/ADT/StringMap.h"
+#include "llvm/Support/Allocator.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/GlobPattern.h"
 #include "llvm/Support/Regex.h"
 #include <memory>
 #include <string>
 #include <utility>
+#include <variant>
 #include <vector>
 
 namespace llvm {
@@ -118,46 +120,65 @@ class SpecialCaseList {
   SpecialCaseList(SpecialCaseList const &) = delete;
   SpecialCaseList &operator=(SpecialCaseList const &) = delete;
 
-  /// Represents a set of globs and their line numbers
-  class Matcher {
+  // Lagacy v1 matcher.
+  class RegexMatcher {
+  public:
+    LLVM_ABI unsigned match(StringRef Query) const;
+    LLVM_ABI Error insert(StringRef Pattern, unsigned LineNumber);
+    std::vector<std::pair<std::unique_ptr<Regex>, unsigned>> RegExes;
+  };
+
+  class GlobMatcher {
   public:
-    LLVM_ABI Error insert(StringRef Pattern, unsigned LineNumber,
-                          bool UseRegex);
     // Returns the line number in the source file that this query matches to.
     // Returns zero if no match is found.
     LLVM_ABI unsigned match(StringRef Query) const;
-
+    LLVM_ABI Error insert(StringRef Pattern, unsigned LineNumber);
     struct Glob {
-      std::string Name;
+      Glob(unsigned LineNo, GlobPattern &&Pattern)
+          : LineNo(LineNo), Pattern(std::move(Pattern)) {}
       unsigned LineNo;
       GlobPattern Pattern;
-      // neither copyable nor movable because GlobPattern contains
-      // Glob::StringRef that points to Glob::Name.
-      Glob(Glob &&) = delete;
-      Glob() = default;
     };
 
-    std::vector<std::unique_ptr<Matcher::Glob>> Globs;
-    std::vector<std::pair<std::unique_ptr<Regex>, unsigned>> RegExes;
+    std::vector<GlobMatcher::Glob> Globs;
+  };
+
+  /// Represents a set of globs and their line numbers
+  class Matcher {
+  public:
+    LLVM_ABI explicit Matcher(bool UseGlobs);
+    // Returns the line number in the source file that this query matches to.
+    // Returns zero if no match is found.
+    LLVM_ABI unsigned match(StringRef Query) const;
+
+  private:
+    friend class SpecialCaseList;
+    LLVM_ABI Error insert(StringRef Pattern, unsigned LineNumber);
+
+    std::variant<RegexMatcher, GlobMatcher> M;
   };
 
   using SectionEntries = StringMap<StringMap<Matcher>>;
 
   struct Section {
-    Section(StringRef Str, unsigned FileIdx)
-        : SectionStr(Str), FileIdx(FileIdx) {};
+    Section(StringRef Str, unsigned FileIdx, bool UseGlobs)
+        : SectionMatcher(UseGlobs), SectionStr(Str), FileIdx(FileIdx) {};
+
+    Section(Section &&) = default;
 
-    std::unique_ptr<Matcher> SectionMatcher = std::make_unique<Matcher>();
+    Matcher SectionMatcher;
     SectionEntries Entries;
     std::string SectionStr;
     unsigned FileIdx;
   };
 
+  BumpPtrAllocator StrAlloc;
   std::vector<Section> Sections;
 
   LLVM_ABI Expected<Section *> addSection(StringRef SectionStr,
                                           unsigned FileIdx, unsigned LineNo,
-                                          bool UseGlobs = true);
+                                          bool UseGlobs);
 
   /// Parses just-constructed SpecialCaseList entries from a memory buffer.
   LLVM_ABI bool parse(unsigned FileIdx, const MemoryBuffer *MB,

llvm/lib/Support/SpecialCaseList.cpp

@@ -25,57 +25,75 @@
 
 namespace llvm {
 
-Error SpecialCaseList::Matcher::insert(StringRef Pattern, unsigned LineNumber,
-                                       bool UseGlobs) {
-  if (Pattern.empty())
+Error SpecialCaseList::RegexMatcher::insert(StringRef Pattern,
+                                            unsigned LineNumber) {
+  if (Pattern.empty()) {
     return createStringError(errc::invalid_argument,
-                             Twine("Supplied ") +
-                                 (UseGlobs ? "glob" : "regex") + " was blank");
-
-  if (!UseGlobs) {
-    // Replace * with .*
-    auto Regexp = Pattern.str();
-    for (size_t pos = 0; (pos = Regexp.find('*', pos)) != std::string::npos;
-         pos += strlen(".*")) {
-      Regexp.replace(pos, strlen("*"), ".*");
-    }
+                             "Supplied regex was blank");
+  }
 
-    Regexp = (Twine("^(") + StringRef(Regexp) + ")$").str();
+  // Replace * with .*
+  auto Regexp = Pattern.str();
+  for (size_t pos = 0; (pos = Regexp.find('*', pos)) != std::string::npos;
+       pos += strlen(".*")) {
+    Regexp.replace(pos, strlen("*"), ".*");
+  }
 
-    // Check that the regexp is valid.
-    Regex CheckRE(Regexp);
-    std::string REError;
-    if (!CheckRE.isValid(REError))
-      return createStringError(errc::invalid_argument, REError);
+  Regexp = (Twine("^(") + StringRef(Regexp) + ")$").str();
 
-    RegExes.emplace_back(std::make_pair(
-        std::make_unique<Regex>(std::move(CheckRE)), LineNumber));
+  // Check that the regexp is valid.
+  Regex CheckRE(Regexp);
+  std::string REError;
+  if (!CheckRE.isValid(REError))
+    return createStringError(errc::invalid_argument, REError);
 
-    return Error::success();
-  }
+  RegExes.emplace_back(
+      std::make_pair(std::make_unique<Regex>(std::move(CheckRE)), LineNumber));
 
-  auto Glob = std::make_unique<Matcher::Glob>();
-  Glob->Name = Pattern.str();
-  Glob->LineNo = LineNumber;
-  // We must be sure to use the string in `Glob` rather than the provided
-  // reference which could be destroyed before match() is called
-  if (auto Err = GlobPattern::create(Glob->Name, /*MaxSubPatterns=*/1024)
-                     .moveInto(Glob->Pattern))
-    return Err;
-  Globs.push_back(std::move(Glob));
   return Error::success();
 }
 
-unsigned SpecialCaseList::Matcher::match(StringRef Query) const {
-  for (const auto &Glob : reverse(Globs))
-    if (Glob->Pattern.match(Query))
-      return Glob->LineNo;
+unsigned SpecialCaseList::RegexMatcher::match(StringRef Query) const {
   for (const auto &[Regex, LineNumber] : reverse(RegExes))
     if (Regex->match(Query))
       return LineNumber;
   return 0;
 }
 
+Error SpecialCaseList::GlobMatcher::insert(StringRef Pattern,
+                                           unsigned LineNumber) {
+  if (Pattern.empty())
+    return createStringError(errc::invalid_argument, "Supplied glob was blank");
+
+  auto Res = GlobPattern::create(Pattern, /*MaxSubPatterns=*/1024);
+  if (auto Err = Res.takeError())
+    return Err;
+  Globs.emplace_back(LineNumber, std::move(Res.get()));
+  return Error::success();
+}
+
+unsigned SpecialCaseList::GlobMatcher::match(StringRef Query) const {
+  for (const auto &Glob : reverse(Globs))
+    if (Glob.Pattern.match(Query))
+      return Glob.LineNo;
+  return 0;
+}
+
+SpecialCaseList::Matcher::Matcher(bool UseGlobs) {
+  if (UseGlobs)
+    M.emplace<GlobMatcher>();
+  else
+    M.emplace<RegexMatcher>();
+}
+
+unsigned SpecialCaseList::Matcher::match(StringRef Query) const {
+  return std::visit([&](auto &V) { return V.match(Query); }, M);
+}
+
+Error SpecialCaseList::Matcher::insert(StringRef Pattern, unsigned LineNumber) {
+  return std::visit([&](auto &V) { return V.insert(Pattern, LineNumber); }, M);
+}
+
 // TODO: Refactor this to return Expected<...>
 std::unique_ptr<SpecialCaseList>
 SpecialCaseList::create(const std::vector<std::string> &Paths,
@@ -132,10 +150,11 @@ bool SpecialCaseList::createInternal(const MemoryBuffer *MB,
 Expected<SpecialCaseList::Section *>
 SpecialCaseList::addSection(StringRef SectionStr, unsigned FileNo,
                             unsigned LineNo, bool UseGlobs) {
-  Sections.emplace_back(SectionStr, FileNo);
+  Sections.emplace_back(SectionStr, FileNo, UseGlobs);
   auto &Section = Sections.back();
 
-  if (auto Err = Section.SectionMatcher->insert(SectionStr, LineNo, UseGlobs)) {
+  SectionStr = SectionStr.copy(StrAlloc);
+  if (auto Err = Section.SectionMatcher.insert(SectionStr, LineNo)) {
     return createStringError(errc::invalid_argument,
                              "malformed section at line " + Twine(LineNo) +
                                  ": '" + SectionStr +
@@ -148,7 +167,7 @@ SpecialCaseList::addSection(StringRef SectionStr, unsigned FileNo,
 bool SpecialCaseList::parse(unsigned FileIdx, const MemoryBuffer *MB,
                             std::string &Error) {
   Section *CurrentSection;
-  if (auto Err = addSection("*", FileIdx, 1).moveInto(CurrentSection)) {
+  if (auto Err = addSection("*", FileIdx, 1, true).moveInto(CurrentSection)) {
     Error = toString(std::move(Err));
     return false;
   }
@@ -194,8 +213,10 @@ bool SpecialCaseList::parse(unsigned FileIdx, const MemoryBuffer *MB,
     }
 
     auto [Pattern, Category] = Postfix.split("=");
-    auto &Entry = CurrentSection->Entries[Prefix][Category];
-    if (auto Err = Entry.insert(Pattern, LineNo, UseGlobs)) {
+    auto [It, _] =
+        CurrentSection->Entries[Prefix].try_emplace(Category, UseGlobs);
+    Pattern = Pattern.copy(StrAlloc);
+    if (auto Err = It->second.insert(Pattern, LineNo)) {
       Error =
           (Twine("malformed ") + (UseGlobs ? "glob" : "regex") + " in line " +
            Twine(LineNo) + ": '" + Pattern + "': " + toString(std::move(Err)))
@@ -218,7 +239,7 @@ std::pair<unsigned, unsigned>
 SpecialCaseList::inSectionBlame(StringRef Section, StringRef Prefix,
                                 StringRef Query, StringRef Category) const {
   for (const auto &S : reverse(Sections)) {
-    if (S.SectionMatcher->match(Section)) {
+    if (S.SectionMatcher.match(Section)) {
       unsigned Blame = inSectionBlame(S.Entries, Prefix, Query, Category);
       if (Blame)
         return {S.FileIdx, Blame};