[Clang] Silently ignore unknown warnings in --warning-suppression-mappings

[jyknight]

This allows the same file to be used on multiple Clang versions, without generating output spam.

Also disable parsing of the suppressions file in the Driver, where it's not needed. This was previously causing the diagnostics to be emitted twice, as the file was being parsed twice.

I'll also note that if we do ever wish to emit non-fatal diagnostics from parsing this file, it'll need more: the code deleted here emitted warnings which were not possible for a user to disable, since the suppression file is parsed before the diagnostic state has been setup.

[llvmbot]

@llvm/pr-subscribers-clang

Author: James Y Knight (jyknight)

Changes

This allows the same file to be used on multiple Clang versions, without generating output spam.

Also disable parsing of the suppressions file in the Driver, where it's not needed. This was previously causing the diagnostics to be emitted twice, as the file was being parsed twice.

I'll also note that if we do ever wish to emit non-fatal diagnostics from parsing this file, it'll need more: the code deleted here emitted warnings which were not possible for a user to disable, since the suppression file is parsed before the diagnostic state has been setup.

---

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

4 Files Affected:

• (modified) clang/lib/Basic/Diagnostic.cpp (+3-5)
• (modified) clang/test/Misc/Inputs/suppression-mapping.txt (+4)
• (modified) clang/tools/driver/driver.cpp (+3)
• (modified) clang/unittests/Basic/DiagnosticTest.cpp (+1-2)

diff --git a/clang/lib/Basic/Diagnostic.cpp b/clang/lib/Basic/Diagnostic.cpp
index ae71758bc81e03..55efd5ffafcece 100644
--- a/clang/lib/Basic/Diagnostic.cpp
+++ b/clang/lib/Basic/Diagnostic.cpp
@@ -547,11 +547,9 @@ void WarningsSpecialCaseList::processSections(DiagnosticsEngine &Diags) {
     StringRef DiagGroup = SectionEntry->getKey();
     if (Diags.getDiagnosticIDs()->getDiagnosticsInGroup(
             WarningFlavor, DiagGroup, GroupDiags)) {
-      StringRef Suggestion =
-          DiagnosticIDs::getNearestOption(WarningFlavor, DiagGroup);
-      Diags.Report(diag::warn_unknown_diag_option)
-          << static_cast<unsigned>(WarningFlavor) << DiagGroup
-          << !Suggestion.empty() << Suggestion;
+      // If a diagnostic group name is unknown, simply ignore the
+      // suppressions. This allows use of a single suppression file on multiple
+      // versions of clang.
       continue;
     }
     for (diag::kind Diag : GroupDiags)
diff --git a/clang/test/Misc/Inputs/suppression-mapping.txt b/clang/test/Misc/Inputs/suppression-mapping.txt
index abe4fde0c265d5..cea8c50daee1c5 100644
--- a/clang/test/Misc/Inputs/suppression-mapping.txt
+++ b/clang/test/Misc/Inputs/suppression-mapping.txt
@@ -11,3 +11,7 @@ src:*foo/*=emit
 [format=2]
 src:*
 src:*foo/*=emit
+
+# A warning group that clang doesn't know about should be silently ignored.
+[barglegunk]
+src:*
diff --git a/clang/tools/driver/driver.cpp b/clang/tools/driver/driver.cpp
index 74923247b7ee16..c68367c177910c 100644
--- a/clang/tools/driver/driver.cpp
+++ b/clang/tools/driver/driver.cpp
@@ -319,6 +319,9 @@ int clang_main(int Argc, char **Argv, const llvm::ToolContext &ToolContext) {
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts =
       CreateAndPopulateDiagOpts(Args);
 
+  // The _driver_ (vs cc1) diagnostics engine shouldn't bother to load a suppression file.
+  DiagOpts->DiagnosticSuppressionMappingsFile = "";
+
   TextDiagnosticPrinter *DiagClient
     = new TextDiagnosticPrinter(llvm::errs(), &*DiagOpts);
   FixupDiagPrefixExeName(DiagClient, ProgName);
diff --git a/clang/unittests/Basic/DiagnosticTest.cpp b/clang/unittests/Basic/DiagnosticTest.cpp
index e03d9a464df7f9..6c431ed81c4919 100644
--- a/clang/unittests/Basic/DiagnosticTest.cpp
+++ b/clang/unittests/Basic/DiagnosticTest.cpp
@@ -253,8 +253,7 @@ TEST_F(SuppressionMappingTest, UnknownDiagName) {
   FS->addFile("foo.txt", /*ModificationTime=*/{},
               llvm::MemoryBuffer::getMemBuffer("[non-existing-warning]"));
   clang::ProcessWarningOptions(Diags, Diags.getDiagnosticOptions(), *FS);
