[clang][Modules] Add -fsystem-module flag

[Bigcheese]

Summary

The -fsystem-module flag is used when explicitly building a module. It
forces the module to be treated as a system module. This is used when
converting an implicit build to an explicit build to match the
systemness the implicit build would have had for a given module.

[bruno]

Hi Michael, thanks for improving this.

clang/include/clang/Driver/Options.td

1448

I wonder if -isystem-module wouldn't be better since it's kinda similar with -isystem for headers, but for modules.

clang/lib/Frontend/CompilerInvocation.cpp

1912

What happens when we use this with implicit modules? It will just be ignored? If so, users might think that using -fsystem-module might help them get rid of some warnings/errors, when in fact it won't.

clang/test/Modules/fsystem-module.m

2

Is this really needed given you are only using %t-saved below?

[dexonsmith]

clang/include/clang/Driver/Options.td

1448

FWIW, I prefer -fsystem-module. I would expect -i* to be modifying search paths, and this option does not do that.

[dexonsmith]

This is used when
converting an implicit build to an explicit build to match the
systemness the implicit build would have had for a given module.

I had another thought. What if for the explicitly built "system" modules:

• If -Wsystem-headers is on, leave them as user modules in the explicit build.
• If -Wsystem-headers is off, turn off all diagnostics in the explicit build.

Does that give the right semantics, or is there something subtly different?

[Bigcheese]

clang/lib/Frontend/CompilerInvocation.cpp

1912

It's an error unless you also pass -emit-module. If you also have implicit modules enabled while explicitly building a module then those modules will be treated as normal, but I don't think there's a huge cause for confusion there. The only way to use it is when explicitly building a module, so it's clear which module it applies to.

clang/test/Modules/fsystem-module.m

2

It's not, that's a leftover from the test I based this one on. I'll clean it up.

[Bigcheese]

In D75395#1901778, @dexonsmith wrote:

This is used when
converting an implicit build to an explicit build to match the
systemness the implicit build would have had for a given module.

I had another thought. What if for the explicitly built "system" modules:

• If -Wsystem-headers is on, leave them as user modules in the explicit build.
• If -Wsystem-headers is off, turn off all diagnostics in the explicit build.

Does that give the right semantics, or is there something subtly different?

I considered this, but decided against it because I wanted the implicit and explicit builds to be as similar as possible, and reduce the amount of changes made to the original command line. There's a lot of code in Clang dealing with system files, and I'm not 100% sure that -Wno-everything would be equivalent.

[dexonsmith]

In D75395#1901859, @Bigcheese wrote:

In D75395#1901778, @dexonsmith wrote:

This is used when
converting an implicit build to an explicit build to match the
systemness the implicit build would have had for a given module.

I had another thought. What if for the explicitly built "system" modules:

• If -Wsystem-headers is on, leave them as user modules in the explicit build.
• If -Wsystem-headers is off, turn off all diagnostics in the explicit build.

Does that give the right semantics, or is there something subtly different?

I considered this, but decided against it because I wanted the implicit and explicit builds to be as similar as possible, and reduce the amount of changes made to the original command line. There's a lot of code in Clang dealing with system files, and I'm not 100% sure that -Wno-everything would be equivalent.

Okay, SGTM.

[Bigcheese]

Cleaned up the test to not reference unused paths.

[bruno]

LGTM

clang/include/clang/Driver/Options.td

def fmodules_ts : Flag <["-"], "fmodules-ts">, Group<f_Group>,
   Flags<[CC1Option]>, HelpText<"Enable support for the C++ Modules TS">;
 def fmodule_maps : Flag <["-"], "fmodule-maps">, Alias<fimplicit_module_maps>;
 def fmodule_name_EQ : Joined<["-"], "fmodule-name=">, Group<f_Group>,
   Flags<[DriverOption,CC1Option]>, MetaVarName<"<name>">,
   HelpText<"Specify the name of the module to build">;
 def fmodule_name : Separate<["-"], "fmodule-name">, Alias<fmodule_name_EQ>;
 def fmodule_implementation_of : Separate<["-"], "fmodule-implementation-of">,
   Flags<[CC1Option]>, Alias<fmodule_name_EQ>;
+def fsystem_module : Flag<["-"], "fsystem-module">, Flags<[CC1Option]>,
+  HelpText<"Build this module as a system module. Only used with -emit-module">;
 def fmodule_map_file : Joined<["-"], "fmodule-map-file=">,
   Group<f_Group>, Flags<[DriverOption,CC1Option]>, MetaVarName<"<file>">,
   HelpText<"Load this module map file">;
 def fmodule_file : Joined<["-"], "fmodule-file=">,
   Group<i_Group>, Flags<[DriverOption,CC1Option]>, MetaVarName<"[<name>=]<file>">,
   HelpText<"Specify the mapping of module name to precompiled module file, or load a module file if name is omitted.">;
 def fmodules_ignore_macro : Joined<["-"], "fmodules-ignore-macro=">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Ignore the definition of the given macro when building and loading modules">;

