[clang][modules] Add -Wsystem-headers-in-module=

[benlangmuir]

Summary

Add a way to enable -Wsystem-headers only for a specific module. This is useful for validating a module that would otherwise not see system header diagnostics without being flooded by diagnostics for unrelated headers/modules. It's relatively common for a module to be marked [system] but still wish to validate itself explicitly.

[jansvoboda11]

LGTM

clang/include/clang/Basic/DiagnosticOptions.h

128

Out of interest, is there an existing use-case for having multiple modules here, or is this just future-proofing?

clang/lib/Frontend/CompilerInstance.cpp

1239–1240

I assume llvm::is_contained() wouldn't be okay with the std::string and StringRef mismatch?

[benlangmuir]

Use llvm::is_contained

[benlangmuir]

clang/include/clang/Basic/DiagnosticOptions.h

128

I'm not using it currently, but I think it makes sense to handle any number. You could have multiple such modules you're testing together.

clang/lib/Frontend/CompilerInstance.cpp

1239–1240

Nope, I just didn't think of it here for some reason. Updated.

[iana]

clang/include/clang/Basic/DiagnosticOptions.h

128

I'd imagine you'd typically have Framework and Framework_Private in there

[benlangmuir]

clang/include/clang/Basic/DiagnosticOptions.h

128

Ah, good point. I am in fact doing that already and just forgot.

clang/include/clang/Basic/DiagnosticOptions.h

public:
   /// The list of -R... options used to alter the diagnostic mappings, with the
   /// prefixes removed.
   std::vector<std::string> Remarks;
 
   /// The prefixes for comment directives sought by -verify ("expected" by
   /// default).
   std::vector<std::string> VerifyPrefixes;
 
+  /// The list of -Wsystem-header-in-module=... options used to override
+  /// whether -Wsystem-headers is enabled on a per-module basis.
+  std::vector<std::string> SystemHeaderWarningsModules;
+
 public:
   // Define accessors/mutators for diagnostic options of enumeration type.
 #define DIAGOPT(Name, Bits, Default)
 #define ENUM_DIAGOPT(Name, Type, Bits, Default) \
   Type get##Name() const { return static_cast<Type>(Name); } \
   void set##Name(Type Value) { Name = static_cast<unsigned>(Value); }
 #include "clang/Basic/DiagnosticOptions.def"
 

clang/include/clang/Driver/Options.td

 def V : JoinedOrSeparate<["-"], "V">, Flags<[NoXarchOption, Unsupported]>;
 def Wa_COMMA : CommaJoined<["-"], "Wa,">,
   HelpText<"Pass the comma separated arguments in <arg> to the assembler">,
   MetaVarName<"<arg>">;
 def Wall : Flag<["-"], "Wall">, Group<W_Group>, Flags<[CC1Option, HelpHidden]>;
 def WCL4 : Flag<["-"], "WCL4">, Group<W_Group>, Flags<[CC1Option, HelpHidden]>;
 def Wsystem_headers : Flag<["-"], "Wsystem-headers">, Group<W_Group>, Flags<[CC1Option, HelpHidden]>;
 def Wno_system_headers : Flag<["-"], "Wno-system-headers">, Group<W_Group>, Flags<[CC1Option, HelpHidden]>;
+def Wsystem_headers_in_module_EQ : Joined<["-"], "Wsystem-headers-in-module=">,
+  Flags<[CC1Option, HelpHidden]>, MetaVarName<"<module>">,
+  HelpText<"Enable -Wsystem-headers when building <module>">,
+  MarshallingInfoStringVector<DiagnosticOpts<"SystemHeaderWarningsModules">>;
 def Wdeprecated : Flag<["-"], "Wdeprecated">, Group<W_Group>, Flags<[CC1Option]>,
   HelpText<"Enable warnings for deprecated constructs and define __DEPRECATED">;
 def Wno_deprecated : Flag<["-"], "Wno-deprecated">, Group<W_Group>, Flags<[CC1Option]>;
 def Wl_COMMA : CommaJoined<["-"], "Wl,">, Flags<[LinkerInput, RenderAsInput]>,
   HelpText<"Pass the comma separated arguments in <arg> to the linker">,
   MetaVarName<"<arg>">, Group<Link_Group>;
 // FIXME: This is broken; these should not be Joined arguments.
 def Wno_nonportable_cfstrings : Joined<["-"], "Wno-nonportable-cfstrings">, Group<W_Group>,

clang/lib/Driver/ToolChains/Clang.cpp

if (A->getOption().getID() == options::OPT__SLASH_wd) {
         CmdArgs.push_back(Args.MakeArgString(
             "-Wno-" + DiagnosticIDs::getWarningOptionForGroup(*Group)));
       }
       continue;
     }
     A->render(Args, CmdArgs);
   }
 
+  Args.AddAllArgs(CmdArgs, options::OPT_Wsystem_headers_in_module_EQ);
+
   if (Args.hasFlag(options::OPT_pedantic, options::OPT_no_pedantic, false))
     CmdArgs.push_back("-pedantic");
   Args.AddLastArg(CmdArgs, options::OPT_pedantic_errors);
   Args.AddLastArg(CmdArgs, options::OPT_w);
 
   Args.addOptInFlag(CmdArgs, options::OPT_ffixed_point,
                     options::OPT_fno_fixed_point);
 

