Incorporate review feedback

Author: DKLoehr

clang/docs/SanitizerSpecialCaseList.rst

@@ -174,7 +174,7 @@ 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, "/" matches both styles of path separator ("/" and "\")
+    # 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.

clang/lib/Basic/Diagnostic.cpp

@@ -622,6 +622,8 @@ bool WarningsSpecialCaseList::isDiagSuppressed(diag::kind DiagId,
 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) {
@@ -631,7 +633,8 @@ bool WarningsSpecialCaseList::globsMatches(
     for (const auto &Glob : Matcher.Globs) {
       if (Glob->Name.size() < LongestMatch.size())
         continue;
-      if (!Glob->Pattern.match(FilePath))
+      if (!Glob->Pattern.match(FilePath,
+                               /*IsSlashAgnostic=*/HaveWindowsPathStyle))
         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))                                           \
+  if (S.SectionMatcher->match(NAME, /*IsFilename=*/false))                     \
     Mask |= SanitizerKind::ID;
 #define SANITIZER_GROUP(NAME, ID, ALIAS) SANITIZER(NAME, ID)
 

clang/unittests/Basic/DiagnosticTest.cpp

@@ -363,25 +363,27 @@ TEST_F(SuppressionMappingTest, ParsingRespectsOtherWarningOpts) {
 
 #ifdef _WIN32
 // We're only slash-agnostic on windows hosts
-TEST_F(SuppressionMappingTest, ForwardSlashMatchesBothDirections) {
+TEST_F(SuppressionMappingTest, TreatsFilesAsSlashAgnosticOnWindows) {
   llvm::StringLiteral SuppressionMappingFile = R"(
   [unused]
   src:*clang/*
   src:*clang/lib/Sema/*=emit
-  src:*clang/lib\\Sema/foo*)";
+  src:*clang/lib\\Sema/foo*
+  fun:suppress/me)";
   Diags.getDiagnosticOptions().DiagnosticSuppressionMappingsFile = "foo.txt";
   FS->addFile("foo.txt", /*ModificationTime=*/{},
               llvm::MemoryBuffer::getMemBuffer(SuppressionMappingFile));
   clang::ProcessWarningOptions(Diags, Diags.getDiagnosticOptions(), *FS);
   EXPECT_THAT(diags(), IsEmpty());
 
   EXPECT_TRUE(Diags.isSuppressedViaMapping(
-      diag::warn_unused_function, locForFile(R"(clang/lib/Basic/foo.h)")));
+      diag::warn_unused_function, locForFile(R"(clang/lib/Basic/bar.h)")));
   EXPECT_FALSE(Diags.isSuppressedViaMapping(
-      diag::warn_unused_function, locForFile(R"(clang/lib/Sema\bar.h)")));
+      diag::warn_unused_function, locForFile(R"(clang/lib/Sema\baz.h)")));
+
+  // We require a literal backslash before "Sema"
   EXPECT_TRUE(Diags.isSuppressedViaMapping(
       diag::warn_unused_function, locForFile(R"(clang\lib\Sema/foo.h)")));
-  // The third pattern requires a literal backslash before Sema
   EXPECT_FALSE(Diags.isSuppressedViaMapping(
       diag::warn_unused_function, locForFile(R"(clang/lib/Sema/foo.h)")));
 }

llvm/docs/ReleaseNotes.md

