[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

*whether

209

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

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?

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_hash : Flag<["-"], "fmodules-strict-hash">,
+  HelpText<"Enable hashing of all options that impact modules">;
 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">;

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;
 
+  /// Weather we should include all things that could impact the module in the
+  /// hash.
+  ///
+  /// This includes things like the full header search path.
+  unsigned ModulesStrictHash : 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), UseDebugInfo(false),
-        ModulesValidateDiagnosticOptions(true), ModulesHashContent(false) {}
+        ModulesValidateDiagnosticOptions(true), ModulesHashContent(false),
+        ModulesStrictHash(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

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.ModulesStrictHash = Args.hasArg(OPT_fmodules_strict_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.ModulesStrictHash) {
+    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);
+  }
+
   // 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);
   }

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-hash -Rmodule-build 2> %t3
+// RUN: echo %t | cat - %t1 %t2 %t3 | FileCheck %s
+
+// This test verifies that only strict hashing includes search paths 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'