clang/lib/Frontend/CompilerInstance.cpp

 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "clang/Sema/Sema.h"
 #include "clang/Serialization/ASTReader.h"
 #include "clang/Serialization/GlobalModuleIndex.h"
 #include "clang/Serialization/InMemoryModuleCache.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Config/llvm-config.h"
 #include "llvm/Support/BuryPointer.h"
 #include "llvm/Support/CrashRecoveryContext.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/LockFileManager.h"
compileModuleImpl(CompilerInstance &ImportingInstance, SourceLocation ImportLoc,
   FrontendOpts.OriginalModuleMap = std::string(OriginalModuleMapFile);
   // Force implicitly-built modules to hash the content of the module file.
   HSOpts.ModulesHashContent = true;
   FrontendOpts.Inputs = {Input};
 
   // Don't free the remapped file buffers; they are owned by our caller.
   PPOpts.RetainRemappedFileBuffers = true;
 
-  Invocation->getDiagnosticOpts().VerifyDiagnostics = 0;
+  DiagnosticOptions &DiagOpts = Invocation->getDiagnosticOpts();
+
+  DiagOpts.VerifyDiagnostics = 0;
   assert(ImportingInstance.getInvocation().getModuleHash() ==
          Invocation->getModuleHash() && "Module hash mismatch!");
 
   // Construct a compiler instance that will be used to actually create the
   // module.  Since we're sharing an in-memory module cache,
   // CompilerInstance::CompilerInstance is responsible for finalizing the
   // buffers to prevent use-after-frees.
   CompilerInstance Instance(ImportingInstance.getPCHContainerOperations(),
                             &ImportingInstance.getModuleCache());
   auto &Inv = *Invocation;
   Instance.setInvocation(std::move(Invocation));
 
   Instance.createDiagnostics(new ForwardingDiagnosticConsumer(
                                    ImportingInstance.getDiagnosticClient()),
                              /*ShouldOwnClient=*/true);
 
+  if (llvm::is_contained(DiagOpts.SystemHeaderWarningsModules, ModuleName))
+    Instance.getDiagnostics().setSuppressSystemWarnings(false);
+
   if (FrontendOpts.ModulesShareFileManager) {
     Instance.setFileManager(&ImportingInstance.getFileManager());
   } else {
     Instance.createFileManager(&ImportingInstance.getVirtualFileSystem());
   }
   Instance.createSourceManager(Instance.getFileManager());
   SourceManager &SourceMgr = Instance.getSourceManager();
 

clang/lib/Serialization/ASTReader.cpp

 static bool isExtHandlingFromDiagsError(DiagnosticsEngine &Diags) {
   diag::Severity Ext = Diags.getExtensionHandlingBehavior();
   if (Ext == diag::Severity::Warning && Diags.getWarningsAsErrors())
     return true;
   return Ext >= diag::Severity::Error;
 }
 
 static bool checkDiagnosticMappings(DiagnosticsEngine &StoredDiags,
-                                    DiagnosticsEngine &Diags,
-                                    bool IsSystem, bool Complain) {
+                                    DiagnosticsEngine &Diags, bool IsSystem,
+                                    bool SystemHeaderWarningsInModule,
+                                    bool Complain) {
   // Top-level options
   if (IsSystem) {
     if (Diags.getSuppressSystemWarnings())
       return false;
-    // If -Wsystem-headers was not enabled before, be conservative
-    if (StoredDiags.getSuppressSystemWarnings()) {
+    // If -Wsystem-headers was not enabled before, and it was not explicit,
+    // be conservative
+    if (StoredDiags.getSuppressSystemWarnings() &&
+        !SystemHeaderWarningsInModule) {
       if (Complain)
         Diags.Report(diag::err_pch_diagopt_mismatch) << "-Wsystem-headers";
       return true;
     }
   }
 
   if (Diags.getWarningsAsErrors() && !StoredDiags.getWarningsAsErrors()) {
     if (Complain)
bool PCHValidator::ReadDiagnosticOptions(
 
   ModuleManager &ModuleMgr = Reader.getModuleManager();
   assert(ModuleMgr.size() >= 1 && "what ASTFile is this then");
 
   Module *TopM = getTopImportImplicitModule(ModuleMgr, PP);
   if (!TopM)
     return false;
 
+  Module *Importer = PP.getCurrentModule();
+
+  DiagnosticOptions &ExistingOpts = ExistingDiags.getDiagnosticOptions();
+  bool SystemHeaderWarningsInModule =
+      Importer && llvm::is_contained(ExistingOpts.SystemHeaderWarningsModules,
+                                     Importer->Name);
+
   // FIXME: if the diagnostics are incompatible, save a DiagnosticOptions that
   // contains the union of their flags.
   return checkDiagnosticMappings(*Diags, ExistingDiags, TopM->IsSystem,
-                                 Complain);
+                                 SystemHeaderWarningsInModule, Complain);
 }
 
 /// Collect the macro definitions provided by the given preprocessor
 /// options.
 static void
 collectMacroDefinitions(const PreprocessorOptions &PPOpts,
                         MacroDefinitionsMap &Macros,
                         SmallVectorImpl<StringRef> *MacroNames = nullptr) {

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

 //===- ModuleDepCollector.cpp - Callbacks to collect deps -------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Tooling/DependencyScanning/ModuleDepCollector.h"
 
 #include "clang/Basic/MakeSupport.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/BLAKE3.h"
 #include "llvm/Support/StringSaver.h"
 #include <optional>
 
 using namespace clang;
 using namespace tooling;
 using namespace dependencies;
 
llvm::erase_if(
           StringRef MacroDef = Def.first;
           return CI.getHeaderSearchOpts().ModulesIgnoreMacros.contains(
               llvm::CachedHashString(MacroDef.split('=').first));
         });
     // Remove the now unused option.
     CI.getHeaderSearchOpts().ModulesIgnoreMacros.clear();
   }
 
+  // Apply -Wsystem-headers-in-module for the current module.
+  if (llvm::is_contained(CI.getDiagnosticOpts().SystemHeaderWarningsModules,
+                         Deps.ID.ModuleName))
+    CI.getDiagnosticOpts().Warnings.push_back("system-headers");
+  // Remove the now unused option(s).
+  CI.getDiagnosticOpts().SystemHeaderWarningsModules.clear();
+
   Optimize(CI);
 
   return CI;
 }
 
 llvm::DenseSet<const FileEntry *> ModuleDepCollector::collectModuleMapFiles(
     ArrayRef<ModuleID> ClangModuleDeps) const {
   llvm::DenseSet<const FileEntry *> ModuleMapFiles;

clang/test/ClangScanDeps/Wsystem-headers-in-module.c

+// Check that Wsystem-headers-in-module shows diagnostics in the named system
+// module, but not in other system headers or modules when built with explicit
+// modules.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full > %t/deps.json
+
+// RUN: %deps-to-rsp %t/deps.json --module-name=dependent_sys_mod > %t/dependent_sys_mod.rsp
+// RUN: %deps-to-rsp %t/deps.json --module-name=sys_mod > %t/sys_mod.rsp
+// RUN: %deps-to-rsp %t/deps.json --module-name=other_sys_mod > %t/other_sys_mod.rsp
+// RUN: %deps-to-rsp %t/deps.json --tu-index 0 > %t/tu.rsp
+
+// RUN: %clang @%t/dependent_sys_mod.rsp -verify
+// RUN: %clang @%t/sys_mod.rsp -verify
+// RUN: %clang @%t/other_sys_mod.rsp -verify
+// RUN: %clang @%t/tu.rsp -verify
+
+// CHECK-NOT: warning:
+// CHECK: sys_mod.h:2:7: warning: extra ';'
+// CHECK-NOT: warning:
+
+//--- cdb.json.template
+[{
+  "directory": "DIR",
+  "command": "clang -fsyntax-only -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/mcp DIR/tu.c -isystem DIR/sys -Wextra-semi -Wsystem-headers-in-module=sys_mod",
+  "file": "DIR/tu.c"
+}]
+
+//--- sys/other_sys_header.h
+int x;;
+
+//--- sys_mod.h
+#include "dependent_sys_mod.h"
+int y;; // expected-warning {{extra ';' outside of a function}}
+
+//--- other_sys_mod.h
+int z;;
+// expected-no-diagnostics
+
+//--- dependent_sys_mod.h
+int w;;
+// expected-no-diagnostics
+
+//--- module.modulemap
+module sys_mod [system] { header "sys_mod.h" }
+module other_sys_mod [system] { header "other_sys_mod.h" }
+module dependent_sys_mod [system] { header "dependent_sys_mod.h" }
+
+//--- tu.c
+#include "sys_mod.h"
+#include "other_sys_mod.h"
+#include "other_sys_header.h"
+// expected-no-diagnostics

clang/test/Modules/Wsystem-headers-in-module.c

+// Check that Wsystem-headers-in-module shows diagnostics in the named system
+// module, but not in other system headers or modules.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/mcp \
+// RUN:   -isystem %t/sys %t/tu.c -fsyntax-only -Wextra-semi -Wsystem-headers-in-module=sys_mod \
+// RUN:   2>&1 | FileCheck %s
+
+// CHECK-NOT: warning:
+// CHECK: sys_mod.h:2:7: warning: extra ';'
+// CHECK-NOT: warning:
+
+//--- sys/other_sys_header.h
+int x;;
+//--- sys_mod.h
+#include "dependent_sys_mod.h"
+int y;;
+//--- other_sys_mod.h
+int z;;
+//--- dependent_sys_mod.h
+int w;;
+//--- module.modulemap
+module sys_mod [system] { header "sys_mod.h" }
+module other_sys_mod [system] { header "other_sys_mod.h" }
+module dependent_sys_mod [system] { header "dependent_sys_mod.h" }
+
+//--- tu.c
+#include "sys_mod.h"
+#include "other_sys_mod.h"
+#include "other_sys_header.h"