[Implicit Modules] Add -cc1 option -fmodules-strict-hash which includes search paths and diagnostics.

[Bigcheese]

Add diagnostics.

[bruno]

Hi Michael, thanks for working on this!

include/clang/Lex/HeaderSearchOptions.h

206 ↗

(On Diff #223345)

*whether

209 ↗

(On Diff #223345)

What else do you plan to add in the future as part of "all the things that could impact"? It seems to me that by default this should always be the case, but header search related things are special because one might want to handwave on correctness to have smaller caches (default behavior right now).

I wonder if we should instead have fmodules-strict-header-seach and later on add a more generic thing that group such cases? WDYT?

[Bigcheese]

include/clang/Lex/HeaderSearchOptions.h

209 ↗

(On Diff #223345)

This also includes diagnostic options. I'm not sure it's useful for users to pick each individual thing they care about for the hash. The intent here is just to capture every possible difference we find.

[Bigcheese]

Fixed spelling and updated comment.

[bruno]

LGTM with one minor change. Can you add an entry in the modules docs for this flag and mention that using it can lead to more PCMs in an implicit build?

[bruno]

*using implicit modules in a build where compiler flags in different invocations aren't homogeneous, or something along those lines.

[Bigcheese]

While adding the documentation I realized that a better name for this option would be -fmodules-strict-context-hash to make it clear which hash it's referring to.

[bruno]

While adding the documentation I realized that a better name for this option would be -fmodules-strict-context-hash to make it clear which hash it's referring to.

-fmodules-strict-context-hash SGTM!

[thakis]

Looks like this fails on win: http://45.33.8.238/win/841/step_6.txt

Ptal!

Maybe just cat'ing all files instead of echoing the first and piping into cat works?

clang/docs/Modules.rst

``-fmodule-file=[<name>=]<file>``
   prebuilt module search mechanism (in addition to ``-fprebuilt-module-path``)
   and the module is only loaded if required. Note that in this case the
   specified file also overrides this module's paths that might be embedded
   in other precompiled module files.
 
 ``-fprebuilt-module-path=<directory>``
   Specify the path to the prebuilt modules. If specified, we will look for modules in this directory for a given top-level module name. We don't need a module map for loading prebuilt modules in this directory and the compiler will not try to rebuild these modules. This can be specified multiple times.
 
+-cc1 Options
+^^^^^^^^^^^^
+
+``-fmodules-strict-context-hash``
+  Enables hashing of all compiler options that could impact the semantics of a
+  module in an implicit build. This includes things such as header search paths
+  and diagnostics. Using this option may lead to an excessive number of modules
+  being built if the command line arguments are not homogeneous across your
+  build.
+
 Module Semantics
 ================
 
 Modules are modeled as if each submodule were a separate translation unit, and a module import makes names from the other translation unit visible. Each submodule starts with a new preprocessor state and an empty translation unit.
 
 .. note::
 
   This behavior is currently only approximated when building a module with submodules. Entities within a submodule that has already been built are visible when building later submodules in that module. This can lead to fragile modules that depend on the build order used for the submodules of the module, and should not be relied upon. This behavior is subject to change.

clang/include/clang/Driver/CC1Options.td

 //===----------------------------------------------------------------------===//
 
 def nostdsysteminc : Flag<["-"], "nostdsysteminc">,
   HelpText<"Disable standard system #include directories">;
 def fdisable_module_hash : Flag<["-"], "fdisable-module-hash">,
   HelpText<"Disable the module hash">;
 def fmodules_hash_content : Flag<["-"], "fmodules-hash-content">,
   HelpText<"Enable hashing the content of a module file">;
+def fmodules_strict_context_hash : Flag<["-"], "fmodules-strict-context-hash">,
+  HelpText<"Enable hashing of all compiler options that could impact the "
+           "semantics of a module in an implicit build">;
 def c_isystem : JoinedOrSeparate<["-"], "c-isystem">, MetaVarName<"<directory>">,
   HelpText<"Add directory to the C SYSTEM include search path">;
 def objc_isystem : JoinedOrSeparate<["-"], "objc-isystem">,
   MetaVarName<"<directory>">,
   HelpText<"Add directory to the ObjC SYSTEM include search path">;
 def objcxx_isystem : JoinedOrSeparate<["-"], "objcxx-isystem">,
   MetaVarName<"<directory>">,
   HelpText<"Add directory to the ObjC++ SYSTEM include search path">;

clang/include/clang/Lex/HeaderSearchOptions.h

 //===- HeaderSearchOptions.h ------------------------------------*- 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_LEX_HEADERSEARCHOPTIONS_H
 #define LLVM_CLANG_LEX_HEADERSEARCHOPTIONS_H
 
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/CachedHashString.h"
+#include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/StringRef.h"
 #include <cstdint>
 #include <string>
 #include <vector>
 #include <map>
 
 namespace clang {
public:
 
   /// Whether the module includes debug information (-gmodules).
   unsigned UseDebugInfo : 1;
 
   unsigned ModulesValidateDiagnosticOptions : 1;
 
   unsigned ModulesHashContent : 1;
 
+  /// Whether we should include all things that could impact the module in the
+  /// hash.
+  ///
+  /// This includes things like the full header search path, and enabled
+  /// diagnostics.
+  unsigned ModulesStrictContextHash : 1;
+
   HeaderSearchOptions(StringRef _Sysroot = "/")
       : Sysroot(_Sysroot), ModuleFormat("raw"), DisableModuleHash(false),
         ImplicitModuleMaps(false), ModuleMapFileHomeIsCwd(false),
         UseBuiltinIncludes(true), UseStandardSystemIncludes(true),
         UseStandardCXXIncludes(true), UseLibcxx(false), Verbose(false),
         ModulesValidateOncePerBuildSession(false),
         ModulesValidateSystemHeaders(false),
         ValidateASTInputFilesContent(false), UseDebugInfo(false),
-        ModulesValidateDiagnosticOptions(true), ModulesHashContent(false) {}
+        ModulesValidateDiagnosticOptions(true), ModulesHashContent(false),
+        ModulesStrictContextHash(false) {}
 
   /// 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
   /// path starting with \p Prefix should be considered as naming a system
   /// header.
   void AddSystemHeaderPrefix(StringRef Prefix, bool IsSystemHeader) {
     SystemHeaderPrefixes.emplace_back(Prefix, IsSystemHeader);
   }
 
   void AddVFSOverlayFile(StringRef Name) {
     VFSOverlayFiles.push_back(Name);
   }
 
   void AddPrebuiltModulePath(StringRef Name) {
     PrebuiltModulePaths.push_back(Name);
   }
 };
 
+inline llvm::hash_code hash_value(const HeaderSearchOptions::Entry &E) {
+  return llvm::hash_combine(E.Path, E.Group, E.IsFramework, E.IgnoreSysRoot);
+}
+
+inline llvm::hash_code
+hash_value(const HeaderSearchOptions::SystemHeaderPrefix &SHP) {
+  return llvm::hash_combine(SHP.Prefix, SHP.IsSystemHeader);
+}
+
 } // namespace clang
 
 #endif // LLVM_CLANG_LEX_HEADERSEARCHOPTIONS_H

clang/lib/Frontend/CompilerInvocation.cpp

for (const auto *A : Args.filtered(OPT_fmodule_file)) {
     StringRef Val = A->getValue();
     if (Val.find('=') != StringRef::npos)
       Opts.PrebuiltModuleFiles.insert(Val.split('='));
   }
   for (const auto *A : Args.filtered(OPT_fprebuilt_module_path))
     Opts.AddPrebuiltModulePath(A->getValue());
   Opts.DisableModuleHash = Args.hasArg(OPT_fdisable_module_hash);
   Opts.ModulesHashContent = Args.hasArg(OPT_fmodules_hash_content);
+  Opts.ModulesStrictContextHash = Args.hasArg(OPT_fmodules_strict_context_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);

 }
 
 std::string CompilerInvocation::getModuleHash() const {
   // Note: For QoI reasons, the things we use as a hash here should all be
   // dumped via the -module-info flag.
   using llvm::hash_code;
   using llvm::hash_value;
   using llvm::hash_combine;
+  using llvm::hash_combine_range;
 
   // Start the signature with the compiler version.
   // FIXME: We'd rather use something more cryptographically sound than
   // CityHash, but this will do for now.
   hash_code code = hash_value(getClangFullRepositoryVersion());
 
   // Extend the signature with the language options
 #define LANGOPT(Name, Bits, Default, Description) \
code = hash_combine(code, hsOpts.Sysroot,
                       hsOpts.UseDebugInfo,
                       hsOpts.UseBuiltinIncludes,
                       hsOpts.UseStandardSystemIncludes,
                       hsOpts.UseStandardCXXIncludes,
                       hsOpts.UseLibcxx,
                       hsOpts.ModulesValidateDiagnosticOptions);
   code = hash_combine(code, hsOpts.ResourceDir);
 
+  if (hsOpts.ModulesStrictContextHash) {
+    hash_code SHPC = hash_combine_range(hsOpts.SystemHeaderPrefixes.begin(),
+                                        hsOpts.SystemHeaderPrefixes.end());
+    hash_code UEC = hash_combine_range(hsOpts.UserEntries.begin(),
+                                       hsOpts.UserEntries.end());
+    code = hash_combine(code, hsOpts.SystemHeaderPrefixes.size(), SHPC,
+                        hsOpts.UserEntries.size(), UEC);
+
+    const DiagnosticOptions &diagOpts = getDiagnosticOpts();
+    #define DIAGOPT(Name, Bits, Default) \
+      code = hash_combine(code, diagOpts.Name);
+    #define ENUM_DIAGOPT(Name, Type, Bits, Default) \
+      code = hash_combine(code, diagOpts.get##Name());
+    #include "clang/Basic/DiagnosticOptions.def"
+    #undef DIAGOPT
+    #undef ENUM_DIAGOPT
+  }
+
   // 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) {
     code = ext->hashExtension(code);
   }

clang/test/Modules/context-hash.c

+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -internal-isystem \
+// RUN:   %S/Inputs/System/usr/include -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t %s -Rmodule-build 2> %t1
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -internal-isystem \
+// RUN:   %S/Inputs/System/usr/include -internal-isystem %S -fmodules \
+// RUN:   -fimplicit-module-maps -fmodules-cache-path=%t %s -Rmodule-build 2> \
+// RUN:   %t2
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -internal-isystem \
+// RUN:   %S/Inputs/System/usr/include -internal-isystem %S -fmodules \
+// RUN:   -fimplicit-module-maps -fmodules-cache-path=%t %s \
+// RUN:   -fmodules-strict-context-hash -Rmodule-build 2> %t3
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -Weverything -internal-isystem \
+// RUN:   %S/Inputs/System/usr/include -fmodules -fmodules-strict-context-hash \
+// RUN:   -fimplicit-module-maps -fmodules-cache-path=%t %s -Rmodule-build 2> \
+// RUN:   %t4
+// RUN: echo %t | cat - %t1 %t2 %t3 %t4 | FileCheck %s
+
+// This test verifies that only strict hashing includes search paths and
+// diagnostics in the module context hash.
+
+#include <stdio.h>
+
+// CHECK: [[PREFIX:(.*[/\\])+[a-zA-Z0-9.-]+]]
+// CHECK: building module 'cstd' as '[[PREFIX]]{{[/\\]}}[[CONTEXT_HASH:[A-Z0-9]+]]{{[/\\]}}cstd-[[AST_HASH:[A-Z0-9]+]].pcm'
+// CHECK: building module 'cstd' as '{{.*[/\\]}}[[CONTEXT_HASH]]{{[/\\]}}cstd-[[AST_HASH]].pcm'
+// CHECK-NOT: building module 'cstd' as '{{.*[/\\]}}[[CONTEXT_HASH]]{{[/\\]}}
+// CHECK: cstd-[[AST_HASH]].pcm'
+// CHECK-NOT: building module 'cstd' as '{{.*[/\\]}}[[CONTEXT_HASH]]{{[/\\]}}
+// CHECK: cstd-[[AST_HASH]].pcm'