From ab08fbd92cb7557c198be1f9de0b59b23f8e92e1 Mon Sep 17 00:00:00 2001 From: Vitaly Buka Date: Mon, 6 Oct 2025 16:01:34 -0700 Subject: [PATCH] [clang] Switch warning suppression multi-match rule to "last match takes precedence" The current "longest match takes precedence" rule for warning suppression mappings can be confusing, especially in long suppression files where tracking the length relationship between globs is difficult. For example, with the following rules, it's not immediately obvious why the first one should currently take precedence: ``` src:*test/* src:*lld/*=emit ``` This commit changes the multi-match behavior so the last match takes precedence. This rule is easier to understand and consistent with the approach used by sanitizers, simplifying the mechanism by providing a uniform experience across different tools. This is potentially breaking, but very unlikely. An investigation of known uses showed they do not rely on the length. Reviewers: thurstond, kadircet, fmayer Pull Request: https://github.com/llvm/llvm-project/pull/162237 --- clang/docs/ReleaseNotes.rst | 2 ++ clang/docs/WarningSuppressionMappings.rst | 4 ++-- clang/include/clang/Basic/Diagnostic.h | 2 +- clang/lib/Basic/Diagnostic.cpp | 14 +++++--------- clang/unittests/Basic/DiagnosticTest.cpp | 8 +++----- 5 files changed, 13 insertions(+), 17 deletions(-) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 1b7896ec8711..ad54872913d5 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. - Trailing null statements in GNU statement expressions are no longer ignored by Clang; they now result in a void type. Clang previously matched GCC's behavior, which was recently clarified to be incorrect. diff --git a/clang/docs/WarningSuppressionMappings.rst b/clang/docs/WarningSuppressionMappings.rst index d96341ac6e56..d8af856f64ef 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 e540040ddc52..c6e931d0c951 100644 --- a/clang/include/clang/Basic/Diagnostic.h +++ b/clang/include/clang/Basic/Diagnostic.h @@ -971,7 +971,7 @@ public: /// 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 5e9da245e2b4..4802478c379b 100644 --- a/clang/lib/Basic/Diagnostic.cpp +++ b/clang/lib/Basic/Diagnostic.cpp @@ -525,8 +525,7 @@ std::unique_ptr WarningsSpecialCaseList::create(const llvm::MemoryBuffer &Input, std::string &Err) { auto WarningSuppressionList = std::make_unique(); - if (!WarningSuppressionList->createInternal(&Input, Err, - /*OrderBySize=*/true)) + if (!WarningSuppressionList->createInternal(&Input, Err)) return nullptr; return WarningSuppressionList; } @@ -588,15 +587,12 @@ bool WarningsSpecialCaseList::isDiagSuppressed(diag::kind DiagId, StringRef F = llvm::sys::path::remove_leading_dotslash(PLoc.getFilename()); - StringRef LongestSup = DiagSection->getLongestMatch("src", F, ""); - if (LongestSup.empty()) + unsigned LastSup = DiagSection->getLastMatch("src", F, ""); + if (LastSup == 0) return false; - StringRef LongestEmit = DiagSection->getLongestMatch("src", F, "emit"); - if (LongestEmit.empty()) - return true; - - return LongestSup.size() > LongestEmit.size(); + unsigned LastEmit = DiagSection->getLastMatch("src", F, "emit"); + return LastSup > LastEmit; } bool DiagnosticsEngine::isSuppressedViaMapping(diag::kind DiagId, diff --git a/clang/unittests/Basic/DiagnosticTest.cpp b/clang/unittests/Basic/DiagnosticTest.cpp index de090864e509..5492146f40fa 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) {