Skip to content

[clang] Introduce diagnostics suppression mappings #112517

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.

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

Merged
merged 11 commits into from
Nov 12, 2024

Conversation

kadircet
Copy link
Member

This implements
https://discourse.llvm.org/t/rfc-add-support-for-controlling-diagnostics-severities-at-file-level-granularity-through-command-line/81292.

Users now can suppress warnings for certain headers by providing a
mapping with globs, a sample file looks like:

[unused]
src:*
src:*clang/*=emit

This will suppress warnings from -Wunused group in all files that
aren't under clang/ directory. This mapping file can be passed to
clang via --warning-suppression-mappings=foo.txt.

At a high level, mapping file is stored in DiagnosticOptions and then
processed with rest of the warning flags when creating a
DiagnosticsEngine. This is a functor that uses SpecialCaseLists
underneath to match against globs coming from the mappings file.

This implies processing warning options now performs IO, relevant
interfaces are updated to take in a VFS, falling back to RealFileSystem
when one is not available.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang-tools-extra clang-tidy clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules llvm:support labels Oct 16, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 16, 2024

@llvm/pr-subscribers-llvm-support
@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: kadir çetinkaya (kadircet)

Changes

This implements
https://discourse.llvm.org/t/rfc-add-support-for-controlling-diagnostics-severities-at-file-level-granularity-through-command-line/81292.

Users now can suppress warnings for certain headers by providing a
mapping with globs, a sample file looks like:

[unused]
src:*
src:*clang/*=emit

This will suppress warnings from -Wunused group in all files that
aren't under clang/ directory. This mapping file can be passed to
clang via --warning-suppression-mappings=foo.txt.

At a high level, mapping file is stored in DiagnosticOptions and then
processed with rest of the warning flags when creating a
DiagnosticsEngine. This is a functor that uses SpecialCaseLists
underneath to match against globs coming from the mappings file.

This implies processing warning options now performs IO, relevant
interfaces are updated to take in a VFS, falling back to RealFileSystem
when one is not available.


Patch is 36.96 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/112517.diff

23 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp (+2-1)
  • (modified) clang/include/clang/Basic/Diagnostic.h (+14-1)
  • (modified) clang/include/clang/Basic/DiagnosticDriverKinds.td (+3)
  • (modified) clang/include/clang/Basic/DiagnosticOptions.h (+3)
  • (modified) clang/include/clang/Driver/Options.td (+5)
  • (modified) clang/include/clang/Frontend/CompilerInstance.h (+5-5)
  • (modified) clang/lib/Basic/Diagnostic.cpp (+12)
  • (modified) clang/lib/Basic/DiagnosticIDs.cpp (+22-5)
  • (modified) clang/lib/Basic/Warnings.cpp (+107)
  • (modified) clang/lib/Driver/ToolChains/Clang.cpp (+7)
  • (modified) clang/lib/Frontend/ASTUnit.cpp (+9-6)
  • (modified) clang/lib/Frontend/CompilerInstance.cpp (+13-8)
  • (modified) clang/lib/Frontend/CompilerInvocation.cpp (+8)
  • (modified) clang/lib/Frontend/PrecompiledPreamble.cpp (+1-1)
  • (modified) clang/lib/Interpreter/CodeCompletion.cpp (+2-2)
  • (modified) clang/lib/Serialization/ASTReader.cpp (+3-1)
  • (added) clang/test/Misc/Inputs/suppression-mapping.txt (+4)
  • (added) clang/test/Misc/warning-suppression-mappings.cpp (+16)
  • (modified) clang/tools/driver/cc1gen_reproducer_main.cpp (+4-3)
  • (modified) clang/tools/driver/driver.cpp (+5-2)
  • (modified) clang/unittests/Basic/DiagnosticTest.cpp (+164)
  • (modified) clang/unittests/Frontend/CompilerInvocationTest.cpp (+10)
  • (modified) llvm/include/llvm/Support/SpecialCaseList.h (-1)
diff --git a/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp b/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
index fef086c5a99d86..4c34f9ea122d9e 100644
--- a/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
+++ b/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
@@ -81,7 +81,8 @@ ExpandModularHeadersPPCallbacks::ExpandModularHeadersPPCallbacks(
   Diags.setSourceManager(&Sources);
   // FIXME: Investigate whatever is there better way to initialize DiagEngine
   // or whatever DiagEngine can be shared by multiple preprocessors
-  ProcessWarningOptions(Diags, Compiler.getDiagnosticOpts());
+  ProcessWarningOptions(Diags, Compiler.getDiagnosticOpts(),
+                        Compiler.getVirtualFileSystem());
 
   LangOpts.Modules = false;
 
diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h
index 3b1efdb12824c7..79b7545e92d6da 100644
--- a/clang/include/clang/Basic/Diagnostic.h
+++ b/clang/include/clang/Basic/Diagnostic.h
@@ -20,11 +20,13 @@
 #include "clang/Basic/Specifiers.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/FunctionExtras.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/Compiler.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include <cassert>
 #include <cstdint>
 #include <limits>
@@ -555,6 +557,10 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
   void *ArgToStringCookie = nullptr;
   ArgToStringFnTy ArgToStringFn;
 
+  /// Whether the diagnostic should be suppressed in FilePath.
+  llvm::unique_function<bool(diag::kind, llvm::StringRef /*FilePath*/) const>
+      DiagSuppressionMapping;
+
 public:
   explicit DiagnosticsEngine(IntrusiveRefCntPtr<DiagnosticIDs> Diags,
                              IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts,
@@ -946,6 +952,13 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
     return (Level)Diags->getDiagnosticLevel(DiagID, Loc, *this);
   }
 
+  /// Diagnostic suppression mappings can be used to ignore diagnostics based on
+  /// the file they occur in.
+  /// These take presumed locations into account, and can still be overriden by
+  /// clang-diagnostics pragmas.
+  void setDiagSuppressionMapping(decltype(DiagSuppressionMapping) Mapping);
+  bool isSuppressedViaMapping(diag::kind D, llvm::StringRef FilePath) const;
+
   /// Issue the message to the client.
   ///
   /// This actually returns an instance of DiagnosticBuilder which emits the
@@ -1759,7 +1772,7 @@ const char ToggleHighlight = 127;
 /// warning options specified on the command line.
 void ProcessWarningOptions(DiagnosticsEngine &Diags,
                            const DiagnosticOptions &Opts,
-                           bool ReportDiags = true);
+                           llvm::vfs::FileSystem &VFS, bool ReportDiags = true);
 void EscapeStringForDiagnostic(StringRef Str, SmallVectorImpl<char> &OutStr);
 } // namespace clang
 
diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td b/clang/include/clang/Basic/DiagnosticDriverKinds.td
index 68722ad9633120..cbad93904a899f 100644
--- a/clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ b/clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -831,4 +831,7 @@ def err_drv_triple_version_invalid : Error<
 
 def warn_missing_include_dirs : Warning<
   "no such include directory: '%0'">, InGroup<MissingIncludeDirs>, DefaultIgnore;
+
+def err_drv_malformed_warning_suppression_mapping : Error<
+  "failed to process suppression mapping file '%0' : %1">;
 }
