Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
56faefd
[Clang] Silently ignore unknown warnings in `--warning-suppression-ma…
jyknight Jan 23, 2025
c67cf0b
clang-format.
jyknight Jan 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions clang/lib/Basic/Diagnostic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// suppressions. This allows use of a single suppression file on multiple
// versions of clang.
continue;
}
for (diag::kind Diag : GroupDiags)
Expand Down
4 changes: 4 additions & 0 deletions clang/test/Misc/Inputs/suppression-mapping.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:*
4 changes: 4 additions & 0 deletions clang/tools/driver/driver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,10 @@ 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);
Expand Down
3 changes: 1 addition & 2 deletions clang/unittests/Basic/DiagnosticTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Loading