[clang-tidy][clang][diagnostics] Unify system header/macro supression (#193774)

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 exposes this more refined logic under the name
`DiagnosticIDs::shouldSuppressAsSystemWarning` and 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'.

The changes in `DiagnosticIDs.{cpp,h}` are NFC, they do not affect the
behavior of 'clang'.

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.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

@@ -312,6 +312,10 @@ std::string ClangTidyContext::getCheckName(unsigned DiagnosticID) const {
   return "";
 }
 
+bool ClangTidyContext::isCompilerDiagnostic(unsigned DiagnosticID) const {
+  return !CheckNamesByDiagnosticID.contains(DiagnosticID);
+}
+
 ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer(
     ClangTidyContext &Ctx, DiagnosticsEngine *ExternalDiagEngine,
     bool RemoveIncompatibleErrors, bool GetFixesFromNotes,
@@ -470,7 +474,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 +571,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 +580,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

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h

@@ -146,6 +146,10 @@ public:
   /// 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 @@ private:
   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);

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
 ^^^^^^^^^^
 

clang-tools-extra/test/clang-tidy/infrastructure/Inputs/system-headers/mock_cstddef.h

@@ -0,0 +1 @@
+#define offsetof(t, d) __builtin_offsetof(t, d)

clang-tools-extra/test/clang-tidy/infrastructure/system-macro-diagnostic.cpp

@@ -0,0 +1,28 @@
+// 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
+// '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]
+}

clang/include/clang/Basic/DiagnosticIDs.h

@@ -488,6 +488,15 @@ public:
   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.

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 {