@@ -175,8 +175,8 @@ Changes to Sanitizers
 ---------------------
 
 * On windows hosts, the [sanitizer special case list format](https://clang.llvm.org/docs/SanitizerSpecialCaseList.html#format)
-  now treats forward slashes as either a forward or a backslash, to handle
-  paths with mixed unix and windows styles.
+  now treats forward slashes in filenames as matching either a forward or a
+  backslash, to accommodate paths with mixed unix and windows styles.
 
 Other Changes
 -------------

llvm/include/llvm/Support/GlobPattern.h

@@ -35,9 +35,9 @@ namespace llvm {
 ///   expansions are not supported. If \p MaxSubPatterns is empty then
 ///   brace expansions are not supported and characters `{,}` are treated as
 ///   literals.
-/// * If IsSlashAgnostic is passed, `/` matches both unix and windows path
-///   separators: `/` and `\`.
 /// * `\` 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,
@@ -57,12 +57,11 @@ class GlobPattern {
   /// \param MaxSubPatterns if provided limit the number of allowed subpatterns
   ///                       created from expanding braces otherwise disable
   ///                       brace expansion
-  /// \param IsSlashAgnostic whether to treat '/' as matching '\\' as well
   LLVM_ABI static Expected<GlobPattern>
-  create(StringRef Pat, std::optional<size_t> MaxSubPatterns = {},
-         bool IsSlashAgnostic = false);
+  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) const;
+  LLVM_ABI bool match(StringRef S, bool IsSlashAgnostic = false) const;
 
   // Returns true for glob pattern "*". Can be used to avoid expensive
   // preparation/acquisition of the input for match().
@@ -79,11 +78,10 @@ class GlobPattern {
 
   struct SubGlobPattern {
     /// \param Pat the pattern to match against
-    /// \param SlashAgnostic whether to treat '/' as matching '\\' as well
-    LLVM_ABI static Expected<SubGlobPattern> create(StringRef Pat,
-                                                    bool SlashAgnostic);
+    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) const;
+    LLVM_ABI bool match(StringRef S, bool IsSlashAgnostic) const;
     StringRef getPat() const { return StringRef(Pat.data(), Pat.size()); }
 
     // Brackets with their end position and matched bytes.
@@ -93,7 +91,6 @@ class GlobPattern {
     };
     SmallVector<Bracket, 0> Brackets;
     SmallVector<char, 0> Pat;
-    bool IsSlashAgnostic;
   };
   SmallVector<SubGlobPattern, 1> SubGlobs;
 };

llvm/include/llvm/Support/SpecialCaseList.h

@@ -124,8 +124,9 @@ class SpecialCaseList {
     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;
+    // 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;
 
     struct Glob {
       std::string Name;

llvm/lib/Support/GlobPattern.cpp

@@ -132,9 +132,8 @@ parseBraceExpansions(StringRef S, std::optional<size_t> MaxSubPatterns) {
   return std::move(SubPatterns);
 }
 
-Expected<GlobPattern> GlobPattern::create(StringRef S,
-                                          std::optional<size_t> MaxSubPatterns,
-                                          bool IsSlashAgnostic) {
+Expected<GlobPattern>
+GlobPattern::create(StringRef S, std::optional<size_t> MaxSubPatterns) {
   GlobPattern Pat;
 
   // Store the prefix that does not contain any metacharacter.
@@ -148,7 +147,7 @@ Expected<GlobPattern> GlobPattern::create(StringRef S,
   if (auto Err = parseBraceExpansions(S, MaxSubPatterns).moveInto(SubPats))
     return std::move(Err);
   for (StringRef SubPat : SubPats) {
-    auto SubGlobOrErr = SubGlobPattern::create(SubPat, IsSlashAgnostic);
+    auto SubGlobOrErr = SubGlobPattern::create(SubPat);
     if (!SubGlobOrErr)
       return SubGlobOrErr.takeError();
     Pat.SubGlobs.push_back(*SubGlobOrErr);
@@ -158,9 +157,8 @@ Expected<GlobPattern> GlobPattern::create(StringRef S,
 }
 
 Expected<GlobPattern::SubGlobPattern>
-GlobPattern::SubGlobPattern::create(StringRef S, bool SlashAgnostic) {
+GlobPattern::SubGlobPattern::create(StringRef S) {
   SubGlobPattern Pat;
-  Pat.IsSlashAgnostic = SlashAgnostic;
 
   // Parse brackets.
   Pat.Pat.assign(S.begin(), S.end());
@@ -192,21 +190,22 @@ GlobPattern::SubGlobPattern::create(StringRef S, bool SlashAgnostic) {
   return Pat;
 }
 
-bool GlobPattern::match(StringRef S) const {
+bool GlobPattern::match(StringRef S, bool IsSlashAgnostic) const {
   if (!S.consume_front(Prefix))
     return false;
   if (SubGlobs.empty() && S.empty())
     return true;
   for (auto &Glob : SubGlobs)
-    if (Glob.match(S))
+    if (Glob.match(S, IsSlashAgnostic))
       return true;
   return false;
 }
 
 // Factor the pattern into segments split by '*'. The segment is matched
-// sequentianlly by finding the first occurrence past the end of the previous
+// sequentially by finding the first occurrence past the end of the previous
 // match.
-bool GlobPattern::SubGlobPattern::match(StringRef Str) const {
+bool GlobPattern::SubGlobPattern::match(StringRef Str,
+                                        bool IsSlashAgnostic) 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();
@@ -233,7 +232,7 @@ bool GlobPattern::SubGlobPattern::match(StringRef Str) const {
         ++S;
         continue;
       }
-    } else if (IsSlashAgnostic && *P == '/' && (*S == '/' || *S == '\\')) {
+    } else if (IsSlashAgnostic && *P == '/' && *S == '\\') {
       ++P;
       ++S;
       continue;

llvm/lib/Support/SpecialCaseList.cpp

@@ -17,9 +17,8 @@
 #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 "llvm/TargetParser/Host.h"
-#include "llvm/TargetParser/Triple.h"
 #include <stdio.h>
 #include <string>
 #include <system_error>
@@ -59,21 +58,22 @@ Error SpecialCaseList::Matcher::insert(StringRef Pattern, unsigned LineNumber,
   auto Glob = std::make_unique<Matcher::Glob>();
   Glob->Name = Pattern.str();
   Glob->LineNo = LineNumber;
-  // Backslashes are valid in posix-style filenames.
-  bool IsSlashAgnostic = Triple(sys::getDefaultTargetTriple()).isOSWindows();
   // 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,
-                                     /*IsSlashAgnostic=*/IsSlashAgnostic)
+  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 {
+unsigned SpecialCaseList::Matcher::match(StringRef Query,
+                                         bool IsFilename) const {
+  static bool HaveWindowsPathStyle =
+      llvm::sys::path::is_style_windows(llvm::sys::path::Style::native);
   for (const auto &Glob : reverse(Globs))
-    if (Glob->Pattern.match(Query))
+    if (Glob->Pattern.match(
+            Query, /*IsSlashAgnostic=*/(HaveWindowsPathStyle && IsFilename)))
       return Glob->LineNo;
   for (const auto &[Regex, LineNumber] : reverse(RegExes))
     if (Regex->match(Query))
@@ -223,7 +223,8 @@ 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)) {
+    bool IsFilename = Prefix == "src" || Prefix == "mainfile";
+    if (S.SectionMatcher->match(Section, IsFilename)) {
       unsigned Blame = inSectionBlame(S.Entries, Prefix, Query, Category);
       if (Blame)
         return {S.FileIdx, Blame};
@@ -242,7 +243,8 @@ unsigned SpecialCaseList::inSectionBlame(const SectionEntries &Entries,
   if (II == I->second.end())
     return 0;
 
-  return II->getValue().match(Query);
+  bool IsFilename = Prefix == "src" || Prefix == "mainfile";
+  return II->getValue().match(Query, IsFilename);
 }
 
 } // namespace llvm