diff --git a/clang/include/clang/Basic/DiagnosticOptions.h b/clang/include/clang/Basic/DiagnosticOptions.h
index 30141c2b8f4475..0770805624cec8 100644
--- a/clang/include/clang/Basic/DiagnosticOptions.h
+++ b/clang/include/clang/Basic/DiagnosticOptions.h
@@ -108,6 +108,9 @@ class DiagnosticOptions : public RefCountedBase<DiagnosticOptions>{
   /// The file to serialize diagnostics to (non-appending).
   std::string DiagnosticSerializationFile;
 
+  /// File that defines suppression mappings.
+  std::string SuppressionMappingsFile;
+
   /// The list of -W... options used to alter the diagnostic mappings, with the
   /// prefixes removed.
   std::vector<std::string> Warnings;
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 379e75b197cf96..827126f5af30ca 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -8979,3 +8979,8 @@ def wasm_opt : Flag<["--"], "wasm-opt">,
   Group<m_Group>,
   HelpText<"Enable the wasm-opt optimizer (default)">,
   MarshallingInfoNegativeFlag<LangOpts<"NoWasmOpt">>;
+
+def warning_suppression_mappings_EQ : Joined<["--"],
+  "warning-suppression-mappings=">, Group<Diag_Group>,
+  HelpText<"File containing list of sources to suppress diagnostics for. See XXX for format">,
+  Visibility<[ClangOption, CC1Option]>;
diff --git a/clang/include/clang/Frontend/CompilerInstance.h b/clang/include/clang/Frontend/CompilerInstance.h
index 3464654284f199..338eb3c6bd028d 100644
--- a/clang/include/clang/Frontend/CompilerInstance.h
+++ b/clang/include/clang/Frontend/CompilerInstance.h
@@ -24,6 +24,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/BuryPointer.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include <cassert>
 #include <list>
 #include <memory>
@@ -701,11 +702,10 @@ class CompilerInstance : public ModuleLoader {
   /// used by some diagnostics printers (for logging purposes only).
   ///
   /// \return The new object on success, or null on failure.
-  static IntrusiveRefCntPtr<DiagnosticsEngine>
-  createDiagnostics(DiagnosticOptions *Opts,
-                    DiagnosticConsumer *Client = nullptr,
-                    bool ShouldOwnClient = true,
-                    const CodeGenOptions *CodeGenOpts = nullptr);
+  static IntrusiveRefCntPtr<DiagnosticsEngine> createDiagnostics(
+      DiagnosticOptions *Opts, DiagnosticConsumer *Client = nullptr,
+      bool ShouldOwnClient = true, const CodeGenOptions *CodeGenOpts = nullptr,
+      IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS = nullptr);
 
   /// Create the file manager and replace any existing one with it.
   ///
diff --git a/clang/lib/Basic/Diagnostic.cpp b/clang/lib/Basic/Diagnostic.cpp
index e23362fc7af00d..e536effd25072f 100644
--- a/clang/lib/Basic/Diagnostic.cpp
+++ b/clang/lib/Basic/Diagnostic.cpp
@@ -21,6 +21,7 @@
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Basic/TokenKinds.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
@@ -28,6 +29,7 @@
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/CrashRecoveryContext.h"
 #include "llvm/Support/Unicode.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
@@ -477,6 +479,16 @@ void DiagnosticsEngine::setSeverityForAll(diag::Flavor Flavor,
       setSeverity(Diag, Map, Loc);
 }
 
+void DiagnosticsEngine::setDiagSuppressionMapping(
+    decltype(DiagSuppressionMapping) Mapping) {
+  DiagSuppressionMapping = std::move(Mapping);
+}
+
+bool DiagnosticsEngine::isSuppressedViaMapping(diag::kind D,
+                                               llvm::StringRef FilePath) const {
+  return DiagSuppressionMapping && DiagSuppressionMapping(D, FilePath);
+}
+
 void DiagnosticsEngine::Report(const StoredDiagnostic &storedDiag) {
   DiagnosticStorage DiagStorage;
   DiagStorage.DiagRanges.append(storedDiag.range_begin(),
diff --git a/clang/lib/Basic/DiagnosticIDs.cpp b/clang/lib/Basic/DiagnosticIDs.cpp
index d45bb0f392d457..4f213e02fa9324 100644
--- a/clang/lib/Basic/DiagnosticIDs.cpp
+++ b/clang/lib/Basic/DiagnosticIDs.cpp
@@ -575,6 +575,12 @@ DiagnosticIDs::getDiagnosticSeverity(unsigned DiagID, SourceLocation Loc,
       DiagID != diag::fatal_too_many_errors && Diag.FatalsAsError)
     Result = diag::Severity::Error;
 
+  // Rest of the mappings are only applicable for diagnostics associated with a
+  // SourceLocation, bail out early for others.
+  if (!Diag.hasSourceManager())
+    return Result;
+
+  const auto &SM = Diag.getSourceManager();
   // Custom diagnostics always are emitted in system headers.
   bool ShowInSystemHeader =
       !GetDiagInfo(DiagID) || GetDiagInfo(DiagID)->WarnShowInSystemHeader;
@@ -582,18 +588,29 @@ DiagnosticIDs::getDiagnosticSeverity(unsigned DiagID, SourceLocation Loc,
   // 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.
-  if (State->SuppressSystemWarnings && !ShowInSystemHeader && Loc.isValid() &&
-      Diag.getSourceManager().isInSystemHeader(
-          Diag.getSourceManager().getExpansionLoc(Loc)))
+  if (State->SuppressSystemWarnings && !ShowInSystemHeader &&
+      SM.isInSystemHeader(SM.getExpansionLoc(Loc)))
     return diag::Severity::Ignored;
 
   // We also ignore warnings due to system macros
   bool ShowInSystemMacro =
       !GetDiagInfo(DiagID) || GetDiagInfo(DiagID)->WarnShowInSystemMacro;
-  if (State->SuppressSystemWarnings && !ShowInSystemMacro && Loc.isValid() &&
-      Diag.getSourceManager().isInSystemMacro(Loc))
+  if (State->SuppressSystemWarnings && !ShowInSystemMacro &&
+      SM.isInSystemMacro(Loc))
     return diag::Severity::Ignored;
 
+  // Apply suppression mappings if severity wasn't explicitly mapped with a
+  // clang-diagnostics pragma to ensure pragmas always take precedence over
+  // mappings.
+  // We also use presumed locations here to improve reproducibility for
+  // preprocessed inputs.
+  if (!Mapping.isPragma()) {
+    if (auto PLoc = SM.getPresumedLoc(Loc);
+        PLoc.isValid() &&
+        Diag.isSuppressedViaMapping(DiagID, PLoc.getFilename()))
+      return diag::Severity::Ignored;
+  }
+
   return Result;
 }
 
diff --git a/clang/lib/Basic/Warnings.cpp b/clang/lib/Basic/Warnings.cpp
index 5a5ac555633886..1047980b84d7db 100644
--- a/clang/lib/Basic/Warnings.cpp
+++ b/clang/lib/Basic/Warnings.cpp
@@ -23,10 +23,21 @@
 // simpler because a remark can't be promoted to an error.
 #include "clang/Basic/AllDiagnostics.h"
 #include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticDriver.h"
+#include "clang/Basic/DiagnosticIDs.h"
 #include "clang/Basic/DiagnosticOptions.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringMapEntry.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/SpecialCaseList.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include <algorithm>
 #include <cstring>
+#include <memory>
 #include <utility>
+#include <vector>
 using namespace clang;
 
 // EmitUnknownDiagWarning - Emit a warning and typo hint for unknown warning
@@ -41,8 +52,96 @@ static void EmitUnknownDiagWarning(DiagnosticsEngine &Diags,
       << (Prefix.str() += std::string(Suggestion));
 }
 
+namespace {
+class WarningsSpecialCaseList : public llvm::SpecialCaseList {
+public:
+  static std::unique_ptr<WarningsSpecialCaseList>
+  create(const llvm::MemoryBuffer &MB, std::string &Err) {
+    auto SCL = std::make_unique<WarningsSpecialCaseList>();
+    if (SCL->createInternal(&MB, Err))
+      return SCL;
+    return nullptr;
+  }
+
+  // Section names refer to diagnostic groups, which cover multiple individual
+  // diagnostics. Expand diagnostic groups here to individual diagnostics.
+  // A diagnostic can have multiple diagnostic groups associated with it, we let
+  // the last section take precedence in such cases.
+  void processSections(DiagnosticsEngine &Diags) {
+    // Drop the default section introduced by special case list, we only support
+    // exact diagnostic group names.
+    Sections.erase("*");
+    // Make sure we iterate sections by their line numbers.
+    std::vector<std::pair<unsigned, const llvm::StringMapEntry<Section> *>>
+        LineAndSection;
+    for (const auto &Entry : Sections) {
+      LineAndSection.emplace_back(
+          Entry.second.SectionMatcher->Globs.at(Entry.first()).second, &Entry);
+    }
+    llvm::sort(LineAndSection);
+    for (const auto &[_, Entry] : LineAndSection) {
+      SmallVector<diag::kind, 256> GroupDiags;
+      if (Diags.getDiagnosticIDs()->getDiagnosticsInGroup(
+              clang::diag::Flavor::WarningOrError, Entry->first(),
+              GroupDiags)) {
+        EmitUnknownDiagWarning(Diags, clang::diag::Flavor::WarningOrError, "",
+                               Entry->first());
+        continue;
+      }
+      for (auto D : GroupDiags)
+        DiagToSection[D] = &Entry->getValue();
+    }
+  }
+
+  bool isDiagSuppressed(diag::kind D, llvm::StringRef FilePath) const {
+    auto Section = DiagToSection.find(D);
+    if (Section == DiagToSection.end())
+      return false;
+    auto SrcEntries = Section->second->Entries.find("src");
+    if(SrcEntries == Section->second->Entries.end())
+      return false;
+    // Find the longest glob pattern that matches FilePath. A positive match
+    // implies D should be suppressed for FilePath.
+    llvm::StringRef LongestMatch;
+    bool LongestWasNegative;
+    for (const auto &CatIt : SrcEntries->second) {
+      bool IsNegative = CatIt.first() == "emit";
+      for (const auto &GlobIt : CatIt.second.Globs) {
+        if (GlobIt.getKeyLength() < LongestMatch.size())
+          continue;
+        if (!GlobIt.second.first.match(FilePath))
+          continue;
+        LongestMatch = GlobIt.getKey();
+        LongestWasNegative = IsNegative;
+      }
+    }
+    return !LongestMatch.empty() && !LongestWasNegative;
+  }
+
+private:
+  llvm::DenseMap<diag::kind, const Section*> DiagToSection;
+};
+
+void parseSuppressionMappings(const llvm::MemoryBuffer &MB,
+                              DiagnosticsEngine &Diags) {
+  std::string Err;
+  auto SCL = WarningsSpecialCaseList::create(MB, Err);
+  if (!SCL) {
+    Diags.Report(diag::err_drv_malformed_warning_suppression_mapping)
+        << MB.getBufferIdentifier() << Err;
+    return;
+  }
+  SCL->processSections(Diags);
+  Diags.setDiagSuppressionMapping(
+      [SCL(std::move(SCL))](diag::kind K, llvm::StringRef Path) {
+        return SCL->isDiagSuppressed(K, Path);
+      });
+}
+} // namespace
+
 void clang::ProcessWarningOptions(DiagnosticsEngine &Diags,
                                   const DiagnosticOptions &Opts,
+                                  llvm::vfs::FileSystem& VFS,
                                   bool ReportDiags) {
   Diags.setSuppressSystemWarnings(true);  // Default to -Wno-system-headers
   Diags.setIgnoreAllWarnings(Opts.IgnoreWarnings);
@@ -70,6 +169,14 @@ void clang::ProcessWarningOptions(DiagnosticsEngine &Diags,
   else
     Diags.setExtensionHandlingBehavior(diag::Severity::Ignored);
 
+  if (!Opts.SuppressionMappingsFile.empty()) {
+    if (auto Buf = VFS.getBufferForFile(Opts.SuppressionMappingsFile)) {
+      parseSuppressionMappings(**Buf, Diags);
+    } else if (ReportDiags) {
+      Diags.Report(diag::err_drv_no_such_file) << Opts.SuppressionMappingsFile;
+    }
+  }
+
   SmallVector<diag::kind, 10> _Diags;
   const IntrusiveRefCntPtr< DiagnosticIDs > DiagIDs =
     Diags.getDiagnosticIDs();
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 3fc39296f44281..55ec543c5de514 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -4456,6 +4456,13 @@ static void RenderDiagnosticsOptions(const Driver &D, const ArgList &Args,
 
   Args.addOptOutFlag(CmdArgs, options::OPT_fspell_checking,
                      options::OPT_fno_spell_checking);
+
+  if (const Arg *A =
+          Args.getLastArg(options::OPT_warning_suppression_mappings_EQ)) {
+    if (!D.getVFS().exists(A->getValue()))
+      D.Diag(clang::diag::err_drv_no_such_file) << A->getValue();
+    CmdArgs.push_back(Args.MakeArgString(A->getSpelling() + A->getValue()));
+  }
 }
 
 DwarfFissionKind tools::getDebugFissionKind(const Driver &D,
diff --git a/clang/lib/Frontend/ASTUnit.cpp b/clang/lib/Frontend/ASTUnit.cpp
index bffff0d27af3ab..464ed8090a0ec5 100644
--- a/clang/lib/Frontend/ASTUnit.cpp
+++ b/clang/lib/Frontend/ASTUnit.cpp
@@ -1367,7 +1367,7 @@ ASTUnit::getMainBufferWithPrecompiledPreamble(
       // after parsing the preamble.
       getDiagnostics().Reset();
       ProcessWarningOptions(getDiagnostics(),
-                            PreambleInvocationIn.getDiagnosticOpts());
+                            PreambleInvocationIn.getDiagnosticOpts(), *VFS);
       getDiagnostics().setNumWarnings(NumWarningsInPreamble);
 
       PreambleRebuildCountdown = 1;
@@ -1593,7 +1593,8 @@ ASTUnit *ASTUnit::LoadFromCompilerInvocationAction(
   // We'll manage file buffers ourselves.
   CI->getPreprocessorOpts().RetainRemappedFileBuffers = true;
   CI->getFrontendOpts().DisableFree = false;
-  ProcessWarningOptions(AST->getDiagnostics(), CI->getDiagnosticOpts());
+  ProcessWarningOptions(AST->getDiagnostics(), CI->getDiagnosticOpts(),
+                        AST->getFileManager().getVirtualFileSystem());
 
   // Create the compiler instance to use for building the AST.
   std::unique_ptr<CompilerInstance> Clang(
@@ -1701,7 +1702,7 @@ bool ASTUnit::LoadFromCompilerInvocation(
   Invocation->getPreprocessorOpts().RetainRemappedFileBuffers = true;
   Invocation->getFrontendOpts().DisableFree = false;
   getDiagnostics().Reset();
-  ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts());
+  ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts(), *VFS);
 
   std::unique_ptr<llvm::MemoryBuffer> OverrideMainBuffer;
   if (PrecompilePreambleAfterNParses > 0) {
@@ -1709,7 +1710,7 @@ bool ASTUnit::LoadFromCompilerInvocation(
     OverrideMainBuffer =
         getMainBufferWithPrecompiledPreamble(PCHContainerOps, *Invocation, VFS);
     getDiagnostics().Reset();
-    ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts());
+    ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts(), *VFS);
   }
 
   SimpleTimer ParsingTimer(WantTiming);
@@ -1902,7 +1903,8 @@ bool ASTUnit::Reparse(std::shared_ptr<PCHContainerOperations> PCHContainerOps,
   // Clear out the diagnostics state.
   FileMgr.reset();
   getDiagnostics().Reset();
-  ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts());
+  ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts(),
+                        *VFS);
   if (OverrideMainBuffer)
     getDiagnostics().setNumWarnings(NumWarningsInPreamble);
 
@@ -2241,7 +2243,8 @@ void ASTUnit::CodeComplete(
   CaptureDroppedDiagnostics Capture(CaptureDiagsKind::All,
                                     Clang->getDiagnostics(),
                                     &StoredDiagnostics, nullptr);
-  ProcessWarningOptions(Diag, Inv.getDiagnosticOpts());
+  ProcessWarningOptions(Diag, Inv.getDiagnosticOpts(),
+                        FileMgr.getVirtualFileSystem());
 
   // Create the target instance.
   if (!Clang->createTarget()) {
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index 240305b33824b8..ecc6782c7cb4fb 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -39,6 +39,7 @@
 #include "clang/Serialization/ASTReader.h"
 #include "clang/Serialization/GlobalModuleIndex.h"
 #include "clang/Serialization/InMemoryModuleCache.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/Statistic.h"
@@ -54,6 +55,7 @@
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/TimeProfiler.h"
 #include "llvm/Support/Timer.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/TargetParser/Host.h"
 #include <optional>
@@ -332,19 +334,22 @@ static void SetupSerializedDiagnostics(DiagnosticOptions *DiagOpts,
 
 void CompilerInstance::createDiagnostics(DiagnosticConsumer *Client,
                                          bool ShouldOwnClient) {
-  Diagnostics = createDiagnostics(&getDiagnosticOpts(), Cl...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented Oct 16, 2024

@llvm/pr-subscribers-clang-tools-extra

Author: kadir çetinkaya (kadircet)

Changes

This implements
https://discourse.llvm.org/t/rfc-add-support-for-controlling-diagnostics-severities-at-file-level-granularity-through-command-line/81292.

Users now can suppress warnings for certain headers by providing a
mapping with globs, a sample file looks like:

[unused]
src:*
src:*clang/*=emit

This will suppress warnings from -Wunused group in all files that
aren't under clang/ directory. This mapping file can be passed to
clang via --warning-suppression-mappings=foo.txt.

At a high level, mapping file is stored in DiagnosticOptions and then
processed with rest of the warning flags when creating a
DiagnosticsEngine. This is a functor that uses SpecialCaseLists
underneath to match against globs coming from the mappings file.

This implies processing warning options now performs IO, relevant
interfaces are updated to take in a VFS, falling back to RealFileSystem
when one is not available.


Patch is 36.96 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/112517.diff

23 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp (+2-1)
  • (modified) clang/include/clang/Basic/Diagnostic.h (+14-1)
  • (modified) clang/include/clang/Basic/DiagnosticDriverKinds.td (+3)
  • (modified) clang/include/clang/Basic/DiagnosticOptions.h (+3)
  • (modified) clang/include/clang/Driver/Options.td (+5)
  • (modified) clang/include/clang/Frontend/CompilerInstance.h (+5-5)
  • (modified) clang/lib/Basic/Diagnostic.cpp (+12)
  • (modified) clang/lib/Basic/DiagnosticIDs.cpp (+22-5)
  • (modified) clang/lib/Basic/Warnings.cpp (+107)
  • (modified) clang/lib/Driver/ToolChains/Clang.cpp (+7)
  • (modified) clang/lib/Frontend/ASTUnit.cpp (+9-6)
  • (modified) clang/lib/Frontend/CompilerInstance.cpp (+13-8)
  • (modified) clang/lib/Frontend/CompilerInvocation.cpp (+8)
  • (modified) clang/lib/Frontend/PrecompiledPreamble.cpp (+1-1)
  • (modified) clang/lib/Interpreter/CodeCompletion.cpp (+2-2)
  • (modified) clang/lib/Serialization/ASTReader.cpp (+3-1)
  • (added) clang/test/Misc/Inputs/suppression-mapping.txt (+4)
  • (added) clang/test/Misc/warning-suppression-mappings.cpp (+16)
  • (modified) clang/tools/driver/cc1gen_reproducer_main.cpp (+4-3)
  • (modified) clang/tools/driver/driver.cpp (+5-2)
  • (modified) clang/unittests/Basic/DiagnosticTest.cpp (+164)
  • (modified) clang/unittests/Frontend/CompilerInvocationTest.cpp (+10)
  • (modified) llvm/include/llvm/Support/SpecialCaseList.h (-1)
diff --git a/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp b/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
index fef086c5a99d86..4c34f9ea122d9e 100644
--- a/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
+++ b/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
@@ -81,7 +81,8 @@ ExpandModularHeadersPPCallbacks::ExpandModularHeadersPPCallbacks(
   Diags.setSourceManager(&Sources);
   // FIXME: Investigate whatever is there better way to initialize DiagEngine
   // or whatever DiagEngine can be shared by multiple preprocessors
-  ProcessWarningOptions(Diags, Compiler.getDiagnosticOpts());
+  ProcessWarningOptions(Diags, Compiler.getDiagnosticOpts(),
+                        Compiler.getVirtualFileSystem());
 
   LangOpts.Modules = false;
 
diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h
index 3b1efdb12824c7..79b7545e92d6da 100644
--- a/clang/include/clang/Basic/Diagnostic.h
+++ b/clang/include/clang/Basic/Diagnostic.h
@@ -20,11 +20,13 @@
 #include "clang/Basic/Specifiers.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/FunctionExtras.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/Compiler.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include <cassert>
 #include <cstdint>
 #include <limits>
@@ -555,6 +557,10 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
   void *ArgToStringCookie = nullptr;
   ArgToStringFnTy ArgToStringFn;
 
+  /// Whether the diagnostic should be suppressed in FilePath.
+  llvm::unique_function<bool(diag::kind, llvm::StringRef /*FilePath*/) const>
+      DiagSuppressionMapping;
+
 public:
   explicit DiagnosticsEngine(IntrusiveRefCntPtr<DiagnosticIDs> Diags,
                              IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts,
@@ -946,6 +952,13 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
     return (Level)Diags->getDiagnosticLevel(DiagID, Loc, *this);
   }
 
+  /// Diagnostic suppression mappings can be used to ignore diagnostics based on
+  /// the file they occur in.
+  /// These take presumed locations into account, and can still be overriden by
+  /// clang-diagnostics pragmas.
+  void setDiagSuppressionMapping(decltype(DiagSuppressionMapping) Mapping);
+  bool isSuppressedViaMapping(diag::kind D, llvm::StringRef FilePath) const;
+
   /// Issue the message to the client.
   ///
   /// This actually returns an instance of DiagnosticBuilder which emits the
@@ -1759,7 +1772,7 @@ const char ToggleHighlight = 127;
 /// warning options specified on the command line.
 void ProcessWarningOptions(DiagnosticsEngine &Diags,
                            const DiagnosticOptions &Opts,
-                           bool ReportDiags = true);
+                           llvm::vfs::FileSystem &VFS, bool ReportDiags = true);
 void EscapeStringForDiagnostic(StringRef Str, SmallVectorImpl<char> &OutStr);
 } // namespace clang
 
diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td b/clang/include/clang/Basic/DiagnosticDriverKinds.td
index 68722ad9633120..cbad93904a899f 100644
--- a/clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ b/clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -831,4 +831,7 @@ def err_drv_triple_version_invalid : Error<
 
 def warn_missing_include_dirs : Warning<
   "no such include directory: '%0'">, InGroup<MissingIncludeDirs>, DefaultIgnore;
+
+def err_drv_malformed_warning_suppression_mapping : Error<
+  "failed to process suppression mapping file '%0' : %1">;
 }
diff --git a/clang/include/clang/Basic/DiagnosticOptions.h b/clang/include/clang/Basic/DiagnosticOptions.h
index 30141c2b8f4475..0770805624cec8 100644
--- a/clang/include/clang/Basic/DiagnosticOptions.h
+++ b/clang/include/clang/Basic/DiagnosticOptions.h
@@ -108,6 +108,9 @@ class DiagnosticOptions : public RefCountedBase<DiagnosticOptions>{
   /// The file to serialize diagnostics to (non-appending).
   std::string DiagnosticSerializationFile;
 
+  /// File that defines suppression mappings.
+  std::string SuppressionMappingsFile;
+
   /// The list of -W... options used to alter the diagnostic mappings, with the
   /// prefixes removed.
   std::vector<std::string> Warnings;
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 379e75b197cf96..827126f5af30ca 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -8979,3 +8979,8 @@ def wasm_opt : Flag<["--"], "wasm-opt">,
   Group<m_Group>,
   HelpText<"Enable the wasm-opt optimizer (default)">,
   MarshallingInfoNegativeFlag<LangOpts<"NoWasmOpt">>;
+
+def warning_suppression_mappings_EQ : Joined<["--"],
+  "warning-suppression-mappings=">, Group<Diag_Group>,
+  HelpText<"File containing list of sources to suppress diagnostics for. See XXX for format">,
+  Visibility<[ClangOption, CC1Option]>;
diff --git a/clang/include/clang/Frontend/CompilerInstance.h b/clang/include/clang/Frontend/CompilerInstance.h
index 3464654284f199..338eb3c6bd028d 100644
--- a/clang/include/clang/Frontend/CompilerInstance.h
+++ b/clang/include/clang/Frontend/CompilerInstance.h
@@ -24,6 +24,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/BuryPointer.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include <cassert>
 #include <list>
 #include <memory>
@@ -701,11 +702,10 @@ class CompilerInstance : public ModuleLoader {
   /// used by some diagnostics printers (for logging purposes only).
   ///
   /// \return The new object on success, or null on failure.
-  static IntrusiveRefCntPtr<DiagnosticsEngine>
-  createDiagnostics(DiagnosticOptions *Opts,
-                    DiagnosticConsumer *Client = nullptr,
-                    bool ShouldOwnClient = true,
-                    const CodeGenOptions *CodeGenOpts = nullptr);
+  static IntrusiveRefCntPtr<DiagnosticsEngine> createDiagnostics(
+      DiagnosticOptions *Opts, DiagnosticConsumer *Client = nullptr,
+      bool ShouldOwnClient = true, const CodeGenOptions *CodeGenOpts = nullptr,
+      IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS = nullptr);
 
   /// Create the file manager and replace any existing one with it.
   ///
diff --git a/clang/lib/Basic/Diagnostic.cpp b/clang/lib/Basic/Diagnostic.cpp
index e23362fc7af00d..e536effd25072f 100644
--- a/clang/lib/Basic/Diagnostic.cpp
+++ b/clang/lib/Basic/Diagnostic.cpp
@@ -21,6 +21,7 @@
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Basic/TokenKinds.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
@@ -28,6 +29,7 @@
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/CrashRecoveryContext.h"
 #include "llvm/Support/Unicode.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
@@ -477,6 +479,16 @@ void DiagnosticsEngine::setSeverityForAll(diag::Flavor Flavor,
       setSeverity(Diag, Map, Loc);
 }
 
+void DiagnosticsEngine::setDiagSuppressionMapping(
+    decltype(DiagSuppressionMapping) Mapping) {
+  DiagSuppressionMapping = std::move(Mapping);
+}
+
+bool DiagnosticsEngine::isSuppressedViaMapping(diag::kind D,
+                                               llvm::StringRef FilePath) const {
+  return DiagSuppressionMapping && DiagSuppressionMapping(D, FilePath);
+}
+
 void DiagnosticsEngine::Report(const StoredDiagnostic &storedDiag) {
   DiagnosticStorage DiagStorage;
   DiagStorage.DiagRanges.append(storedDiag.range_begin(),
diff --git a/clang/lib/Basic/DiagnosticIDs.cpp b/clang/lib/Basic/DiagnosticIDs.cpp
index d45bb0f392d457..4f213e02fa9324 100644
--- a/clang/lib/Basic/DiagnosticIDs.cpp
+++ b/clang/lib/Basic/DiagnosticIDs.cpp
@@ -575,6 +575,12 @@ DiagnosticIDs::getDiagnosticSeverity(unsigned DiagID, SourceLocation Loc,
       DiagID != diag::fatal_too_many_errors && Diag.FatalsAsError)
     Result = diag::Severity::Error;
 
+  // Rest of the mappings are only applicable for diagnostics associated with a
+  // SourceLocation, bail out early for others.
+  if (!Diag.hasSourceManager())
+    return Result;
+
+  const auto &SM = Diag.getSourceManager();
   // Custom diagnostics always are emitted in system headers.
   bool ShowInSystemHeader =
       !GetDiagInfo(DiagID) || GetDiagInfo(DiagID)->WarnShowInSystemHeader;
@@ -582,18 +588,29 @@ DiagnosticIDs::getDiagnosticSeverity(unsigned DiagID, SourceLocation Loc,
   // 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.
-  if (State->SuppressSystemWarnings && !ShowInSystemHeader && Loc.isValid() &&
-      Diag.getSourceManager().isInSystemHeader(
-          Diag.getSourceManager().getExpansionLoc(Loc)))
+  if (State->SuppressSystemWarnings && !ShowInSystemHeader &&
+      SM.isInSystemHeader(SM.getExpansionLoc(Loc)))
     return diag::Severity::Ignored;
 
   // We also ignore warnings due to system macros
   bool ShowInSystemMacro =
       !GetDiagInfo(DiagID) || GetDiagInfo(DiagID)->WarnShowInSystemMacro;
-  if (State->SuppressSystemWarnings && !ShowInSystemMacro && Loc.isValid() &&
-      Diag.getSourceManager().isInSystemMacro(Loc))
+  if (State->SuppressSystemWarnings && !ShowInSystemMacro &&
+      SM.isInSystemMacro(Loc))
     return diag::Severity::Ignored;
 
+  // Apply suppression mappings if severity wasn't explicitly mapped with a
+  // clang-diagnostics pragma to ensure pragmas always take precedence over
+  // mappings.
+  // We also use presumed locations here to improve reproducibility for
+  // preprocessed inputs.
+  if (!Mapping.isPragma()) {
+    if (auto PLoc = SM.getPresumedLoc(Loc);
+        PLoc.isValid() &&
+        Diag.isSuppressedViaMapping(DiagID, PLoc.getFilename()))
+      return diag::Severity::Ignored;
+  }
+
   return Result;
 }
 
diff --git a/clang/lib/Basic/Warnings.cpp b/clang/lib/Basic/Warnings.cpp
index 5a5ac555633886..1047980b84d7db 100644
--- a/clang/lib/Basic/Warnings.cpp
+++ b/clang/lib/Basic/Warnings.cpp
@@ -23,10 +23,21 @@
 // simpler because a remark can't be promoted to an error.
 #include "clang/Basic/AllDiagnostics.h"
 #include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticDriver.h"
+#include "clang/Basic/DiagnosticIDs.h"
 #include "clang/Basic/DiagnosticOptions.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringMapEntry.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/SpecialCaseList.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include <algorithm>
 #include <cstring>
+#include <memory>
 #include <utility>
+#include <vector>
 using namespace clang;
 
 // EmitUnknownDiagWarning - Emit a warning and typo hint for unknown warning
@@ -41,8 +52,96 @@ static void EmitUnknownDiagWarning(DiagnosticsEngine &Diags,
       << (Prefix.str() += std::string(Suggestion));
 }
 
+namespace {
+class WarningsSpecialCaseList : public llvm::SpecialCaseList {
+public:
+  static std::unique_ptr<WarningsSpecialCaseList>
+  create(const llvm::MemoryBuffer &MB, std::string &Err) {
+    auto SCL = std::make_unique<WarningsSpecialCaseList>();
+    if (SCL->createInternal(&MB, Err))
+      return SCL;
+    return nullptr;
+  }
+
+  // Section names refer to diagnostic groups, which cover multiple individual
+  // diagnostics. Expand diagnostic groups here to individual diagnostics.
+  // A diagnostic can have multiple diagnostic groups associated with it, we let
+  // the last section take precedence in such cases.
+  void processSections(DiagnosticsEngine &Diags) {
+    // Drop the default section introduced by special case list, we only support
+    // exact diagnostic group names.
+    Sections.erase("*");
+    // Make sure we iterate sections by their line numbers.
+    std::vector<std::pair<unsigned, const llvm::StringMapEntry<Section> *>>
+        LineAndSection;
+    for (const auto &Entry : Sections) {
+      LineAndSection.emplace_back(
+          Entry.second.SectionMatcher->Globs.at(Entry.first()).second, &Entry);
+    }
+    llvm::sort(LineAndSection);
+    for (const auto &[_, Entry] : LineAndSection) {
+      SmallVector<diag::kind, 256> GroupDiags;
+      if (Diags.getDiagnosticIDs()->getDiagnosticsInGroup(
+              clang::diag::Flavor::WarningOrError, Entry->first(),
+              GroupDiags)) {
+        EmitUnknownDiagWarning(Diags, clang::diag::Flavor::WarningOrError, "",
+                               Entry->first());
+        continue;
+      }
+      for (auto D : GroupDiags)
+        DiagToSection[D] = &Entry->getValue();
+    }
+  }
+
+  bool isDiagSuppressed(diag::kind D, llvm::StringRef FilePath) const {
+    auto Section = DiagToSection.find(D);
+    if (Section == DiagToSection.end())
+      return false;
+    auto SrcEntries = Section->second->Entries.find("src");
+    if(SrcEntries == Section->second->Entries.end())
+      return false;
+    // Find the longest glob pattern that matches FilePath. A positive match
+    // implies D should be suppressed for FilePath.
+    llvm::StringRef LongestMatch;
+    bool LongestWasNegative;
+    for (const auto &CatIt : SrcEntries->second) {
+      bool IsNegative = CatIt.first() == "emit";
+      for (const auto &GlobIt : CatIt.second.Globs) {
+        if (GlobIt.getKeyLength() < LongestMatch.size())
+          continue;
+        if (!GlobIt.second.first.match(FilePath))
+          continue;
+        LongestMatch = GlobIt.getKey();
+        LongestWasNegative = IsNegative;
+      }
+    }
+    return !LongestMatch.empty() && !LongestWasNegative;
+  }
+
+private:
+  llvm::DenseMap<diag::kind, const Section*> DiagToSection;
+};
+
+void parseSuppressionMappings(const llvm::MemoryBuffer &MB,
+                              DiagnosticsEngine &Diags) {
+  std::string Err;
+  auto SCL = WarningsSpecialCaseList::create(MB, Err);
+  if (!SCL) {
+    Diags.Report(diag::err_drv_malformed_warning_suppression_mapping)
+        << MB.getBufferIdentifier() << Err;
+    return;
+  }
+  SCL->processSections(Diags);
+  Diags.setDiagSuppressionMapping(
+      [SCL(std::move(SCL))](diag::kind K, llvm::StringRef Path) {
+        return SCL->isDiagSuppressed(K, Path);
+      });
+}
+} // namespace
+
 void clang::ProcessWarningOptions(DiagnosticsEngine &Diags,
                                   const DiagnosticOptions &Opts,
+                                  llvm::vfs::FileSystem& VFS,
                                   bool ReportDiags) {
   Diags.setSuppressSystemWarnings(true);  // Default to -Wno-system-headers
   Diags.setIgnoreAllWarnings(Opts.IgnoreWarnings);
@@ -70,6 +169,14 @@ void clang::ProcessWarningOptions(DiagnosticsEngine &Diags,
   else
     Diags.setExtensionHandlingBehavior(diag::Severity::Ignored);
 
+  if (!Opts.SuppressionMappingsFile.empty()) {
+    if (auto Buf = VFS.getBufferForFile(Opts.SuppressionMappingsFile)) {
+      parseSuppressionMappings(**Buf, Diags);
+    } else if (ReportDiags) {
+      Diags.Report(diag::err_drv_no_such_file) << Opts.SuppressionMappingsFile;
+    }
+  }
+
   SmallVector<diag::kind, 10> _Diags;
   const IntrusiveRefCntPtr< DiagnosticIDs > DiagIDs =
     Diags.getDiagnosticIDs();
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 3fc39296f44281..55ec543c5de514 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -4456,6 +4456,13 @@ static void RenderDiagnosticsOptions(const Driver &D, const ArgList &Args,
 
   Args.addOptOutFlag(CmdArgs, options::OPT_fspell_checking,
                      options::OPT_fno_spell_checking);
+
+  if (const Arg *A =
+          Args.getLastArg(options::OPT_warning_suppression_mappings_EQ)) {
+    if (!D.getVFS().exists(A->getValue()))
+      D.Diag(clang::diag::err_drv_no_such_file) << A->getValue();
+    CmdArgs.push_back(Args.MakeArgString(A->getSpelling() + A->getValue()));
+  }
 }
 
 DwarfFissionKind tools::getDebugFissionKind(const Driver &D,
diff --git a/clang/lib/Frontend/ASTUnit.cpp b/clang/lib/Frontend/ASTUnit.cpp
index bffff0d27af3ab..464ed8090a0ec5 100644
--- a/clang/lib/Frontend/ASTUnit.cpp
+++ b/clang/lib/Frontend/ASTUnit.cpp
@@ -1367,7 +1367,7 @@ ASTUnit::getMainBufferWithPrecompiledPreamble(
       // after parsing the preamble.
       getDiagnostics().Reset();
       ProcessWarningOptions(getDiagnostics(),
-                            PreambleInvocationIn.getDiagnosticOpts());
+                            PreambleInvocationIn.getDiagnosticOpts(), *VFS);
       getDiagnostics().setNumWarnings(NumWarningsInPreamble);
 
       PreambleRebuildCountdown = 1;
@@ -1593,7 +1593,8 @@ ASTUnit *ASTUnit::LoadFromCompilerInvocationAction(
   // We'll manage file buffers ourselves.
   CI->getPreprocessorOpts().RetainRemappedFileBuffers = true;
   CI->getFrontendOpts().DisableFree = false;
-  ProcessWarningOptions(AST->getDiagnostics(), CI->getDiagnosticOpts());
+  ProcessWarningOptions(AST->getDiagnostics(), CI->getDiagnosticOpts(),
+                        AST->getFileManager().getVirtualFileSystem());
 
   // Create the compiler instance to use for building the AST.
   std::unique_ptr<CompilerInstance> Clang(
@@ -1701,7 +1702,7 @@ bool ASTUnit::LoadFromCompilerInvocation(
   Invocation->getPreprocessorOpts().RetainRemappedFileBuffers = true;
   Invocation->getFrontendOpts().DisableFree = false;
   getDiagnostics().Reset();
-  ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts());
+  ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts(), *VFS);
 
   std::unique_ptr<llvm::MemoryBuffer> OverrideMainBuffer;
   if (PrecompilePreambleAfterNParses > 0) {
@@ -1709,7 +1710,7 @@ bool ASTUnit::LoadFromCompilerInvocation(
     OverrideMainBuffer =
         getMainBufferWithPrecompiledPreamble(PCHContainerOps, *Invocation, VFS);
     getDiagnostics().Reset();
-    ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts());
+    ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts(), *VFS);
   }
 
   SimpleTimer ParsingTimer(WantTiming);
@@ -1902,7 +1903,8 @@ bool ASTUnit::Reparse(std::shared_ptr<PCHContainerOperations> PCHContainerOps,
   // Clear out the diagnostics state.
   FileMgr.reset();
   getDiagnostics().Reset();
-  ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts());
+  ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts(),
+                        *VFS);
   if (OverrideMainBuffer)
     getDiagnostics().setNumWarnings(NumWarningsInPreamble);
 
@@ -2241,7 +2243,8 @@ void ASTUnit::CodeComplete(
   CaptureDroppedDiagnostics Capture(CaptureDiagsKind::All,
                                     Clang->getDiagnostics(),
                                     &StoredDiagnostics, nullptr);
-  ProcessWarningOptions(Diag, Inv.getDiagnosticOpts());
+  ProcessWarningOptions(Diag, Inv.getDiagnosticOpts(),
+                        FileMgr.getVirtualFileSystem());
 
   // Create the target instance.
   if (!Clang->createTarget()) {
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index 240305b33824b8..ecc6782c7cb4fb 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -39,6 +39,7 @@
 #include "clang/Serialization/ASTReader.h"
 #include "clang/Serialization/GlobalModuleIndex.h"
 #include "clang/Serialization/InMemoryModuleCache.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/Statistic.h"
@@ -54,6 +55,7 @@
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/TimeProfiler.h"
 #include "llvm/Support/Timer.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/TargetParser/Host.h"
 #include <optional>
@@ -332,19 +334,22 @@ static void SetupSerializedDiagnostics(DiagnosticOptions *DiagOpts,
 
 void CompilerInstance::createDiagnostics(DiagnosticConsumer *Client,
                                          bool ShouldOwnClient) {
-  Diagnostics = createDiagnostics(&getDiagnosticOpts(), Cl...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented Oct 16, 2024

@llvm/pr-subscribers-clang-modules

Author: kadir çetinkaya (kadircet)

Changes

This implements
https://discourse.llvm.org/t/rfc-add-support-for-controlling-diagnostics-severities-at-file-level-granularity-through-command-line/81292.

Users now can suppress warnings for certain headers by providing a
mapping with globs, a sample file looks like:

[unused]
src:*
src:*clang/*=emit

This will suppress warnings from -Wunused group in all files that
aren't under clang/ directory. This mapping file can be passed to
clang via --warning-suppression-mappings=foo.txt.

At a high level, mapping file is stored in DiagnosticOptions and then
processed with rest of the warning flags when creating a
DiagnosticsEngine. This is a functor that uses SpecialCaseLists
underneath to match against globs coming from the mappings file.

This implies processing warning options now performs IO, relevant
interfaces are updated to take in a VFS, falling back to RealFileSystem
when one is not available.


Patch is 36.96 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/112517.diff

23 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp (+2-1)
  • (modified) clang/include/clang/Basic/Diagnostic.h (+14-1)
  • (modified) clang/include/clang/Basic/DiagnosticDriverKinds.td (+3)
  • (modified) clang/include/clang/Basic/DiagnosticOptions.h (+3)
  • (modified) clang/include/clang/Driver/Options.td (+5)
  • (modified) clang/include/clang/Frontend/CompilerInstance.h (+5-5)
  • (modified) clang/lib/Basic/Diagnostic.cpp (+12)
  • (modified) clang/lib/Basic/DiagnosticIDs.cpp (+22-5)
  • (modified) clang/lib/Basic/Warnings.cpp (+107)
  • (modified) clang/lib/Driver/ToolChains/Clang.cpp (+7)
  • (modified) clang/lib/Frontend/ASTUnit.cpp (+9-6)
  • (modified) clang/lib/Frontend/CompilerInstance.cpp (+13-8)
  • (modified) clang/lib/Frontend/CompilerInvocation.cpp (+8)
  • (modified) clang/lib/Frontend/PrecompiledPreamble.cpp (+1-1)
  • (modified) clang/lib/Interpreter/CodeCompletion.cpp (+2-2)
  • (modified) clang/lib/Serialization/ASTReader.cpp (+3-1)
  • (added) clang/test/Misc/Inputs/suppression-mapping.txt (+4)
  • (added) clang/test/Misc/warning-suppression-mappings.cpp (+16)
  • (modified) clang/tools/driver/cc1gen_reproducer_main.cpp (+4-3)
  • (modified) clang/tools/driver/driver.cpp (+5-2)
  • (modified) clang/unittests/Basic/DiagnosticTest.cpp (+164)
  • (modified) clang/unittests/Frontend/CompilerInvocationTest.cpp (+10)
  • (modified) llvm/include/llvm/Support/SpecialCaseList.h (-1)
diff --git a/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp b/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
index fef086c5a99d86..4c34f9ea122d9e 100644
--- a/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
+++ b/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
@@ -81,7 +81,8 @@ ExpandModularHeadersPPCallbacks::ExpandModularHeadersPPCallbacks(
   Diags.setSourceManager(&Sources);
   // FIXME: Investigate whatever is there better way to initialize DiagEngine
   // or whatever DiagEngine can be shared by multiple preprocessors
-  ProcessWarningOptions(Diags, Compiler.getDiagnosticOpts());
+  ProcessWarningOptions(Diags, Compiler.getDiagnosticOpts(),
+                        Compiler.getVirtualFileSystem());
 
   LangOpts.Modules = false;
 
diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h
index 3b1efdb12824c7..79b7545e92d6da 100644
--- a/clang/include/clang/Basic/Diagnostic.h
+++ b/clang/include/clang/Basic/Diagnostic.h
@@ -20,11 +20,13 @@
 #include "clang/Basic/Specifiers.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/FunctionExtras.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/Compiler.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include <cassert>
 #include <cstdint>
 #include <limits>
@@ -555,6 +557,10 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
   void *ArgToStringCookie = nullptr;
   ArgToStringFnTy ArgToStringFn;
 
+  /// Whether the diagnostic should be suppressed in FilePath.
+  llvm::unique_function<bool(diag::kind, llvm::StringRef /*FilePath*/) const>
+      DiagSuppressionMapping;
+
 public:
   explicit DiagnosticsEngine(IntrusiveRefCntPtr<DiagnosticIDs> Diags,
                              IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts,
@@ -946,6 +952,13 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
     return (Level)Diags->getDiagnosticLevel(DiagID, Loc, *this);
   }
 
+  /// Diagnostic suppression mappings can be used to ignore diagnostics based on
+  /// the file they occur in.
+  /// These take presumed locations into account, and can still be overriden by
+  /// clang-diagnostics pragmas.
+  void setDiagSuppressionMapping(decltype(DiagSuppressionMapping) Mapping);
+  bool isSuppressedViaMapping(diag::kind D, llvm::StringRef FilePath) const;
+
   /// Issue the message to the client.
   ///
   /// This actually returns an instance of DiagnosticBuilder which emits the
@@ -1759,7 +1772,7 @@ const char ToggleHighlight = 127;
 /// warning options specified on the command line.
 void ProcessWarningOptions(DiagnosticsEngine &Diags,
                            const DiagnosticOptions &Opts,
-                           bool ReportDiags = true);
+                           llvm::vfs::FileSystem &VFS, bool ReportDiags = true);
 void EscapeStringForDiagnostic(StringRef Str, SmallVectorImpl<char> &OutStr);
 } // namespace clang
 
diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td b/clang/include/clang/Basic/DiagnosticDriverKinds.td
index 68722ad9633120..cbad93904a899f 100644
--- a/clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ b/clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -831,4 +831,7 @@ def err_drv_triple_version_invalid : Error<
 
 def warn_missing_include_dirs : Warning<
   "no such include directory: '%0'">, InGroup<MissingIncludeDirs>, DefaultIgnore;
+
+def err_drv_malformed_warning_suppression_mapping : Error<
+  "failed to process suppression mapping file '%0' : %1">;
 }
diff --git a/clang/include/clang/Basic/DiagnosticOptions.h b/clang/include/clang/Basic/DiagnosticOptions.h
index 30141c2b8f4475..0770805624cec8 100644
--- a/clang/include/clang/Basic/DiagnosticOptions.h
+++ b/clang/include/clang/Basic/DiagnosticOptions.h
@@ -108,6 +108,9 @@ class DiagnosticOptions : public RefCountedBase<DiagnosticOptions>{
   /// The file to serialize diagnostics to (non-appending).
   std::string DiagnosticSerializationFile;
 
+  /// File that defines suppression mappings.
+  std::string SuppressionMappingsFile;
+
   /// The list of -W... options used to alter the diagnostic mappings, with the
   /// prefixes removed.
   std::vector<std::string> Warnings;
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 379e75b197cf96..827126f5af30ca 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -8979,3 +8979,8 @@ def wasm_opt : Flag<["--"], "wasm-opt">,
   Group<m_Group>,
   HelpText<"Enable the wasm-opt optimizer (default)">,
   MarshallingInfoNegativeFlag<LangOpts<"NoWasmOpt">>;
+
+def warning_suppression_mappings_EQ : Joined<["--"],
+  "warning-suppression-mappings=">, Group<Diag_Group>,
+  HelpText<"File containing list of sources to suppress diagnostics for. See XXX for format">,
+  Visibility<[ClangOption, CC1Option]>;
diff --git a/clang/include/clang/Frontend/CompilerInstance.h b/clang/include/clang/Frontend/CompilerInstance.h
index 3464654284f199..338eb3c6bd028d 100644
--- a/clang/include/clang/Frontend/CompilerInstance.h
+++ b/clang/include/clang/Frontend/CompilerInstance.h
@@ -24,6 +24,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/BuryPointer.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include <cassert>
 #include <list>
 #include <memory>
@@ -701,11 +702,10 @@ class CompilerInstance : public ModuleLoader {
   /// used by some diagnostics printers (for logging purposes only).
   ///
   /// \return The new object on success, or null on failure.
-  static IntrusiveRefCntPtr<DiagnosticsEngine>
-  createDiagnostics(DiagnosticOptions *Opts,
-                    DiagnosticConsumer *Client = nullptr,
-                    bool ShouldOwnClient = true,
-                    const CodeGenOptions *CodeGenOpts = nullptr);
+  static IntrusiveRefCntPtr<DiagnosticsEngine> createDiagnostics(
+      DiagnosticOptions *Opts, DiagnosticConsumer *Client = nullptr,
+      bool ShouldOwnClient = true, const CodeGenOptions *CodeGenOpts = nullptr,
+      IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS = nullptr);
 
   /// Create the file manager and replace any existing one with it.
   ///
diff --git a/clang/lib/Basic/Diagnostic.cpp b/clang/lib/Basic/Diagnostic.cpp
index e23362fc7af00d..e536effd25072f 100644
--- a/clang/lib/Basic/Diagnostic.cpp
+++ b/clang/lib/Basic/Diagnostic.cpp
@@ -21,6 +21,7 @@
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Basic/TokenKinds.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
@@ -28,6 +29,7 @@
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/CrashRecoveryContext.h"
 #include "llvm/Support/Unicode.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
@@ -477,6 +479,16 @@ void DiagnosticsEngine::setSeverityForAll(diag::Flavor Flavor,
       setSeverity(Diag, Map, Loc);
 }
 
+void DiagnosticsEngine::setDiagSuppressionMapping(
+    decltype(DiagSuppressionMapping) Mapping) {
+  DiagSuppressionMapping = std::move(Mapping);
+}
+
+bool DiagnosticsEngine::isSuppressedViaMapping(diag::kind D,
+                                               llvm::StringRef FilePath) const {
+  return DiagSuppressionMapping && DiagSuppressionMapping(D, FilePath);
+}
+
 void DiagnosticsEngine::Report(const StoredDiagnostic &storedDiag) {
   DiagnosticStorage DiagStorage;
   DiagStorage.DiagRanges.append(storedDiag.range_begin(),
diff --git a/clang/lib/Basic/DiagnosticIDs.cpp b/clang/lib/Basic/DiagnosticIDs.cpp
index d45bb0f392d457..4f213e02fa9324 100644
--- a/clang/lib/Basic/DiagnosticIDs.cpp
+++ b/clang/lib/Basic/DiagnosticIDs.cpp
@@ -575,6 +575,12 @@ DiagnosticIDs::getDiagnosticSeverity(unsigned DiagID, SourceLocation Loc,
       DiagID != diag::fatal_too_many_errors && Diag.FatalsAsError)
     Result = diag::Severity::Error;
 
+  // Rest of the mappings are only applicable for diagnostics associated with a
+  // SourceLocation, bail out early for others.
+  if (!Diag.hasSourceManager())
+    return Result;
+
+  const auto &SM = Diag.getSourceManager();
   // Custom diagnostics always are emitted in system headers.
   bool ShowInSystemHeader =
       !GetDiagInfo(DiagID) || GetDiagInfo(DiagID)->WarnShowInSystemHeader;
@@ -582,18 +588,29 @@ DiagnosticIDs::getDiagnosticSeverity(unsigned DiagID, SourceLocation Loc,
   // 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.
-  if (State->SuppressSystemWarnings && !ShowInSystemHeader && Loc.isValid() &&
-      Diag.getSourceManager().isInSystemHeader(
-          Diag.getSourceManager().getExpansionLoc(Loc)))
+  if (State->SuppressSystemWarnings && !ShowInSystemHeader &&
+      SM.isInSystemHeader(SM.getExpansionLoc(Loc)))
     return diag::Severity::Ignored;
 
   // We also ignore warnings due to system macros
   bool ShowInSystemMacro =
       !GetDiagInfo(DiagID) || GetDiagInfo(DiagID)->WarnShowInSystemMacro;
-  if (State->SuppressSystemWarnings && !ShowInSystemMacro && Loc.isValid() &&
-      Diag.getSourceManager().isInSystemMacro(Loc))
+  if (State->SuppressSystemWarnings && !ShowInSystemMacro &&
+      SM.isInSystemMacro(Loc))
     return diag::Severity::Ignored;
 
+  // Apply suppression mappings if severity wasn't explicitly mapped with a
+  // clang-diagnostics pragma to ensure pragmas always take precedence over
+  // mappings.
+  // We also use presumed locations here to improve reproducibility for
+  // preprocessed inputs.
+  if (!Mapping.isPragma()) {
+    if (auto PLoc = SM.getPresumedLoc(Loc);
+        PLoc.isValid() &&
+        Diag.isSuppressedViaMapping(DiagID, PLoc.getFilename()))
+      return diag::Severity::Ignored;
+  }
+
   return Result;
 }
 
diff --git a/clang/lib/Basic/Warnings.cpp b/clang/lib/Basic/Warnings.cpp
index 5a5ac555633886..1047980b84d7db 100644
--- a/clang/lib/Basic/Warnings.cpp
+++ b/clang/lib/Basic/Warnings.cpp
@@ -23,10 +23,21 @@
 // simpler because a remark can't be promoted to an error.
 #include "clang/Basic/AllDiagnostics.h"
 #include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticDriver.h"
+#include "clang/Basic/DiagnosticIDs.h"
 #include "clang/Basic/DiagnosticOptions.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringMapEntry.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/SpecialCaseList.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include <algorithm>
 #include <cstring>
+#include <memory>
 #include <utility>
+#include <vector>
 using namespace clang;
 
 // EmitUnknownDiagWarning - Emit a warning and typo hint for unknown warning
@@ -41,8 +52,96 @@ static void EmitUnknownDiagWarning(DiagnosticsEngine &Diags,
       << (Prefix.str() += std::string(Suggestion));
 }
 
+namespace {
+class WarningsSpecialCaseList : public llvm::SpecialCaseList {
+public:
+  static std::unique_ptr<WarningsSpecialCaseList>
+  create(const llvm::MemoryBuffer &MB, std::string &Err) {
+    auto SCL = std::make_unique<WarningsSpecialCaseList>();
+    if (SCL->createInternal(&MB, Err))
+      return SCL;
+    return nullptr;
+  }
+
+  // Section names refer to diagnostic groups, which cover multiple individual
+  // diagnostics. Expand diagnostic groups here to individual diagnostics.
+  // A diagnostic can have multiple diagnostic groups associated with it, we let
+  // the last section take precedence in such cases.
+  void processSections(DiagnosticsEngine &Diags) {
+    // Drop the default section introduced by special case list, we only support
+    // exact diagnostic group names.
+    Sections.erase("*");
+    // Make sure we iterate sections by their line numbers.
+    std::vector<std::pair<unsigned, const llvm::StringMapEntry<Section> *>>
+        LineAndSection;
+    for (const auto &Entry : Sections) {
+      LineAndSection.emplace_back(
+          Entry.second.SectionMatcher->Globs.at(Entry.first()).second, &Entry);
+    }
+    llvm::sort(LineAndSection);
+    for (const auto &[_, Entry] : LineAndSection) {
+      SmallVector<diag::kind, 256> GroupDiags;
+      if (Diags.getDiagnosticIDs()->getDiagnosticsInGroup(
+              clang::diag::Flavor::WarningOrError, Entry->first(),
+              GroupDiags)) {
+        EmitUnknownDiagWarning(Diags, clang::diag::Flavor::WarningOrError, "",
+                               Entry->first());
+        continue;
+      }
+      for (auto D : GroupDiags)
+        DiagToSection[D] = &Entry->getValue();
+    }
+  }
+
+  bool isDiagSuppressed(diag::kind D, llvm::StringRef FilePath) const {
+    auto Section = DiagToSection.find(D);
+    if (Section == DiagToSection.end())
+      return false;
+    auto SrcEntries = Section->second->Entries.find("src");
+    if(SrcEntries == Section->second->Entries.end())
+      return false;
+    // Find the longest glob pattern that matches FilePath. A positive match
+    // implies D should be suppressed for FilePath.
+    llvm::StringRef LongestMatch;
+    bool LongestWasNegative;
+    for (const auto &CatIt : SrcEntries->second) {
+      bool IsNegative = CatIt.first() == "emit";
+      for (const auto &GlobIt : CatIt.second.Globs) {
+        if (GlobIt.getKeyLength() < LongestMatch.size())
+          continue;
+        if (!GlobIt.second.first.match(FilePath))
+          continue;
+        LongestMatch = GlobIt.getKey();
+        LongestWasNegative = IsNegative;
+      }
+    }
+    return !LongestMatch.empty() && !LongestWasNegative;
+  }
+
+private:
+  llvm::DenseMap<diag::kind, const Section*> DiagToSection;
+};
+
+void parseSuppressionMappings(const llvm::MemoryBuffer &MB,
+                              DiagnosticsEngine &Diags) {
+  std::string Err;
+  auto SCL = WarningsSpecialCaseList::create(MB, Err);
+  if (!SCL) {
+    Diags.Report(diag::err_drv_malformed_warning_suppression_mapping)
+        << MB.getBufferIdentifier() << Err;
+    return;
+  }
+  SCL->processSections(Diags);
+  Diags.setDiagSuppressionMapping(
+      [SCL(std::move(SCL))](diag::kind K, llvm::StringRef Path) {
+        return SCL->isDiagSuppressed(K, Path);
+      });
+}
+} // namespace
+
 void clang::ProcessWarningOptions(DiagnosticsEngine &Diags,
                                   const DiagnosticOptions &Opts,
+                                  llvm::vfs::FileSystem& VFS,
                                   bool ReportDiags) {
   Diags.setSuppressSystemWarnings(true);  // Default to -Wno-system-headers
   Diags.setIgnoreAllWarnings(Opts.IgnoreWarnings);
@@ -70,6 +169,14 @@ void clang::ProcessWarningOptions(DiagnosticsEngine &Diags,
   else
     Diags.setExtensionHandlingBehavior(diag::Severity::Ignored);
 
+  if (!Opts.SuppressionMappingsFile.empty()) {
+    if (auto Buf = VFS.getBufferForFile(Opts.SuppressionMappingsFile)) {
+      parseSuppressionMappings(**Buf, Diags);
+    } else if (ReportDiags) {
+      Diags.Report(diag::err_drv_no_such_file) << Opts.SuppressionMappingsFile;
+    }
+  }
+
   SmallVector<diag::kind, 10> _Diags;
   const IntrusiveRefCntPtr< DiagnosticIDs > DiagIDs =
     Diags.getDiagnosticIDs();
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 3fc39296f44281..55ec543c5de514 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -4456,6 +4456,13 @@ static void RenderDiagnosticsOptions(const Driver &D, const ArgList &Args,
 
   Args.addOptOutFlag(CmdArgs, options::OPT_fspell_checking,
                      options::OPT_fno_spell_checking);
+
+  if (const Arg *A =
+          Args.getLastArg(options::OPT_warning_suppression_mappings_EQ)) {
+    if (!D.getVFS().exists(A->getValue()))
+      D.Diag(clang::diag::err_drv_no_such_file) << A->getValue();
+    CmdArgs.push_back(Args.MakeArgString(A->getSpelling() + A->getValue()));
+  }
 }
 
 DwarfFissionKind tools::getDebugFissionKind(const Driver &D,
diff --git a/clang/lib/Frontend/ASTUnit.cpp b/clang/lib/Frontend/ASTUnit.cpp
index bffff0d27af3ab..464ed8090a0ec5 100644
--- a/clang/lib/Frontend/ASTUnit.cpp
+++ b/clang/lib/Frontend/ASTUnit.cpp
@@ -1367,7 +1367,7 @@ ASTUnit::getMainBufferWithPrecompiledPreamble(
       // after parsing the preamble.
       getDiagnostics().Reset();
       ProcessWarningOptions(getDiagnostics(),
-                            PreambleInvocationIn.getDiagnosticOpts());
+                            PreambleInvocationIn.getDiagnosticOpts(), *VFS);
       getDiagnostics().setNumWarnings(NumWarningsInPreamble);
 
       PreambleRebuildCountdown = 1;
@@ -1593,7 +1593,8 @@ ASTUnit *ASTUnit::LoadFromCompilerInvocationAction(
   // We'll manage file buffers ourselves.
   CI->getPreprocessorOpts().RetainRemappedFileBuffers = true;
   CI->getFrontendOpts().DisableFree = false;
-  ProcessWarningOptions(AST->getDiagnostics(), CI->getDiagnosticOpts());
+  ProcessWarningOptions(AST->getDiagnostics(), CI->getDiagnosticOpts(),
+                        AST->getFileManager().getVirtualFileSystem());
 
   // Create the compiler instance to use for building the AST.
   std::unique_ptr<CompilerInstance> Clang(
@@ -1701,7 +1702,7 @@ bool ASTUnit::LoadFromCompilerInvocation(
   Invocation->getPreprocessorOpts().RetainRemappedFileBuffers = true;
   Invocation->getFrontendOpts().DisableFree = false;
   getDiagnostics().Reset();
-  ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts());
+  ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts(), *VFS);
 
   std::unique_ptr<llvm::MemoryBuffer> OverrideMainBuffer;
   if (PrecompilePreambleAfterNParses > 0) {
@@ -1709,7 +1710,7 @@ bool ASTUnit::LoadFromCompilerInvocation(
     OverrideMainBuffer =
         getMainBufferWithPrecompiledPreamble(PCHContainerOps, *Invocation, VFS);
     getDiagnostics().Reset();
-    ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts());
+    ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts(), *VFS);
   }
 
   SimpleTimer ParsingTimer(WantTiming);
@@ -1902,7 +1903,8 @@ bool ASTUnit::Reparse(std::shared_ptr<PCHContainerOperations> PCHContainerOps,
   // Clear out the diagnostics state.
   FileMgr.reset();
   getDiagnostics().Reset();
-  ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts());
+  ProcessWarningOptions(getDiagnostics(), Invocation->getDiagnosticOpts(),
+                        *VFS);
   if (OverrideMainBuffer)
     getDiagnostics().setNumWarnings(NumWarningsInPreamble);
 
@@ -2241,7 +2243,8 @@ void ASTUnit::CodeComplete(
   CaptureDroppedDiagnostics Capture(CaptureDiagsKind::All,
                                     Clang->getDiagnostics(),
                                     &StoredDiagnostics, nullptr);
-  ProcessWarningOptions(Diag, Inv.getDiagnosticOpts());
+  ProcessWarningOptions(Diag, Inv.getDiagnosticOpts(),
+                        FileMgr.getVirtualFileSystem());
 
   // Create the target instance.
   if (!Clang->createTarget()) {
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index 240305b33824b8..ecc6782c7cb4fb 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -39,6 +39,7 @@
 #include "clang/Serialization/ASTReader.h"
 #include "clang/Serialization/GlobalModuleIndex.h"
 #include "clang/Serialization/InMemoryModuleCache.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/Statistic.h"
@@ -54,6 +55,7 @@
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/TimeProfiler.h"
 #include "llvm/Support/Timer.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/TargetParser/Host.h"
 #include <optional>
@@ -332,19 +334,22 @@ static void SetupSerializedDiagnostics(DiagnosticOptions *DiagOpts,
 
 void CompilerInstance::createDiagnostics(DiagnosticConsumer *Client,
                                          bool ShouldOwnClient) {
-  Diagnostics = createDiagnostics(&getDiagnosticOpts(), Cl...
[truncated]

Copy link

github-actions bot commented Oct 16, 2024

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

Copy link
Contributor

@ilya-biryukov ilya-biryukov left a comment

Choose a reason for hiding this comment

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

I have only done a quick pass, but still wanted to leave a few NITs.

@kadircet
Copy link
Member Author

gentle ping @AaronBallman, I believe you were supportive of this approach in final version of RFC but I'd like to make sure this agreement was implemented in an accurate manner.

So it'd be great if you can check the overall interaction in https://github.com/llvm/llvm-project/blob/61b963131975ca3f6536b38427e25024f8759c57/clang/test/Misc/warning-suppression-mappings.cpp and https://github.com/llvm/llvm-project/blob/61b963131975ca3f6536b38427e25024f8759c57/clang/test/Misc/Inputs/suppression-mapping.txt. If those look good, I am happy to settle on the details of the implementation with other reviewers.

@bricknerb bricknerb self-requested a review October 24, 2024 14:54
@bricknerb
Copy link
Contributor

Add that change to clang/docs/ReleaseNotes.rst?

@kadircet
Copy link
Member Author

kadircet commented Nov 8, 2024

thanks for the review!

@AaronBallman i'd like to land this early next week and start testing/polishing. It'd be great if you can give some explicit LGTM.

@kadircet
Copy link
Member Author

@AaronBallman merging this to main now. PLMK if you have any concerns, I am happy to address them post-submit (or revert if need be)

@kadircet kadircet merged commit 41e3919 into llvm:main Nov 12, 2024
6 of 9 checks passed
@kadircet kadircet deleted the ratcheting_up branch November 12, 2024 09:53
@Michael137
Copy link
Member

Michael137 commented Nov 12, 2024

FYI, looks like this is causing following LLDB tests to fail: https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/as-lldb-cmake/15106/execution/node/97/log/

Unresolved Tests (17):
  lldb-api :: commands/expression/import-std-module/array/TestArrayFromStdModule.py
  lldb-api :: commands/expression/import-std-module/deque-basic/TestDequeFromStdModule.py
  lldb-api :: commands/expression/import-std-module/deque-dbg-info-content/TestDbgInfoContentDequeFromStdModule.py
  lldb-api :: commands/expression/import-std-module/forward_list-dbg-info-content/TestDbgInfoContentForwardListFromStdModule.py
  lldb-api :: commands/expression/import-std-module/forward_list/TestForwardListFromStdModule.py
  lldb-api :: commands/expression/import-std-module/iterator/TestIteratorFromStdModule.py
  lldb-api :: commands/expression/import-std-module/list-dbg-info-content/TestDbgInfoContentListFromStdModule.py
  lldb-api :: commands/expression/import-std-module/list/TestListFromStdModule.py
  lldb-api :: commands/expression/import-std-module/non-module-type-separation/TestNonModuleTypeSeparation.py
  lldb-api :: commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py
  lldb-api :: commands/expression/import-std-module/shared_ptr-dbg-info-content/TestSharedPtrDbgInfoContentFromStdModule.py
  lldb-api :: commands/expression/import-std-module/shared_ptr/TestSharedPtrFromStdModule.py
  lldb-api :: commands/expression/import-std-module/unique_ptr-dbg-info-content/TestUniquePtrDbgInfoContent.py
  lldb-api :: commands/expression/import-std-module/unique_ptr/TestUniquePtrFromStdModule.py
  lldb-api :: commands/expression/import-std-module/vector-of-vectors/TestVectorOfVectorsFromStdModule.py
  lldb-api :: commands/expression/import-std-module/weak_ptr-dbg-info-content/TestDbgInfoContentWeakPtrFromStdModule.py
  lldb-api :: commands/expression/import-std-module/weak_ptr/TestWeakPtrFromStdModule.py

Example assertion:

Assertion failed: (0 && "Invalid SLocOffset or bad function choice"), function getFileIDLoaded, file SourceManager.cpp, line 867.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
``

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

@AaronBallman merging this to main now. PLMK if you have any concerns, I am happy to address them post-submit (or revert if need be)

Sorry, there was a US holiday on Monday and I was mostly out on Friday, so that's why there was a delay in review. In the future, I would recommend waiting before landing significant changes like these unless the review is accepted by a maintainer or someone known to be familiar with that area of the code. That reduces pressure on reviewers and allows folks to be sick/take vacations/etc without things landing out from under them. As it turns out, I can no longer leave suggested changes on the PR because it was already merged (thanks, GitHub....), so my review comments will be somewhat awkward.


Warning suppression mappings enable users to suppress Clang's diagnostics in a
per-file granular manner. Enabling enforcement of diagnostics in specific parts
of the project, even if there are violations in some headers.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove comma: project even if there
Reword: some headers -> some headers which cannot be modified.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reword: some headers -> some headers which cannot be modified.

But this covers headers even inside the project (i.e. the incremental cleanup of a new warning). So I feel like which cannot be modified. adds a non-existing limitation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's true, but I think we want to guide users towards fixing diagnostics rather than suppressing them. Users will use any suppression mechanisms we provide regardless of how we document it, so I don't think it's harmful to imply this is more about cases where you can't modify the code. But I also don't feel strongly. WDYT?

@@ -9017,3 +9017,8 @@ def wasm_opt : Flag<["--"], "wasm-opt">,
Group<m_Group>,
HelpText<"Enable the wasm-opt optimizer (default)">,
MarshallingInfoNegativeFlag<LangOpts<"NoWasmOpt">>;

def warning_suppression_mappings_EQ : Joined<["--"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should move elsewhere in Options.td, it's currently listed under clang-dxc options.

Also, should this be exposed in clang-cl as well? What about flang? CC @zmodem @rnk @MaskRay @jansvoboda11 for opinions

Copy link
Member Author

@kadircet kadircet Nov 13, 2024

Choose a reason for hiding this comment

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

This should move elsewhere in Options.td, it's currently listed under clang-dxc options.

Oh didn't notice we actually had "lexical" sections in the doc (AFAICT it doesn't have any semantic affect afterwards, flag is still only visible for cc1 and clang-driver). Moved it near other warning options.

Also, should this be exposed in clang-cl as well? What about flang? CC @zmodem @rnk @MaskRay @jansvoboda11 for opinions

Going to carry this discussion into RFC to make sure it doesn't get lost. https://discourse.llvm.org/t/rfc-add-support-for-controlling-diagnostics-severities-at-file-level-granularity-through-command-line/81292/21?u=kadircet

@kadircet
Copy link
Member Author

FYI, looks like this is causing following LLDB tests to fail: https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/as-lldb-cmake/15106/execution/node/97/log/

Unresolved Tests (17):
  lldb-api :: commands/expression/import-std-module/array/TestArrayFromStdModule.py
  lldb-api :: commands/expression/import-std-module/deque-basic/TestDequeFromStdModule.py
  lldb-api :: commands/expression/import-std-module/deque-dbg-info-content/TestDbgInfoContentDequeFromStdModule.py
  lldb-api :: commands/expression/import-std-module/forward_list-dbg-info-content/TestDbgInfoContentForwardListFromStdModule.py
  lldb-api :: commands/expression/import-std-module/forward_list/TestForwardListFromStdModule.py
  lldb-api :: commands/expression/import-std-module/iterator/TestIteratorFromStdModule.py
  lldb-api :: commands/expression/import-std-module/list-dbg-info-content/TestDbgInfoContentListFromStdModule.py
  lldb-api :: commands/expression/import-std-module/list/TestListFromStdModule.py
  lldb-api :: commands/expression/import-std-module/non-module-type-separation/TestNonModuleTypeSeparation.py
  lldb-api :: commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py
  lldb-api :: commands/expression/import-std-module/shared_ptr-dbg-info-content/TestSharedPtrDbgInfoContentFromStdModule.py
  lldb-api :: commands/expression/import-std-module/shared_ptr/TestSharedPtrFromStdModule.py
  lldb-api :: commands/expression/import-std-module/unique_ptr-dbg-info-content/TestUniquePtrDbgInfoContent.py
  lldb-api :: commands/expression/import-std-module/unique_ptr/TestUniquePtrFromStdModule.py
  lldb-api :: commands/expression/import-std-module/vector-of-vectors/TestVectorOfVectorsFromStdModule.py
  lldb-api :: commands/expression/import-std-module/weak_ptr-dbg-info-content/TestDbgInfoContentWeakPtrFromStdModule.py
  lldb-api :: commands/expression/import-std-module/weak_ptr/TestWeakPtrFromStdModule.py

Example assertion:

Assertion failed: (0 && "Invalid SLocOffset or bad function choice"), function getFileIDLoaded, file SourceManager.cpp, line 867.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
``

FWIW, I am still trying to reproduce these failures. It seems to be hard on a linux machine as tests are all disabled with a link to https://discourse.llvm.org/t/lldb-test-failures-on-linux/80095.

@nikic
Copy link
Contributor

nikic commented Nov 12, 2024

It looks like this increases time to build clang by 0.5% (see affected files here: https://llvm-compile-time-tracker.com/compare_clang.php?from=e385e0d3e71e17da0b2023f480259c95923707bd&to=41e3919ded78d8870f7c95e9181c7f7e29aa3cc4&stat=instructions%3Au&sortBy=interestingness)

This usually indicates that you are including something expensive in a core header -- it would be good to check whether it can be avoided.

@nikic
Copy link
Contributor

nikic commented Nov 12, 2024

Though looking at that file list in more details, this also affects files outside clang, so I think it might be adding some compile-time overhead that is only visible on specific workloads. E.g. LLVMSupport.dir/UnicodeNameToCodepointGenerated.cpp has a 4.5% increase.

@Michael137
Copy link
Member

FYI, looks like this is causing following LLDB tests to fail: https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/as-lldb-cmake/15106/execution/node/97/log/

Unresolved Tests (17):
  lldb-api :: commands/expression/import-std-module/array/TestArrayFromStdModule.py
  lldb-api :: commands/expression/import-std-module/deque-basic/TestDequeFromStdModule.py
  lldb-api :: commands/expression/import-std-module/deque-dbg-info-content/TestDbgInfoContentDequeFromStdModule.py
  lldb-api :: commands/expression/import-std-module/forward_list-dbg-info-content/TestDbgInfoContentForwardListFromStdModule.py
  lldb-api :: commands/expression/import-std-module/forward_list/TestForwardListFromStdModule.py
  lldb-api :: commands/expression/import-std-module/iterator/TestIteratorFromStdModule.py
  lldb-api :: commands/expression/import-std-module/list-dbg-info-content/TestDbgInfoContentListFromStdModule.py
  lldb-api :: commands/expression/import-std-module/list/TestListFromStdModule.py
  lldb-api :: commands/expression/import-std-module/non-module-type-separation/TestNonModuleTypeSeparation.py
  lldb-api :: commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py
  lldb-api :: commands/expression/import-std-module/shared_ptr-dbg-info-content/TestSharedPtrDbgInfoContentFromStdModule.py
  lldb-api :: commands/expression/import-std-module/shared_ptr/TestSharedPtrFromStdModule.py
  lldb-api :: commands/expression/import-std-module/unique_ptr-dbg-info-content/TestUniquePtrDbgInfoContent.py
  lldb-api :: commands/expression/import-std-module/unique_ptr/TestUniquePtrFromStdModule.py
  lldb-api :: commands/expression/import-std-module/vector-of-vectors/TestVectorOfVectorsFromStdModule.py
  lldb-api :: commands/expression/import-std-module/weak_ptr-dbg-info-content/TestDbgInfoContentWeakPtrFromStdModule.py
  lldb-api :: commands/expression/import-std-module/weak_ptr/TestWeakPtrFromStdModule.py

Example assertion:

Assertion failed: (0 && "Invalid SLocOffset or bad function choice"), function getFileIDLoaded, file SourceManager.cpp, line 867.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
``

FWIW, I am still trying to reproduce these failures. It seems to be hard on a linux machine as tests are all disabled with a link to https://discourse.llvm.org/t/lldb-test-failures-on-linux/80095.

AFAIK only a few of these are skipped on Linux. E.g., TestIteratorFromStdModule.py should still reproduce. Happy to help you reproduce/narrow this down

@Michael137
Copy link
Member

Hmm this one's a bit strange. Looks like the issue is in:

// Clang-diagnostics pragmas always take precedence over suppression mapping.
if (!Mapping.isPragma()) {                                                   
  // We also use presumed locations here to improve reproducibility for      
  // preprocessed inputs.                                                    
  if (PresumedLoc PLoc = SM.getPresumedLoc(Loc);                             
      PLoc.isValid() && Diag.isSuppressedViaMapping(                         
                            DiagID, llvm::sys::path::remove_leading_dotslash(
                                        PLoc.getFilename())))                
    return diag::Severity::Ignored;                                          
}                                                                            

The assert triggers inside getPresumedLoc. So doesn't look like it's an inherent issue with the patch. But it seems to be breaking assumptions that LLDB is making about these source locations

@kadircet
Copy link
Member Author

so 12e3ed8 should resolve both lldb buildbot breakage and compile time regression in clang.

that being said, I don't think 12e3ed8
is the right fix for lldb breakage. I think this change reveals an actual bug in lldb's handling of modules, but I can't repro locally. I hit a similar issue with clang-repl during development, you can find the relevant fix in https://github.com/llvm/llvm-project/pull/112517/files#diff-23c4b4ecf706e1f0b594e365095a343594e3978b47991718782e1f76064f241a.

Basically I think LLDB is doing something similar, it's probably loading/building modules with one SourceManager and then it's trying to consume them using another. @Michael137, If you can provide more detailed instructions about how to set up a similar testing environment in linux (I couldn't get the std-modules bit to work), or if you can provide a detailed stack trace from assertion failure, I can try to dig deeper.

@Michael137
Copy link
Member

Michael137 commented Nov 12, 2024

so 12e3ed8 should resolve both lldb buildbot breakage and compile time regression in clang.

that being said, I don't think 12e3ed8 is the right fix for lldb breakage. I think this change reveals an actual bug in lldb's handling of modules, but I can't repro locally. I hit a similar issue with clang-repl during development, you can find the relevant fix in https://github.com/llvm/llvm-project/pull/112517/files#diff-23c4b4ecf706e1f0b594e365095a343594e3978b47991718782e1f76064f241a.

Basically I think LLDB is doing something similar, it's probably loading/building modules with one SourceManager and then it's trying to consume them using another. @Michael137, If you can provide more detailed instructions about how to set up a similar testing environment in linux (I couldn't get the std-modules bit to work), or if you can provide a detailed stack trace from assertion failure, I can try to dig deeper.

Awesome thanks for the quick workaround! I agree, LLDB is doing something funky here. Example backtrace for the assertion:

frame #19: 0x000000012919b5a4 liblldb.20.0.0git.dylib`IsTemplateDeclCommonStructurallyEquivalent(Ctx=0x000000016fdeef40, D1=0x0000000107a49230, D2=0x0000000107a48dc0) at ASTStructuralEquivalence.cpp:2073:10
frame #18: 0x000000012919b278 liblldb.20.0.0git.dylib`IsStructurallyEquivalent(Context=0x000000016fdeef40, Params1=0x0000000107a49d98, Params2=0x0000000107a48f10) at ASTStructuralEquivalence.cpp:1964:7
frame #17: 0x000000012381b288 liblldb.20.0.0git.dylib`clang::DiagnosticBuilder::~DiagnosticBuilder(this=0x000000016fdeec80) at Diagnostic.h:1313:24
frame #16: 0x0000000123822bfc liblldb.20.0.0git.dylib`clang::DiagnosticBuilder::~DiagnosticBuilder(this=0x000000016fdeec80) at Diagnostic.h:1313:26
frame #15: 0x0000000123822c74 liblldb.20.0.0git.dylib`clang::DiagnosticBuilder::Emit(this=0x000000016fdeec80) at Diagnostic.h:1278:28
frame #14: 0x0000000129b98b58 liblldb.20.0.0git.dylib`clang::DiagnosticsEngine::EmitDiagnostic(this=0x0000000113ec4400, DB=0x000000016fdeec80, Force=false) at Diagnostic.cpp:658:15
frame #13: 0x0000000129b98be4 liblldb.20.0.0git.dylib`clang::DiagnosticsEngine::ProcessDiag(this=0x0000000113ec4400, DiagBuilder=0x000000016fdeec80) at Diagnostic.h:1034:19
frame #12: 0x0000000129baefc0 liblldb.20.0.0git.dylib`clang::DiagnosticIDs::ProcessDiag(this=0x0000600000af0d60, Diag=0x0000000113ec4400, DiagBuilder=0x000000016fdeec80) const at DiagnosticIDs.cpp:777:7
frame #11: 0x0000000129badb98 liblldb.20.0.0git.dylib`clang::DiagnosticIDs::getDiagnosticLevel(this=0x0000600000af0d60, DiagID=2351, Loc=(ID = 2093809277), Diag=0x0000000113ec4400) const at DiagnosticIDs.cpp:501:18
frame #10: 0x0000000129bae270 liblldb.20.0.0git.dylib`clang::DiagnosticIDs::getDiagnosticSeverity(this=0x0000600000af0d60, DiagID=2351, Loc=(ID = 2093809277), Diag=0x0000000113ec4400) const at DiagnosticIDs.cpp:607:31
frame #9: 0x0000000129c0797c liblldb.20.0.0git.dylib`clang::SourceManager::getPresumedLoc(this=0x0000000112f3f730, Loc=(ID = 2093809277), UseLineDirectives=true) const at SourceManager.cpp:1463:41
frame #8: 0x000000012646726c liblldb.20.0.0git.dylib`clang::SourceManager::getDecomposedExpansionLoc(this=0x0000000112f3f730, Loc=(ID = 2093809277)) const at SourceManager.h:1286:18
frame #7: 0x0000000123fd5410 liblldb.20.0.0git.dylib`clang::SourceManager::getFileID(this=0x0000000112f3f730, SpellingLoc=(ID = 2093809277)) const at SourceManager.h:1146:12
frame #6: 0x0000000123ffaa20 liblldb.20.0.0git.dylib`clang::SourceManager::getFileID(this=0x0000000112f3f730, SLocOffset=2093809277) const at SourceManager.h:1901:12
frame #5: 0x0000000129c05b80 liblldb.20.0.0git.dylib`clang::SourceManager::getFileIDSlow(this=0x0000000112f3f730, SLocOffset=2093809277) const at SourceManager.cpp:778:10
frame #4: 0x0000000129c05ed4 liblldb.20.0.0git.dylib`clang::SourceManager::getFileIDLoaded(this=0x0000000112f3f730, SLocOffset=2093809277) const at SourceManager.cpp:867:5
frame #3: 0x0000000183ef8c1c libsystem_c.dylib`__assert_rtn + 284
frame #2: 0x0000000183ef9908 libsystem_c.dylib`abort + 128
frame #1: 0x0000000183fecf50 libsystem_pthread.dylib`pthread_kill + 288
frame #0: 0x0000000183fb4600 libsystem_kernel.dylib`__pthread_kill + 8
(lldb) f
frame #4: 0x0000000129c05ed4 liblldb.20.0.0git.dylib`clang::SourceManager::getFileIDLoaded(this=0x0000000112f3f730, SLocOffset=2093809277) const at SourceManager.cpp:867:5
   857      LessIndex = MiddleIndex;
   858    }
   859  }
   860 
   861  /// Return the FileID for a SourceLocation with a high offset.
   862  ///
   863  /// This function knows that the SourceLocation is in a loaded buffer, not a
   864  /// local one.
   865  FileID SourceManager::getFileIDLoaded(SourceLocation::UIntTy SLocOffset) const {
   866    if (SLocOffset < CurrentLoadedOffset) {
-> 867      assert(0 && "Invalid SLocOffset or bad function choice");
   868      return FileID();
   869    }
   870 
   871    return FileID::get(ExternalSLocEntries->getSLocEntryID(SLocOffset));
   872  }
   873 
   874  SourceLocation SourceManager::
   875  getExpansionLocSlowCase(SourceLocation Loc) const {
   876    do {
   877      // Note: If Loc indicates an offset into a token that came from a macro
(lldb) v SLocOffset
(clang::SourceLocation::UIntTy) SLocOffset = 2093809277

(lldb) v CurrentLoadedOffset
(clang::SourceLocation::UIntTy) CurrentLoadedOffset = 2147483648

(lldb) p SourceManager::MaxLoadedOffset
(const clang::SourceLocation::UIntTy) 2147483648

Not sure if that's enough to go on though. Let me know what else could be helpful here.

it's probably loading/building modules with one SourceManager and then it's trying to consume them using another

Yea that sounds entirely plausible.

@kadircet
Copy link
Member Author

Not sure if that's enough to go on though. Let me know what else could be helpful here.

Unfortunately it isn't, can you include all the frames? at least some call that originates from LLDB ?

@nikic
Copy link
Contributor

nikic commented Nov 12, 2024

so 12e3ed8 should resolve both lldb buildbot breakage and compile time regression in clang.

Thank you, I can confirm that this fixes the compile-time regression (or at least most of it).

@kadircet
Copy link
Member Author

@Michael137 looks like there's a new failure after 12e3ed8. Unfortunately I can't repro this one either, despite being a non-mac machine, the tests are run this time but they all pass :/ Also I do not see how that can be triggered after these patches.

I am out of time on my investigation for today, I'll revert the changes to not break head until tomorrow. But it'd be great if you can share any advise around reproducing these buildbots environment on other machines.

kadircet added a commit that referenced this pull request Nov 12, 2024
@kadircet
Copy link
Member Author

alright NVM, apparently that breakage was due to #107849. I'll still reland this tomorrow after addressing @AaronBallman's latest comments.

@AaronBallman
Copy link
Collaborator

Thank you for your quick attention to the issue!

searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Nov 12, 2024
relands:
eea8b44 [GVN] Handle empty attrs in Expression == (llvm#115761)
984bca9 [GVN][NewGVN] Take call attributes into account in expressions (llvm#114545)

Reverts: breaks comgr build
41e3919 [clang] Introduce diagnostics suppression mappings (llvm#112517)

Change-Id: I3189bf1b5d66b980d2d711c3fe1d5fe217699bb1
kadircet added a commit that referenced this pull request Nov 13, 2024
github-merge-queue bot pushed a commit to carbon-language/carbon-lang that referenced this pull request Nov 15, 2024
Includes updates to reflect these LLVM changes:
* llvm/llvm-project#112517
* llvm/llvm-project#113331

---------

Co-authored-by: Josh L <[email protected]>
Groverkss pushed a commit to iree-org/llvm-project that referenced this pull request Nov 15, 2024
This implements

https://discourse.llvm.org/t/rfc-add-support-for-controlling-diagnostics-severities-at-file-level-granularity-through-command-line/81292.

Users now can suppress warnings for certain headers by providing a
mapping with globs, a sample file looks like:
```
[unused]
src:*
src:*clang/*=emit
```

This will suppress warnings from `-Wunused` group in all files that
aren't under `clang/` directory. This mapping file can be passed to
clang via `--warning-suppression-mappings=foo.txt`.

At a high level, mapping file is stored in DiagnosticOptions and then
processed with rest of the warning flags when creating a
DiagnosticsEngine. This is a functor that uses SpecialCaseLists
underneath to match against globs coming from the mappings file.

This implies processing warning options now performs IO, relevant
interfaces are updated to take in a VFS, falling back to RealFileSystem
when one is not available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang Clang issues not falling into any other category clang-tidy clang-tools-extra llvm:support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants