Canonicalize special case list filenames when loading

Author: DKLoehr

clang/docs/SanitizerSpecialCaseList.rst

@@ -174,7 +174,6 @@ tool-specific docs.
     # Lines starting with # are ignored.
     # Turn off checks for the source file
     # Entries without sections are placed into [*] and apply to all sanitizers
-    # On windows, "/" also matches "\" in filenames
     src:path/to/source/file.c
     src:*/source/file.c
     # Turn off checks for this main file, including files included by it.
@@ -197,6 +196,23 @@ tool-specific docs.
     [{cfi-vcall,cfi-icall}]
     fun:*BadCfiCall
 
+
+.. note::
+
+  By default, ``src`` and ``mainfile`` are matched against the filename as seen
+  by LLVM. On Windows, this might involve a mix of forward and backslashes as
+  file separators, and writing patterns to match both variants can be
+  inconvenient.
+  
+  If the special case list file begins with ``#!canonical-paths``, then paths
+  will be canonicalized before patterns are matched against them. This involves
+  stripping any leading dots and slashes, and (on Windows only) converting all
+  backslashes to forward slashes.
+
+  If the file uses both ``#!special-case-list-v1`` and ``#!canonical-paths``,
+  then they should occupy the first two lines, and ``#!canonical-paths`` must
+  appear on the second line.
+
 ``mainfile`` is similar to applying ``-fno-sanitize=`` to a set of files but
 does not need plumbing into the build system. This works well for internal
 linkage functions but has a caveat for C++ vague linkage functions.

clang/lib/Basic/Diagnostic.cpp

@@ -612,18 +612,24 @@ bool WarningsSpecialCaseList::isDiagSuppressed(diag::kind DiagId,
       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()));
