Modules: add command line option fmodules-disable-diagnostic-validation to disable validation of the diagnostic options when loading the module

[manmanren]

Summary

With PCH+Module, sometimes compiler gives a hard error:
"Module file ‘<some-file path>.pcm' is out of date and needs to be rebuilt"

This happens when we have a PCH importing a module and the module gets overwritten by another compiler instance after we build the pch (one example is that both compiler instances hash to the same pcm file but use different diagnostic options). When we try to load the pch later on, the compiler notices that the imported module is out of date (modification date, size do not match) but it can't handle this out of date pcm (i.e it does not know how to rebuild the pch).

This patch introduces a new command line option so for PCH + module, we can turn on this option and if two compiler instances only differ in diagnostic options, the latter instance will not invalidate the original pcm.

[benlangmuir]

We need to add this option to the module hash (see getModuleHash - we already add a bunch of other HSOpts there). Otherwise the pcm could still be rewritten by a compilation that doesn't use a PCH, and then it would be out of date because of the timestamp instead of the diagnostic options.

[manmanren]

In D22773#495403, @benlangmuir wrote:

We need to add this option to the module hash (see getModuleHash - we already add a bunch of other HSOpts there). Otherwise the pcm could still be rewritten by a compilation that doesn't use a PCH, and then it would be out of date because of the timestamp instead of the diagnostic options.

I was thinking that for a project using PCH+Module, all clang invocations will be using -fmodules-disable-diagnostic-validation, so if only the diagnostic options change, they will hash to the same pcm file but the compiler will not regenerate and overwrite the existing pcm file. So when we load the pch, the time stamp etc will match and the compiler will not throw out-of-date error.

We can definitely hash the option HSOpts.ModulesValidateDiagnosticOptions in getModuleHash. But I don't quite get the reason you give here :)
the pcm could still be rewritten by a compilation that doesn't use a PCH, and then it would be out of date because of the timestamp instead of the diagnostic options

"a compilation that doesn't use a PCH", is that a different project? And we have two projects building in parallel? Just to make sure I understand.

Thanks!
Manman

[benlangmuir]

the pcm could still be rewritten by a compilation that doesn't use a PCH, and then it would be out of date because of the timestamp instead of the diagnostic options

"a compilation that doesn't use a PCH", is that a different project? And we have two projects building in parallel? Just to make sure I understand.

Two different projects (or targets, or whatever), but they don't even have to build at the same time. They just have to share a cache. Suppose you have a project A with a PCH and -fmodules-disable-diagnostic-validation. Suppose you have another project B that does not use this flag, and does not have a PCH. With this patch, A and B can share a module cache.

A builds a PCH that depends on some module X -- OK
B builds with -Werror. Rebuilds X.pcm -- OK

Now suppose we build A again because of some change: it can't build because X.pcm changed, but we haven't rebuilt the PCH.

[manmanren]

In D22773#495524, @benlangmuir wrote:

the pcm could still be rewritten by a compilation that doesn't use a PCH, and then it would be out of date because of the timestamp instead of the diagnostic options

"a compilation that doesn't use a PCH", is that a different project? And we have two projects building in parallel? Just to make sure I understand.

Two different projects (or targets, or whatever), but they don't even have to build at the same time. They just have to share a cache. Suppose you have a project A with a PCH and -fmodules-disable-diagnostic-validation. Suppose you have another project B that does not use this flag, and does not have a PCH. With this patch, A and B can share a module cache.

A builds a PCH that depends on some module X -- OK
B builds with -Werror. Rebuilds X.pcm -- OK

Now suppose we build A again because of some change: it can't build because X.pcm changed, but we haven't rebuilt the PCH.

Got it, I will update the patch!

Manman

[manmanren]

Addressing Ben's comments

[benlangmuir]

LGTM

cfe/trunk/include/clang/Driver/Options.td

def fbuild_session_timestamp : Joined<["-"], "fbuild-session-timestamp=">,
   HelpText<"Time when the current build session started">;
 def fbuild_session_file : Joined<["-"], "fbuild-session-file=">,
   Group<i_Group>, MetaVarName<"<file>">,
   HelpText<"Use the last modification time of <file> as the build session timestamp">;
 def fmodules_validate_once_per_build_session : Flag<["-"], "fmodules-validate-once-per-build-session">,
   Group<i_Group>, Flags<[CC1Option]>,
   HelpText<"Don't verify input files for the modules if the module has been "
            "successfully validated or loaded during this build session">;
+def fmodules_disable_diagnostic_validation : Flag<["-"], "fmodules-disable-diagnostic-validation">,
+  Group<i_Group>, Flags<[CC1Option]>,
+  HelpText<"Disable validation of the diagnostic options when loading the module">;
 def fmodules_validate_system_headers : Flag<["-"], "fmodules-validate-system-headers">,
   Group<i_Group>, Flags<[CC1Option]>,
   HelpText<"Validate the system headers that a module depends on when loading the module">;
 def fmodules : Flag <["-"], "fmodules">, Group<f_Group>,
   Flags<[DriverOption, CC1Option]>,
   HelpText<"Enable the 'modules' language feature">;
 def fimplicit_module_maps : Flag <["-"], "fimplicit-module-maps">, Group<f_Group>,
   Flags<[DriverOption, CC1Option]>,

cfe/trunk/include/clang/Lex/HeaderSearchOptions.h

public:
   unsigned ModulesValidateOncePerBuildSession : 1;
 
   /// \brief Whether to validate system input files when a module is loaded.
   unsigned ModulesValidateSystemHeaders : 1;
 
   /// Whether the module includes debug information (-gmodules).
   unsigned UseDebugInfo : 1;
 
+  unsigned ModulesValidateDiagnosticOptions : 1;
+
   HeaderSearchOptions(StringRef _Sysroot = "/")
       : Sysroot(_Sysroot), ModuleFormat("raw"), DisableModuleHash(0),
         ImplicitModuleMaps(0), ModuleMapFileHomeIsCwd(0),
         ModuleCachePruneInterval(7 * 24 * 60 * 60),
         ModuleCachePruneAfter(31 * 24 * 60 * 60), BuildSessionTimestamp(0),
         UseBuiltinIncludes(true), UseStandardSystemIncludes(true),
         UseStandardCXXIncludes(true), UseLibcxx(false), Verbose(false),
         ModulesValidateOncePerBuildSession(false),
         ModulesValidateSystemHeaders(false),
-        UseDebugInfo(false) {}
+        UseDebugInfo(false), ModulesValidateDiagnosticOptions(true) {}
 
   /// AddPath - Add the \p Path path to the specified \p Group list.
   void AddPath(StringRef Path, frontend::IncludeDirGroup Group,
                bool IsFramework, bool IgnoreSysRoot) {
     UserEntries.emplace_back(Path, Group, IsFramework, IgnoreSysRoot);
   }
 
   /// AddSystemHeaderPrefix - Override whether \#include directives naming a

cfe/trunk/include/clang/Serialization/ASTReader.h