clang/include/clang/Frontend/FrontendOptions.h

public:
   unsigned ModulesEmbedAllFiles : 1;
 
   /// Whether timestamps should be written to the produced PCH file.
   unsigned IncludeTimestamps : 1;
 
   /// Should a temporary file be used during compilation.
   unsigned UseTemporary : 1;
 
+  /// When using -emit-module, treat the modulemap as a system module.
+  unsigned IsSystemModule : 1;
+
   CodeCompleteOptions CodeCompleteOpts;
 
   /// Specifies the output format of the AST.
   ASTDumpOutputFormat ASTDumpFormat = ADOF_Default;
 
   enum {
     ARCMT_None,
     ARCMT_Check,

clang/lib/Frontend/CompilerInvocation.cpp

for (const auto *A : Args.filtered(OPT_fmodule_file)) {
     StringRef Val = A->getValue();
     if (Val.find('=') == StringRef::npos)
       Opts.ModuleFiles.push_back(std::string(Val));
   }
   Opts.ModulesEmbedFiles = Args.getAllArgValues(OPT_fmodules_embed_file_EQ);
   Opts.ModulesEmbedAllFiles = Args.hasArg(OPT_fmodules_embed_all_files);
   Opts.IncludeTimestamps = !Args.hasArg(OPT_fno_pch_timestamp);
   Opts.UseTemporary = !Args.hasArg(OPT_fno_temp_file);
+  Opts.IsSystemModule = Args.hasArg(OPT_fsystem_module);
+
+  if (Opts.ProgramAction != frontend::GenerateModule && Opts.IsSystemModule)
+    Diags.Report(diag::err_drv_argument_only_allowed_with) << "-fsystem-module"
+                                                           << "-emit-module";
 
   Opts.CodeCompleteOpts.IncludeMacros
     = Args.hasArg(OPT_code_completion_macros);
   Opts.CodeCompleteOpts.IncludeCodePatterns
     = Args.hasArg(OPT_code_completion_patterns);
   Opts.CodeCompleteOpts.IncludeGlobals
     = !Args.hasArg(OPT_no_code_completion_globals);
   Opts.CodeCompleteOpts.IncludeNamespaceLevelDecls
if (IK.isUnknown()) {
       // FIXME: Warn on this?
       if (IK.isUnknown())
         IK = Language::C;
       // FIXME: Remove this hack.
       if (i == 0)
         DashX = IK;
     }
 
+    bool IsSystem = false;
+
     // The -emit-module action implicitly takes a module map.
     if (Opts.ProgramAction == frontend::GenerateModule &&
-        IK.getFormat() == InputKind::Source)
+        IK.getFormat() == InputKind::Source) {
       IK = IK.withFormat(InputKind::ModuleMap);
+      IsSystem = Opts.IsSystemModule;
+    }
 
-    Opts.Inputs.emplace_back(std::move(Inputs[i]), IK);
+    Opts.Inputs.emplace_back(std::move(Inputs[i]), IK, IsSystem);
   }
 
   return DashX;
 }
 
 std::string CompilerInvocation::GetResourcesPath(const char *Argv0,
                                                  void *MainAddr) {
   std::string ClangExecutable =

clang/test/Modules/fsystem-module.m

+// RUN: rm -rf %t.dir
+// RUN: mkdir %t.dir
+
+// -fsystem-module requires -emit-module
+// RUN: not %clang_cc1 -fsyntax-only -fsystem-module %s 2>&1 | grep "-emit-module"
+
+// RUN: not %clang_cc1 -fmodules -I %S/Inputs \
+// RUN:   -emit-module -fmodule-name=warning -pedantic -Werror \
+// RUN:   %S/Inputs/module.map -o %t.dir/warning.pcm
+
+// RUN: %clang_cc1 -fmodules -I %S/Inputs \
+// RUN:   -emit-module -fmodule-name=warning -pedantic -Werror \
+// RUN:   %S/Inputs/module.map -o %t.dir/warning-system.pcm -fsystem-module
+
+// RUN: not %clang_cc1 -fmodules -I %S/Inputs \
+// RUN:   -emit-module -fmodule-name=warning -pedantic -Werror \
+// RUN:   %S/Inputs/module.map -o %t.dir/warning-system.pcm -fsystem-module \
+// RUN:   -Wsystem-headers