+  if (PresumedLoc PLoc = SM.getPresumedLoc(DiagLoc); PLoc.isValid()) {
+    if (CanonicalizePaths) {
+      return globsMatches(
+          CategoriesToMatchers,
+          llvm::sys::path::convert_to_slash(
+              llvm::sys::path::remove_leading_dotslash(PLoc.getFilename())));
+    } else {
+      return globsMatches(
+          CategoriesToMatchers,
+          llvm::sys::path::remove_leading_dotslash(PLoc.getFilename()));
+    }
+  }
   return false;
 }
 
 bool WarningsSpecialCaseList::globsMatches(
     const llvm::StringMap<Matcher> &CategoriesToMatchers,
     StringRef FilePath) const {
-  static bool HaveWindowsPathStyle =
-      llvm::sys::path::is_style_windows(llvm::sys::path::Style::native);
   StringRef LongestMatch;
   bool LongestIsPositive = false;
   for (const auto &Entry : CategoriesToMatchers) {
@@ -633,8 +639,7 @@ bool WarningsSpecialCaseList::globsMatches(
     for (const auto &Glob : Matcher.Globs) {
       if (Glob->Name.size() < LongestMatch.size())
         continue;
-      if (!Glob->Pattern.match(FilePath,
-                               /*IsSlashAgnostic=*/HaveWindowsPathStyle))
+      if (!Glob->Pattern.match(FilePath))
         continue;
       LongestMatch = Glob->Name;
       LongestIsPositive = IsPositive;

clang/lib/Basic/SanitizerSpecialCaseList.cpp

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

clang/unittests/Basic/DiagnosticTest.cpp

@@ -362,9 +362,8 @@ TEST_F(SuppressionMappingTest, ParsingRespectsOtherWarningOpts) {
 }
 
 #ifdef _WIN32
-// We're only slash-agnostic on windows hosts
-TEST_F(SuppressionMappingTest, TreatsFilesAsSlashAgnosticOnWindows) {
-  llvm::StringLiteral SuppressionMappingFile = R"(
+TEST_F(SuppressionMappingTest, CanonicalizesSlashesOnWindows) {
+  llvm::StringLiteral SuppressionMappingFile = R"(#!canonical-paths
   [unused]
   src:*clang/*
   src:*clang/lib/Sema/*=emit
@@ -378,14 +377,21 @@ TEST_F(SuppressionMappingTest, TreatsFilesAsSlashAgnosticOnWindows) {
 
   EXPECT_TRUE(Diags.isSuppressedViaMapping(
       diag::warn_unused_function, locForFile(R"(clang/lib/Basic/bar.h)")));
+  EXPECT_TRUE(Diags.isSuppressedViaMapping(
+      diag::warn_unused_function, locForFile(R"(clang/lib/Basic\bar.h)")));
+  EXPECT_TRUE(Diags.isSuppressedViaMapping(
+      diag::warn_unused_function, locForFile(R"(clang\lib/Basic/bar.h)")));
+  EXPECT_FALSE(Diags.isSuppressedViaMapping(
+      diag::warn_unused_function, locForFile(R"(clang/lib/Sema/baz.h)")));
   EXPECT_FALSE(Diags.isSuppressedViaMapping(
       diag::warn_unused_function, locForFile(R"(clang/lib/Sema\baz.h)")));
 
-  // We require a literal backslash before "Sema"
-  EXPECT_TRUE(Diags.isSuppressedViaMapping(
+  // The backslash gets canonicalized so we never match the third pattern
+  EXPECT_FALSE(Diags.isSuppressedViaMapping(
       diag::warn_unused_function, locForFile(R"(clang\lib\Sema/foo.h)")));
   EXPECT_FALSE(Diags.isSuppressedViaMapping(
       diag::warn_unused_function, locForFile(R"(clang/lib/Sema/foo.h)")));
 }
 #endif
+
 } // namespace

llvm/docs/ReleaseNotes.md

@@ -174,9 +174,10 @@ Changes to BOLT
 Changes to Sanitizers
 ---------------------
 
-* On windows hosts, the [sanitizer special case list format](https://clang.llvm.org/docs/SanitizerSpecialCaseList.html#format)
-  now treats forward slashes in filenames as matching either a forward or a
-  backslash, to accommodate paths with mixed unix and windows styles.
+* (Sanitizer Special Case Lists)[https://clang.llvm.org/docs/SanitizerSpecialCaseList.html]
+  may now be prefixed with ``#!canonical-paths`` to specify that filename patterns
+  should be matched against canonicalized paths, without leading dots or slashes
+  and (on Windows only) without any backslashes.
 
 Other Changes
 -------------

llvm/include/llvm/Support/GlobPattern.h

@@ -36,8 +36,6 @@ namespace llvm {
 ///   brace expansions are not supported and characters `{,}` are treated as
 ///   literals.
 /// * `\` escapes the next character so it is treated as a literal.
-/// * If \p IsSlashAgnostic is passed to the match function, then forward
-///   slashes `/` also match backslashes `\`.
 ///
 /// Some known edge cases are:
 /// * The literal `]` is allowed as the first character in a character class,
@@ -59,9 +57,8 @@ class GlobPattern {
   ///                       brace expansion
   LLVM_ABI static Expected<GlobPattern>
   create(StringRef Pat, std::optional<size_t> MaxSubPatterns = {});
-  /// \param IsSlashAgnostic whether to treat '/' as also matching '\'
   /// \returns \p true if \p S matches this glob pattern
-  LLVM_ABI bool match(StringRef S, bool IsSlashAgnostic = false) const;
+  LLVM_ABI bool match(StringRef S) const;
 
   // Returns true for glob pattern "*". Can be used to avoid expensive
   // preparation/acquisition of the input for match().
@@ -79,9 +76,8 @@ class GlobPattern {
   struct SubGlobPattern {
     /// \param Pat the pattern to match against
     LLVM_ABI static Expected<SubGlobPattern> create(StringRef Pat);
-    /// \param IsSlashAgnostic whether to treat '/' as also matching '\'
     /// \returns \p true if \p S matches this glob pattern
-    LLVM_ABI bool match(StringRef S, bool IsSlashAgnostic) const;
+    LLVM_ABI bool match(StringRef S) const;
     StringRef getPat() const { return StringRef(Pat.data(), Pat.size()); }
 
     // Brackets with their end position and matched bytes.

llvm/include/llvm/Support/SpecialCaseList.h

@@ -122,11 +122,10 @@ class SpecialCaseList {
   class Matcher {
   public:
     LLVM_ABI Error insert(StringRef Pattern, unsigned LineNumber,
-                          bool UseRegex);
+                          bool UseGlobs);
     // Returns the line number in the source file that this query matches to.
-    // On windows, treat '/' as also matching '\' in filenames when using globs.
-    // Returns zero if no match is found
-    LLVM_ABI unsigned match(StringRef Query, bool IsFilename) const;
+    // Returns zero if no match is found.
+    LLVM_ABI unsigned match(StringRef Query) const;
 
     struct Glob {
       std::string Name;
@@ -155,6 +154,7 @@ class SpecialCaseList {
   };
 
   std::vector<Section> Sections;
+  bool CanonicalizePaths = false;
 
   LLVM_ABI Expected<Section *> addSection(StringRef SectionStr,
                                           unsigned FileIdx, unsigned LineNo,

llvm/lib/Support/GlobPattern.cpp

@@ -137,7 +137,7 @@ GlobPattern::create(StringRef S, std::optional<size_t> MaxSubPatterns) {
   GlobPattern Pat;
 
   // Store the prefix that does not contain any metacharacter.
-  size_t PrefixSize = S.find_first_of("?*[{\\/");
+  size_t PrefixSize = S.find_first_of("?*[{\\");
   Pat.Prefix = S.substr(0, PrefixSize);
   if (PrefixSize == std::string::npos)
     return Pat;
@@ -190,22 +190,21 @@ GlobPattern::SubGlobPattern::create(StringRef S) {
   return Pat;
 }
 
-bool GlobPattern::match(StringRef S, bool IsSlashAgnostic) const {
+bool GlobPattern::match(StringRef S) const {
   if (!S.consume_front(Prefix))
     return false;
   if (SubGlobs.empty() && S.empty())
     return true;
   for (auto &Glob : SubGlobs)
-    if (Glob.match(S, IsSlashAgnostic))
+    if (Glob.match(S))
       return true;
   return false;
 }
 
 // Factor the pattern into segments split by '*'. The segment is matched
-// sequentially by finding the first occurrence past the end of the previous
+// sequentianlly by finding the first occurrence past the end of the previous
 // match.
-bool GlobPattern::SubGlobPattern::match(StringRef Str,
-                                        bool IsSlashAgnostic) const {
+bool GlobPattern::SubGlobPattern::match(StringRef Str) const {
   const char *P = Pat.data(), *SegmentBegin = nullptr, *S = Str.data(),
              *SavedS = S;
   const char *const PEnd = P + Pat.size(), *const End = S + Str.size();
@@ -232,10 +231,6 @@ bool GlobPattern::SubGlobPattern::match(StringRef Str,
         ++S;
         continue;
       }
-    } else if (IsSlashAgnostic && *P == '/' && *S == '\\') {
-      ++P;
-      ++S;
-      continue;
     } else if (*P == *S || *P == '?') {
       ++P;
       ++S;

llvm/lib/Support/SpecialCaseList.cpp

@@ -17,7 +17,6 @@
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/LineIterator.h"
 #include "llvm/Support/MemoryBuffer.h"
-#include "llvm/Support/Path.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include <stdio.h>
 #include <string>
@@ -67,13 +66,9 @@ Error SpecialCaseList::Matcher::insert(StringRef Pattern, unsigned LineNumber,
   return Error::success();
 }
 
-unsigned SpecialCaseList::Matcher::match(StringRef Query,
-                                         bool IsFilename) const {
-  static bool HaveWindowsPathStyle =
-      llvm::sys::path::is_style_windows(llvm::sys::path::Style::native);
+unsigned SpecialCaseList::Matcher::match(StringRef Query) const {
   for (const auto &Glob : reverse(Globs))
-    if (Glob->Pattern.match(
-            Query, /*IsSlashAgnostic=*/(HaveWindowsPathStyle && IsFilename)))
+    if (Glob->Pattern.match(Query))
       return Glob->LineNo;
   for (const auto &[Regex, LineNumber] : reverse(RegExes))
     if (Regex->match(Query))
@@ -158,12 +153,17 @@ bool SpecialCaseList::parse(unsigned FileIdx, const MemoryBuffer *MB,
     return false;
   }
 
+  // Scan the start of the file for special comments. These don't appear when
+  // iterating below because comment lines are automatically skipped.
+  StringRef Buffer = MB->getBuffer();
   // In https://reviews.llvm.org/D154014 we added glob support and planned to
   // remove regex support in patterns. We temporarily support the original
-  // behavior using regexes if "#!special-case-list-v1" is the first line of the
-  // file. For more details, see
+  // behavior using regexes if "#!special-case-list-v1" is the first line of
+  // the file. For more details, see
   // https://discourse.llvm.org/t/use-glob-instead-of-regex-for-specialcaselists/71666
-  bool UseGlobs = !MB->getBuffer().starts_with("#!special-case-list-v1\n");
+  bool UseGlobs = !Buffer.consume_front("#!special-case-list-v1\n");
+  // Specifies that patterns should be matched against canonicalized filepaths.
+  CanonicalizePaths = Buffer.consume_front("#!canonical-paths\n");
 
   for (line_iterator LineIt(*MB, /*SkipBlanks=*/true, /*CommentMarker=*/'#');
        !LineIt.is_at_eof(); LineIt++) {
@@ -223,8 +223,7 @@ std::pair<unsigned, unsigned>
 SpecialCaseList::inSectionBlame(StringRef Section, StringRef Prefix,
                                 StringRef Query, StringRef Category) const {
   for (const auto &S : reverse(Sections)) {
-    bool IsFilename = Prefix == "src" || Prefix == "mainfile";
-    if (S.SectionMatcher->match(Section, IsFilename)) {
+    if (S.SectionMatcher->match(Section)) {
       unsigned Blame = inSectionBlame(S.Entries, Prefix, Query, Category);
       if (Blame)
         return {S.FileIdx, Blame};
@@ -243,8 +242,12 @@ unsigned SpecialCaseList::inSectionBlame(const SectionEntries &Entries,
   if (II == I->second.end())
     return 0;
 
-  bool IsFilename = Prefix == "src" || Prefix == "mainfile";
-  return II->getValue().match(Query, IsFilename);
+  if (CanonicalizePaths && (Prefix == "src" || Prefix == "mainfile")) {
+    return II->getValue().match(llvm::sys::path::convert_to_slash(
+        llvm::sys::path::remove_leading_dotslash(Query)));
+  } else {
+    return II->getValue().match(Query);
+  }
 }
 
 } // namespace llvm

llvm/unittests/Support/GlobPatternTest.cpp

@@ -271,13 +271,4 @@ TEST_F(GlobPatternTest, Pathological) {
   EXPECT_FALSE(Pat->match(S));
   EXPECT_TRUE(Pat->match(S + 'b'));
 }
-
-TEST_F(GlobPatternTest, SlashAgnostic) {
-  auto Pat = GlobPattern::create("clang/*");
-  ASSERT_TRUE((bool)Pat);
-  EXPECT_TRUE(Pat->match("clang/foo"));
-  EXPECT_FALSE(Pat->match(R"(clang\foo)"));
-  EXPECT_TRUE(Pat->match("clang/foo", /*isSlashAgnostic=*/true));
-  EXPECT_TRUE(Pat->match(R"(clang\foo)", /*isSlashAgnostic=*/true));
-}
 }