-  EXPECT_THAT(diags(), ElementsAre(WithMessage(
-                           "unknown warning option 'non-existing-warning'")));
+  EXPECT_THAT(diags(), IsEmpty());
 }
 
 TEST_F(SuppressionMappingTest, SuppressesGroup) {

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[kadircet]

it's intentional to emit these warnings so can we keep that but rather make sure we do respect rest of the warning options?

we can do that by moving https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Warnings.cpp#L76-L84 past processing of other warning flags.

in addition to that, we should propagate ReportDiags into DiagnosticsEngine::setDiagSuppressionMapping and not emit warnings when it's set to false (to prevent double emitting in driver & cc1).

this is more of a FR, but we should also use source-manager and attach source-location to these warnings.

--

I am happy to do these changes myself, as they're more involved then what's in this PR currently. PLMK if you'd prefer that.

[jyknight]

I realize it was intentional to emit unknown-warning diagnostics, but I really think we should not. Yes, that could catch a typo in the suppressions file, but you could also typo the filenames in the same way without notice, so I don't find that terribly persuasive as a use-case. Because this is a suppression file, the primary notice that you've made a mistake is that the diagnostic you expected to suppress did not get suppressed.

On the other hand, not emitting unknown-warning diagnostic is particularly useful here, as it allows the suppressions file for a given codebase to be easily used across multiple compilers. It allows users to easily write a file which suppresses some instance of a newly-introduced diagnostic, yet doesn't complain when the compiler doesn't yet support the suppressed-diagnostic. Notably, on the version which doesn't recognize the flag, generally the suppression is not necessary, since it doesn't know how to emit the diagnostic anyways.

And there's not the ability to guard with #if __has_warning like you can if adding a suppression via editing the source file.

make sure we do respect rest of the warning options?

But, yes, if we do continue to emit an unknown-warning diagnostic, it needs to honor -Werror and -Wno-unknown-warning-option.

in addition to that, we should propagate ReportDiags into DiagnosticsEngine::setDiagSuppressionMapping and not emit warnings when it's set to false (to prevent double emitting in driver & cc1).

Why parse the suppression file in the driver? Even if we do check ReportDiags, isn't it just wasted work to read it at all?

I am happy to do these changes myself, as they're more involved then what's in this PR currently.

I'd be happy to have you do so! (Even if a bit less happy than going with the simpler proposal, here).

[kadircet]

I realize it was intentional to emit unknown-warning diagnostics, but I really think we should not.

i still feel a little bit uneasy about leaving the user in the dark, when they're trying to use the suppression mapping and it silently doesn't work (even if it's due to a configuration error). so i'd rather punt on silently ignoring, until concerns like version skew is really biting us in practice.

Most of the projects that maintain their own toolchain already needs to put effort into fiddling with warning flags as they update their toolchain hence I don't think we're creating any new maintenance surface here.

Why parse the suppression file in the driver? Even if we do check ReportDiags, isn't it just wasted work to read it at all?

sorry my comment was a little misleading. i agree that we shouldn't try parsing this in driver, there's simply no need. but we should still respect ReportDiags in the implementation.

I'd be happy to have you do so! (Even if a bit less happy than going with the simpler proposal, here).

thanks! i'll put together a patch soon

[jyknight]

Abandoning in favor of kadircet's #125722 and #125714.