https://github.com/NagyDonat updated https://github.com/llvm/llvm-project/pull/193774
From bb01de2de85720c9b590468155a7c0da9f08db7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 23 Apr 2026 15:20:22 +0200 Subject: [PATCH 1/6] [NFC][clang-tidy] Add test 'system-macro-diagnostic.cpp' This testcase describes the behavior that would be correct, so currently it fails, but the other commits in this PR will stabilize it. --- .../Inputs/system-headers/mock_cstddef.h | 1 + .../system-macro-diagnostic.cpp | 23 +++++++++++++++++++ 2 files changed, 24 insertions(+) create mode 100644 clang-tools-extra/test/clang-tidy/infrastructure/Inputs/system-headers/mock_cstddef.h create mode 100644 clang-tools-extra/test/clang-tidy/infrastructure/system-macro-diagnostic.cpp diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/Inputs/system-headers/mock_cstddef.h b/clang-tools-extra/test/clang-tidy/infrastructure/Inputs/system-headers/mock_cstddef.h new file mode 100644 index 0000000000000..641e3e7f072ea --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/infrastructure/Inputs/system-headers/mock_cstddef.h @@ -0,0 +1 @@ +#define offsetof(t, d) __builtin_offsetof(t, d) diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/system-macro-diagnostic.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/system-macro-diagnostic.cpp new file mode 100644 index 0000000000000..0e88f6f0463c7 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/infrastructure/system-macro-diagnostic.cpp @@ -0,0 +1,23 @@ +// RUN: clang-tidy -checks='-*,clang-diagnostic-invalid-offsetof,concurrency-mt-unsafe' -header-filter='.*' -system-headers=true %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-SYSTEM-HEADERS %s +// RUN: clang-tidy -checks='-*,clang-diagnostic-invalid-offsetof,concurrency-mt-unsafe' -header-filter='.*' -system-headers=false %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS %s +// FIXME: The check 'concurrency-mt-unsafe' is completely unrelated to this +// test, it is only added to the RUN lines because Clang-Tidy aborts the +// analysis with "Error: no checks enabled." if all the enabled checks are +// 'clang-diagnostic-*' checks (i.e. compiler warnings). +// Once GH#192713 is resolved, remove 'concurrency-mt-unsafe'. + +#include <mock_cstddef.h> + +struct D { + virtual void f() {} + virtual ~D() {} + int i; +}; + +int main() { + // Previously Clang-Tidy was suppressing this -Winvalid-offsetof report + // because the error location is in a system macro (namely, 'offsetof'). + (void) offsetof(D, i); + // CHECK-SYSTEM-HEADERS: :[[@LINE-1]]:10: warning: 'offsetof' on non-standard-layout type 'D' [clang-diagnostic-invalid-offsetof] + // CHECK-NO-SYSTEM-HEADERS: :[[@LINE-2]]:10: warning: 'offsetof' on non-standard-layout type 'D' [clang-diagnostic-invalid-offsetof] +} From 64898864dcaec3b00f78de4190f268007e703109 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 23 Apr 2026 15:42:41 +0200 Subject: [PATCH 2/6] [NFC][diagnostics] Introduce DiagnosticIDs::shouldSuppressAsSystemWarning This change introduces a public method for accessing the parts of `getDiagnosticSeverity()` that were responsible for suppressing reports that are in system headers or system macros. This new method will be used in Clang-Tidy to ensure that this sort of diagnostic suppression behaves consistently between the clang compiler frontend and Clang-Tidy. (Clang-Tidy has its own complex logic for suppressing reports and adjusting their severity, so I don't think that it is feasible to reuse the full `getDiagnosticSeverity` in Clang-Tidy, but this aspect should behave the same way.) --- clang/include/clang/Basic/DiagnosticIDs.h | 9 +++++ clang/lib/Basic/DiagnosticIDs.cpp | 48 ++++++++++++++--------- 2 files changed, 39 insertions(+), 18 deletions(-) diff --git a/clang/include/clang/Basic/DiagnosticIDs.h b/clang/include/clang/Basic/DiagnosticIDs.h index 09e2d12dd040e..47f63933e1e2f 100644 --- a/clang/include/clang/Basic/DiagnosticIDs.h +++ b/clang/include/clang/Basic/DiagnosticIDs.h @@ -488,6 +488,15 @@ class DiagnosticIDs : public RefCountedBase<DiagnosticIDs> { static unsigned getCXXCompatDiagId(const LangOptions &LangOpts, unsigned CompatDiagId); + /// Return true if either of the following two conditions hold: + /// 1. \p Loc is in a system header and the diagnostic kind \p DiagID does + /// not have the property 'ShowInSystemHeader'. + /// 2. \p Loc is in the expansion of a macro defined in a system header and + /// the diagnostic kind \p DiagID does not have the property + /// 'ShowInSystemMacro'. + bool shouldSuppressAsSystemWarning(unsigned DiagID, SourceLocation Loc, + const DiagnosticsEngine &Diag) const; + private: /// Classify the specified diagnostic ID into a Level, consumable by /// the DiagnosticClient. diff --git a/clang/lib/Basic/DiagnosticIDs.cpp b/clang/lib/Basic/DiagnosticIDs.cpp index 0aa543c8bcb1d..d9c5e4082c1a7 100644 --- a/clang/lib/Basic/DiagnosticIDs.cpp +++ b/clang/lib/Basic/DiagnosticIDs.cpp @@ -620,18 +620,33 @@ DiagnosticIDs::getDiagnosticSeverity(unsigned DiagID, SourceLocation Loc, if (!Diag.hasSourceManager()) return Result; - const auto &SM = Diag.getSourceManager(); - // If we are in a system header, we ignore it. We look at the diagnostic class - // because we also want to ignore extensions and warnings in -Werror and - // -pedantic-errors modes, which *map* warnings/extensions to errors. - // // We check both the location-specific state and the ForceSystemWarnings // override. In some cases (like template instantiations from system modules), // the location-specific state might have suppression enabled, but the // engine might have an override (e.g. AllowWarningInSystemHeaders) to show // the warning. if (State->SuppressSystemWarnings && !Diag.getForceSystemWarnings() && - Loc.isValid() && SM.isInSystemHeader(SM.getExpansionLoc(Loc))) { + shouldSuppressAsSystemWarning(DiagID, Loc, Diag)) { + return diag::Severity::Ignored; + } + + // Clang-diagnostics pragmas always take precedence over suppression mapping. + if (!Mapping.isPragma() && Diag.isSuppressedViaMapping(DiagID, Loc)) + return diag::Severity::Ignored; + + return Result; +} + +bool DiagnosticIDs::shouldSuppressAsSystemWarning( + unsigned DiagID, SourceLocation Loc, const DiagnosticsEngine &Diag) const { + if (!Loc.isValid()) + return false; + + bool IsCustomDiag = DiagnosticIDs::IsCustomDiag(DiagID); + const auto &SM = Diag.getSourceManager(); + + // If we are in a system header, we ignore it. + if (SM.isInSystemHeader(SM.getExpansionLoc(Loc))) { bool ShowInSystemHeader = true; if (IsCustomDiag) ShowInSystemHeader = @@ -640,25 +655,22 @@ DiagnosticIDs::getDiagnosticSeverity(unsigned DiagID, SourceLocation Loc, ShowInSystemHeader = Rec->WarnShowInSystemHeader; if (!ShowInSystemHeader) - return diag::Severity::Ignored; + return true; } - // We also ignore warnings due to system macros. As above, we respect the - // ForceSystemWarnings override. - if (State->SuppressSystemWarnings && !Diag.getForceSystemWarnings() && - Loc.isValid()) { - + // We also ignore warnings due to system macros. + if (Loc.isValid()) { bool ShowInSystemMacro = true; + + // FIXME: Respect the "show in system macro" information in the + // CustomDiagInfo (which is currently ignored). + if (const StaticDiagInfoRec *Rec = GetDiagInfo(DiagID)) ShowInSystemMacro = Rec->WarnShowInSystemMacro; if (!ShowInSystemMacro && SM.isInSystemMacro(Loc)) - return diag::Severity::Ignored; + return true; } - // Clang-diagnostics pragmas always take precedence over suppression mapping. - if (!Mapping.isPragma() && Diag.isSuppressedViaMapping(DiagID, Loc)) - return diag::Severity::Ignored; - - return Result; + return false; } DiagnosticIDs::Class DiagnosticIDs::getDiagClass(unsigned DiagID) const { From 43acd490383d910bceac74a2742dd7f446a90997 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 23 Apr 2026 15:52:55 +0200 Subject: [PATCH 3/6] [clang-tidy] Adjust suppression of 'clang-diagnostic-*' Before this change, if Clang-Tidy ran with `--system-headers=false` (the default config), it suppressed all reports where the location was in a system header or in the expansion of a macro from a system header. This was problematic because e.g. practically all reports of 'clang-diagnostic-invalid-offsetof' (a/k/a `-Winvalid-offsetof`) are in the expansion of a system macro (namely 'offsetof'), so Clang-Tidy was unable to display these reports. In the clang frontend there was a more refined logic: certain diagnostics have the 'ShowInSystemHeader' and/or 'ShowInSystemMacro' properties which ensure that they are printed (by the clang frontend) even if the compiler wouldn't otherwise print diagnostics from system headers/macros. This commit also invokes this logic from Clang-Tidy to ensure that the `--system-headers=false` flag of Clang-Tidy cannot suppress 'clang-diagnostic-*' reports that are "protected by" the properties 'ShowInSystemHeader' or 'ShowInSystemMacro'. This commit does not influence the reporting of "native" tidy reports and 'clang-analyzer-*' reports. However, in the future it would be possible to extend this logic and assign 'ShowInSystemHeader' and/or 'ShowInSystemMacro' properties to clang-tidy checks that need them. --- .../ClangTidyDiagnosticConsumer.cpp | 23 +++++++++++++++---- .../clang-tidy/ClangTidyDiagnosticConsumer.h | 9 ++++++-- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp index f7232645a329c..19828c1926591 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp @@ -312,6 +312,12 @@ std::string ClangTidyContext::getCheckName(unsigned DiagnosticID) const { return ""; } +bool ClangTidyContext::isCompilerDiagnostic(unsigned DiagnosticID) const { + const llvm::DenseMap<unsigned, std::string>::const_iterator I = + CheckNamesByDiagnosticID.find(DiagnosticID); + return I == CheckNamesByDiagnosticID.end(); +} + ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer( ClangTidyContext &Ctx, DiagnosticsEngine *ExternalDiagEngine, bool RemoveIncompatibleErrors, bool GetFixesFromNotes, @@ -470,7 +476,7 @@ void ClangTidyDiagnosticConsumer::HandleDiagnostic( } if (Info.hasSourceManager()) - checkFilters(Info.getLocation(), Info.getSourceManager()); + checkFilters(Info.getLocation(), Info.getID(), Info.getSourceManager()); for (const auto &Error : SuppressionErrors) Context.diag(Error); @@ -567,6 +573,7 @@ void ClangTidyDiagnosticConsumer::forwardDiagnostic(const Diagnostic &Info) { } void ClangTidyDiagnosticConsumer::checkFilters(SourceLocation Location, + unsigned DiagnosticID, const SourceManager &Sources) { // Invalid location may mean a diagnostic in a command line, don't skip these. if (!Location.isValid()) { @@ -575,9 +582,17 @@ void ClangTidyDiagnosticConsumer::checkFilters(SourceLocation Location, return; } - if (!Context.getOptions().SystemHeaders.value_or(false) && - (Sources.isInSystemHeader(Location) || Sources.isInSystemMacro(Location))) - return; + if (!Context.getOptions().SystemHeaders.value_or(false)) { + if (Context.isCompilerDiagnostic(DiagnosticID)) { + if (Context.DiagEngine->getDiagnosticIDs()->shouldSuppressAsSystemWarning( + DiagnosticID, Location, *Context.DiagEngine)) + return; + } else { + if (Sources.isInSystemHeader(Location) || + Sources.isInSystemMacro(Location)) + return; + } + } // FIXME: We start with a conservative approach here, but the actual type of // location needed depends on the check (in particular, where this check wants diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h index 8de5778dfefb0..c76f58bc4cc86 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h @@ -146,6 +146,10 @@ class ClangTidyContext { /// diagnostic ID. std::string getCheckName(unsigned DiagnosticID) const; + /// Returns true if this clang-tidy check is in fact a compiler warning + /// exposed as a 'clang-diagnostic-*' check. + bool isCompilerDiagnostic(unsigned DiagnosticID) const; + /// Returns \c true if the check is enabled for the \c CurrentFile. /// /// The \c CurrentFile can be changed using \c setCurrentFile. @@ -319,8 +323,9 @@ class ClangTidyDiagnosticConsumer : public DiagnosticConsumer { llvm::Regex *getExcludeHeaderFilter(); /// Updates \c LastErrorRelatesToUserCode and LastErrorPassesLineFilter - /// according to the diagnostic \p Location. - void checkFilters(SourceLocation Location, const SourceManager &Sources); + /// according to the diagnostic kind \p DiagnosticID and the \p Location. + void checkFilters(SourceLocation Location, unsigned DiagnosticID, + const SourceManager &Sources); bool passesLineFilter(StringRef FileName, unsigned LineNumber) const; void forwardDiagnostic(const Diagnostic &Info); From 70c4c1c2c2e524f48696fed4c3bd63a76e72ebf6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Fri, 24 Apr 2026 11:05:26 +0200 Subject: [PATCH 4/6] [NFC] Validate that disabling 'clang-diagnostic-*' still works --- .../clang-tidy/infrastructure/system-macro-diagnostic.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/system-macro-diagnostic.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/system-macro-diagnostic.cpp index 0e88f6f0463c7..ac098b3449bce 100644 --- a/clang-tools-extra/test/clang-tidy/infrastructure/system-macro-diagnostic.cpp +++ b/clang-tools-extra/test/clang-tidy/infrastructure/system-macro-diagnostic.cpp @@ -1,5 +1,10 @@ // RUN: clang-tidy -checks='-*,clang-diagnostic-invalid-offsetof,concurrency-mt-unsafe' -header-filter='.*' -system-headers=true %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-SYSTEM-HEADERS %s // RUN: clang-tidy -checks='-*,clang-diagnostic-invalid-offsetof,concurrency-mt-unsafe' -header-filter='.*' -system-headers=false %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS %s + +// Validate that we don't get the diagnostics when the 'clang-diagnostic-*' +// check is disabled: +// RUN: clang-tidy -checks='-*,concurrency-mt-unsafe' -header-filter='.*' -system-headers=true %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -implicit-check-not='{{warning:|error:}}' %s + // FIXME: The check 'concurrency-mt-unsafe' is completely unrelated to this // test, it is only added to the RUN lines because Clang-Tidy aborts the // analysis with "Error: no checks enabled." if all the enabled checks are From e514307a6211bcbc7c16cb6b925b5e88e89028d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Fri, 24 Apr 2026 11:06:28 +0200 Subject: [PATCH 5/6] Use 'contains' to simplify code Co-authored-by: Victor Chernyakin <[email protected]> --- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp index 19828c1926591..88d0a433bc7fb 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp @@ -313,9 +313,7 @@ std::string ClangTidyContext::getCheckName(unsigned DiagnosticID) const { } bool ClangTidyContext::isCompilerDiagnostic(unsigned DiagnosticID) const { - const llvm::DenseMap<unsigned, std::string>::const_iterator I = - CheckNamesByDiagnosticID.find(DiagnosticID); - return I == CheckNamesByDiagnosticID.end(); + return !CheckNamesByDiagnosticID.contains(DiagnosticID); } ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer( From 90d6b4f7a496bcd78f78079cab64b97f16fd8633 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 11 May 2026 17:53:24 +0200 Subject: [PATCH 6/6] Mention change in the release notes --- clang-tools-extra/docs/ReleaseNotes.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 241ae52cffdd2..1b469cad4eb9c 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -163,6 +163,13 @@ Improvements to clang-tidy - Improved :program:`clang-tidy` ``-store-check-profile`` by generating valid JSON when the source file path contains characters that require JSON escaping. +- Ensured that :program:`clang-tidy` and the clang compiler uses the same logic + for the suppression of compiler diagnostics in system headers and expansions + of macros defined in system headers. Previously the default setting of tidy + overzealously suppressed some diagnostics that would have been emitted by the + compiler. (E.g. tidy suppressed many ``clang-diagnostic-invalid-offsetof`` + reports because they usually occur in expansion of the macro ``offsetof``.) + New checks ^^^^^^^^^^ _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