ASTReadResult ReadASTCore(StringRef FileName, ModuleKind Type,
                             unsigned ClientLoadCapabilities);
   ASTReadResult ReadControlBlock(ModuleFile &F,
                                  SmallVectorImpl<ImportedModule> &Loaded,
                                  const ModuleFile *ImportedBy,
                                  unsigned ClientLoadCapabilities);
   static ASTReadResult ReadOptionsBlock(
       llvm::BitstreamCursor &Stream, unsigned ClientLoadCapabilities,
       bool AllowCompatibleConfigurationMismatch, ASTReaderListener &Listener,
-      std::string &SuggestedPredefines);
+      std::string &SuggestedPredefines, bool ValidateDiagnosticOptions);
   ASTReadResult ReadASTBlock(ModuleFile &F, unsigned ClientLoadCapabilities);
   ASTReadResult ReadExtensionBlock(ModuleFile &F);
   bool ParseLineTable(ModuleFile &F, const RecordData &Record);
   bool ReadSourceManagerBlock(ModuleFile &F);
   llvm::BitstreamCursor &SLocCursorForID(int ID);
   SourceLocation getImportLocation(ModuleFile *F);
   ASTReadResult ReadModuleMapFileBlock(RecordData &Record, ModuleFile &F,
                                        const ModuleFile *ImportedBy,
public:
 
   /// \brief Read the control block for the named AST file.
   ///
   /// \returns true if an error occurred, false otherwise.
   static bool
   readASTFileControlBlock(StringRef Filename, FileManager &FileMgr,
                           const PCHContainerReader &PCHContainerRdr,
                           bool FindModuleFileExtensions,
-                          ASTReaderListener &Listener);
+                          ASTReaderListener &Listener,
+                          bool ValidateDiagnosticOptions);
 
   /// \brief Determine whether the given AST file is acceptable to load into a
   /// translation unit with the given language and target options.
   static bool isAcceptableASTFile(StringRef Filename, FileManager &FileMgr,
                                   const PCHContainerReader &PCHContainerRdr,
                                   const LangOptions &LangOpts,
                                   const TargetOptions &TargetOpts,
                                   const PreprocessorOptions &PPOpts,

cfe/trunk/lib/Frontend/CompilerInvocation.cpp

static void ParseHeaderSearchArgs(HeaderSearchOptions &Opts, ArgList &Args) {
   Opts.UseStandardSystemIncludes = !Args.hasArg(OPT_nostdsysteminc);
   Opts.UseStandardCXXIncludes = !Args.hasArg(OPT_nostdincxx);
   if (const Arg *A = Args.getLastArg(OPT_stdlib_EQ))
     Opts.UseLibcxx = (strcmp(A->getValue(), "libc++") == 0);
   Opts.ResourceDir = Args.getLastArgValue(OPT_resource_dir);
   Opts.ModuleCachePath = Args.getLastArgValue(OPT_fmodules_cache_path);
   Opts.ModuleUserBuildPath = Args.getLastArgValue(OPT_fmodules_user_build_path);
   Opts.DisableModuleHash = Args.hasArg(OPT_fdisable_module_hash);
+  Opts.ModulesValidateDiagnosticOptions =
+      !Args.hasArg(OPT_fmodules_disable_diagnostic_validation);
   Opts.ImplicitModuleMaps = Args.hasArg(OPT_fimplicit_module_maps);
   Opts.ModuleMapFileHomeIsCwd = Args.hasArg(OPT_fmodule_map_file_home_is_cwd);
   Opts.ModuleCachePruneInterval =
       getLastArgIntValue(Args, OPT_fmodules_prune_interval, 7 * 24 * 60 * 60);
   Opts.ModuleCachePruneAfter =
       getLastArgIntValue(Args, OPT_fmodules_prune_after, 31 * 24 * 60 * 60);
   Opts.ModulesValidateOncePerBuildSession =
       Args.hasArg(OPT_fmodules_validate_once_per_build_session);
#include "clang/Basic/LangOptions.def"
 
   // Extend the signature with the sysroot and other header search options.
   code = hash_combine(code, hsOpts.Sysroot,
                       hsOpts.ModuleFormat,
                       hsOpts.UseDebugInfo,
                       hsOpts.UseBuiltinIncludes,
                       hsOpts.UseStandardSystemIncludes,
                       hsOpts.UseStandardCXXIncludes,
-                      hsOpts.UseLibcxx);
+                      hsOpts.UseLibcxx,
+                      hsOpts.ModulesValidateDiagnosticOptions);
   code = hash_combine(code, hsOpts.ResourceDir);
 
   // Extend the signature with the user build path.
   code = hash_combine(code, hsOpts.ModuleUserBuildPath);
 
   // Extend the signature with the module file extensions.
   const FrontendOptions &frontendOpts = getFrontendOpts();
   for (const auto &ext : frontendOpts.ModuleFileExtensions) {

cfe/trunk/lib/Frontend/FrontendActions.cpp

void DumpModuleInfoAction::ExecuteAction() {
   if (!OutputFileName.empty() && OutputFileName != "-") {
     std::error_code EC;
     OutFile.reset(new llvm::raw_fd_ostream(OutputFileName.str(), EC,
                                            llvm::sys::fs::F_Text));
   }
   llvm::raw_ostream &Out = OutFile.get()? *OutFile.get() : llvm::outs();
 
   Out << "Information for module file '" << getCurrentFile() << "':\n";
+  Preprocessor &PP = getCompilerInstance().getPreprocessor();
   DumpModuleInfoListener Listener(Out);
+  HeaderSearchOptions &HSOpts =
+      PP.getHeaderSearchInfo().getHeaderSearchOpts();
   ASTReader::readASTFileControlBlock(
       getCurrentFile(), getCompilerInstance().getFileManager(),
       getCompilerInstance().getPCHContainerReader(),
-      /*FindModuleFileExtensions=*/true, Listener);
+      /*FindModuleFileExtensions=*/true, Listener,
+      HSOpts.ModulesValidateDiagnosticOptions);
 }
 
 //===----------------------------------------------------------------------===//
 // Preprocessor Actions
 //===----------------------------------------------------------------------===//
 
 void DumpRawTokensAction::ExecuteAction() {
   Preprocessor &PP = getCompilerInstance().getPreprocessor();

cfe/trunk/lib/Serialization/ASTReader.cpp

static bool isDiagnosedResult(ASTReader::ASTReadResult ARR, unsigned Caps) {
   }
 
   llvm_unreachable("unknown ASTReadResult");
 }
 
 ASTReader::ASTReadResult ASTReader::ReadOptionsBlock(
     BitstreamCursor &Stream, unsigned ClientLoadCapabilities,
     bool AllowCompatibleConfigurationMismatch, ASTReaderListener &Listener,
-    std::string &SuggestedPredefines) {
+    std::string &SuggestedPredefines, bool ValidateDiagnosticOptions) {
   if (Stream.EnterSubBlock(OPTIONS_BLOCK_ID))
     return Failure;
 
   // Read all of the records in the options block.
   RecordData Record;
   ASTReadResult Result = Success;
   while (1) {
     llvm::BitstreamEntry Entry = Stream.advance();
case TARGET_OPTIONS: {
       if (ParseTargetOptions(Record, Complain, Listener,
                              AllowCompatibleConfigurationMismatch))
         Result = ConfigurationMismatch;
       break;
     }
 
     case DIAGNOSTIC_OPTIONS: {
       bool Complain = (ClientLoadCapabilities & ARR_OutOfDate) == 0;
-      if (!AllowCompatibleConfigurationMismatch &&
+      if (ValidateDiagnosticOptions &&
+          !AllowCompatibleConfigurationMismatch &&
           ParseDiagnosticOptions(Record, Complain, Listener))
         return OutOfDate;
       break;
     }
 
     case FILE_SYSTEM_OPTIONS: {
       bool Complain = (ClientLoadCapabilities & ARR_ConfigurationMismatch) == 0;
       if (!AllowCompatibleConfigurationMismatch &&
case llvm::BitstreamEntry::SubBlock:
         if (Listener && !ImportedBy) {
           // Should we allow the configuration of the module file to differ from
           // the configuration of the current translation unit in a compatible
           // way?
           //
           // FIXME: Allow this for files explicitly specified with -include-pch.
           bool AllowCompatibleConfigurationMismatch =
               F.Kind == MK_ExplicitModule;
+          const HeaderSearchOptions &HSOpts =
+              PP.getHeaderSearchInfo().getHeaderSearchOpts();
 
           Result = ReadOptionsBlock(Stream, ClientLoadCapabilities,
                                     AllowCompatibleConfigurationMismatch,
-                                    *Listener, SuggestedPredefines);
+                                    *Listener, SuggestedPredefines,
+                                    HSOpts.ModulesValidateDiagnosticOptions);
           if (Result == Failure) {
             Error("malformed block record in AST file");
             return Result;
           }
 
           if (DisableValidation ||
               (AllowConfigurationMismatch && Result == ConfigurationMismatch))
             Result = Success;
public:
     }
   };
 }
 
 bool ASTReader::readASTFileControlBlock(
     StringRef Filename, FileManager &FileMgr,
     const PCHContainerReader &PCHContainerRdr,
     bool FindModuleFileExtensions,
-    ASTReaderListener &Listener) {
+    ASTReaderListener &Listener, bool ValidateDiagnosticOptions) {
   // Open the AST file.
   // FIXME: This allows use of the VFS; we do not allow use of the
   // VFS when actually loading a module.
   auto Buffer = FileMgr.getBufferForFile(Filename);
   if (!Buffer) {
     return true;
   }
 
while (!DoneWithControlBlock) {
 
     switch (Entry.Kind) {
     case llvm::BitstreamEntry::SubBlock: {
       switch (Entry.ID) {
       case OPTIONS_BLOCK_ID: {
         std::string IgnoredSuggestedPredefines;
         if (ReadOptionsBlock(Stream, ARR_ConfigurationMismatch | ARR_OutOfDate,
                              /*AllowCompatibleConfigurationMismatch*/ false,
-                             Listener, IgnoredSuggestedPredefines) != Success)
+                             Listener, IgnoredSuggestedPredefines,
+                             ValidateDiagnosticOptions) != Success)
           return true;
         break;
       }
 
       case INPUT_FILES_BLOCK_ID:
         InputFilesCursor = Stream;
         if (Stream.SkipBlock() ||
             (NeedsInputFiles &&
bool ASTReader::isAcceptableASTFile(
     StringRef Filename, FileManager &FileMgr,
     const PCHContainerReader &PCHContainerRdr, const LangOptions &LangOpts,
     const TargetOptions &TargetOpts, const PreprocessorOptions &PPOpts,
     std::string ExistingModuleCachePath) {
   SimplePCHValidator validator(LangOpts, TargetOpts, PPOpts,
                                ExistingModuleCachePath, FileMgr);
   return !readASTFileControlBlock(Filename, FileMgr, PCHContainerRdr,
                                   /*FindModuleFileExtensions=*/false,
-                                  validator);
+                                  validator,
+                                  /*ValidateDiagnosticOptions=*/true);
 }
 
 ASTReader::ASTReadResult
 ASTReader::ReadSubmoduleBlock(ModuleFile &F, unsigned ClientLoadCapabilities) {
   // Enter the submodule block.
   if (F.Stream.EnterSubBlock(SUBMODULE_BLOCK_ID)) {
     Error("malformed submodule block record in AST file");
     return Failure;

cfe/trunk/test/Driver/modules.m

 // MODULES_VALIDATE_ONCE_ERR: option '-fmodules-validate-once-per-build-session' requires '-fbuild-session-timestamp=<seconds since Epoch>' or '-fbuild-session-file=<file>'
 
 // RUN: %clang -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_SYSTEM_HEADERS_DEFAULT %s
 // MODULES_VALIDATE_SYSTEM_HEADERS_DEFAULT-NOT: -fmodules-validate-system-headers
 
 // RUN: %clang -fmodules-validate-system-headers -### %s 2>&1 | FileCheck -check-prefix=MODULES_VALIDATE_SYSTEM_HEADERS %s
 // MODULES_VALIDATE_SYSTEM_HEADERS: -fmodules-validate-system-headers
 
+// RUN: %clang -### %s 2>&1 | FileCheck -check-prefix=MODULES_DISABLE_DIAGNOSTIC_VALIDATION_DEFAULT %s
+// MODULES_DISABLE_DIAGNOSTIC_VALIDATION_DEFAULT-NOT: -fmodules-disable-diagnostic-validation
+
+// RUN: %clang -fmodules-disable-diagnostic-validation -### %s 2>&1 | FileCheck -check-prefix=MODULES_DISABLE_DIAGNOSTIC_VALIDATION %s
+// MODULES_DISABLE_DIAGNOSTIC_VALIDATION: -fmodules-disable-diagnostic-validation
+
 // RUN: %clang -fmodules -fmodule-map-file=foo.map -fmodule-map-file=bar.map -### %s 2>&1 | FileCheck -check-prefix=CHECK-MODULE-MAP-FILES %s
 // CHECK-MODULE-MAP-FILES: "-fmodules"
 // CHECK-MODULE-MAP-FILES: "-fmodule-map-file=foo.map"
 // CHECK-MODULE-MAP-FILES: "-fmodule-map-file=bar.map"
 
 // RUN: %clang -fmodules -fmodule-file=foo.pcm -fmodule-file=bar.pcm -### %s 2>&1 | FileCheck -check-prefix=CHECK-MODULE-FILES %s
 // CHECK-MODULE-FILES: "-fmodules"
 // CHECK-MODULE-FILES: "-fmodule-file=foo.pcm"
 // CHECK-MODULE-FILES: "-fmodule-file=bar.pcm"
 
 // RUN: %clang -fno-modules -fmodule-file=foo.pcm -fmodule-file=bar.pcm -### %s 2>&1 | FileCheck -check-prefix=CHECK-NO-MODULE-FILES %s
 // CHECK-NO-MODULE-FILES-NOT: "-fmodules"
 // CHECK-NO-MODULE-FILES-NOT: "-fmodule-file=foo.pcm"
 // CHECK-NO-MODULE-FILES-NOT: "-fmodule-file=bar.pcm"

cfe/trunk/test/Modules/Inputs/DiagOutOfDate.h

+const int a = 1;

cfe/trunk/test/Modules/Inputs/module.map

module MethodPoolString2 {
   header "MethodPoolString2.h"
 }
 
 module Empty {}
 
 module MacroFabs1 {
   header "MacroFabs1.h"
 }
+
+module DiagOutOfDate {
+  header "DiagOutOfDate.h"
+}

cfe/trunk/test/Modules/Inputs/pch-import-module-out-of-date.pch

+@import DiagOutOfDate;

cfe/trunk/test/Modules/diagnostic-options-out-of-date.m

+// RUN: rm -rf %t
+// Build A.pcm
+// RUN: %clang_cc1 -Werror -Wno-conversion -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -I %S/Inputs %s -fmodules-disable-diagnostic-validation
+// Build pch that imports A.pcm
+// RUN: %clang_cc1 -Werror -Wno-conversion -emit-pch -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -o %t.pch -I %S/Inputs -x objective-c-header %S/Inputs/pch-import-module-out-of-date.pch -fmodules-disable-diagnostic-validation
+// Make sure that we don't rebuild A.pcm and overwrite the original A.pcm that the pch imports
+// RUN: %clang_cc1 -Werror -Wconversion -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -I %S/Inputs %s -fmodules-disable-diagnostic-validation
+// Make sure we don't error out when using the pch
+// RUN: %clang_cc1 -Werror -Wno-conversion -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -fsyntax-only -I %S/Inputs -include-pch %t.pch %s -verify -fmodules-disable-diagnostic-validation
+// expected-no-diagnostics
+
+@import DiagOutOfDate;