Re: [PATCH] D68528: [Implicit Modules] Add -cc1 option -fmodules-strict-hash which includes search paths and diagnostics.
On Fri, Oct 18, 2019 at 7:38 PM Nico Weber via Phabricator < revi...@reviews.llvm.org> wrote: > thakis added a comment. > > 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? > > Reverted in r375338. I'll fix it locally and recommit. - Michael Spencer ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D68528: [Implicit Modules] Add -cc1 option -fmodules-strict-hash which includes search paths and diagnostics.
thakis added a comment. 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? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68528/new/ https://reviews.llvm.org/D68528 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D68528: [Implicit Modules] Add -cc1 option -fmodules-strict-hash which includes search paths and diagnostics.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG14a3f77ba103: [Implicit Modules] Add -cc1 option
-fmodules-strict-context-hash which includes… (authored by Bigcheese).
Changed prior to commit:
https://reviews.llvm.org/D68528?vs=225326=225731#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68528/new/
https://reviews.llvm.org/D68528
Files:
clang/docs/Modules.rst
clang/include/clang/Driver/CC1Options.td
clang/include/clang/Lex/HeaderSearchOptions.h
clang/lib/Frontend/CompilerInvocation.cpp
clang/test/Modules/context-hash.c
Index: clang/test/Modules/context-hash.c
===
--- /dev/null
+++ clang/test/Modules/context-hash.c
@@ -0,0 +1,33 @@
+// 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
+
+// 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'
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2067,6 +2067,7 @@
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);
@@ -3546,6 +3547,7 @@
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
@@ -3600,6 +3602,24 @@
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 = 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);
Index: clang/include/clang/Lex/HeaderSearchOptions.h
===
--- clang/include/clang/Lex/HeaderSearchOptions.h
+++ clang/include/clang/Lex/HeaderSearchOptions.h
@@ -11,6 +11,7 @@
#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
@@ -206,6 +207,13 @@
unsigned ModulesHashContent : 1;
+ ///
[PATCH] D68528: [Implicit Modules] Add -cc1 option -fmodules-strict-hash which includes search paths and diagnostics.
bruno added a comment. > 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! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68528/new/ https://reviews.llvm.org/D68528 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D68528: [Implicit Modules] Add -cc1 option -fmodules-strict-hash which includes search paths and diagnostics.
Bigcheese added a comment. 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68528/new/ https://reviews.llvm.org/D68528 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D68528: [Implicit Modules] Add -cc1 option -fmodules-strict-hash which includes search paths and diagnostics.
bruno added a comment. *using implicit modules in a build where compiler flags in different invocations aren't homogeneous, or something along those lines. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68528/new/ https://reviews.llvm.org/D68528 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D68528: [Implicit Modules] Add -cc1 option -fmodules-strict-hash which includes search paths and diagnostics.
bruno accepted this revision. bruno added a comment. This revision is now accepted and ready to land. 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? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68528/new/ https://reviews.llvm.org/D68528 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D68528: [Implicit Modules] Add -cc1 option -fmodules-strict-hash which includes search paths and diagnostics.
Bigcheese updated this revision to Diff 225326.
Bigcheese marked an inline comment as done.
Bigcheese added a comment.
Fixed spelling and updated comment.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68528/new/
https://reviews.llvm.org/D68528
Files:
include/clang/Driver/CC1Options.td
include/clang/Lex/HeaderSearchOptions.h
lib/Frontend/CompilerInvocation.cpp
test/Modules/context-hash.c
Index: test/Modules/context-hash.c
===
--- /dev/null
+++ test/Modules/context-hash.c
@@ -0,0 +1,33 @@
+// 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: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -Weverything -internal-isystem \
+// RUN: %S/Inputs/System/usr/include -fmodules -fmodules-strict-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
+
+// 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'
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -2065,6 +2065,7 @@
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);
@@ -3544,6 +3545,7 @@
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
@@ -3598,6 +3600,24 @@
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);
+
+const DiagnosticOptions = 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);
Index: include/clang/Lex/HeaderSearchOptions.h
===
--- include/clang/Lex/HeaderSearchOptions.h
+++ include/clang/Lex/HeaderSearchOptions.h
@@ -11,6 +11,7 @@
#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
@@ -206,6 +207,13 @@
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 ModulesStrictHash : 1;
+
HeaderSearchOptions(StringRef _Sysroot = "/")
: Sysroot(_Sysroot),
[PATCH] D68528: [Implicit Modules] Add -cc1 option -fmodules-strict-hash which includes search paths and diagnostics.
Bigcheese marked an inline comment as done. Bigcheese added inline comments. Comment at: include/clang/Lex/HeaderSearchOptions.h:209 + /// + /// This includes things like the full header search path. + unsigned ModulesStrictHash : 1; bruno wrote: > 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? 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68528/new/ https://reviews.llvm.org/D68528 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D68528: [Implicit Modules] Add -cc1 option -fmodules-strict-hash which includes search paths and diagnostics.
bruno added a comment. Hi Michael, thanks for working on this! Comment at: include/clang/Lex/HeaderSearchOptions.h:206 + /// Weather we should include all things that could impact the module in the + /// hash. *whether Comment at: include/clang/Lex/HeaderSearchOptions.h:209 + /// + /// This includes things like the full header search path. + unsigned ModulesStrictHash : 1; 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? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68528/new/ https://reviews.llvm.org/D68528 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D68528: [Implicit Modules] Add -cc1 option -fmodules-strict-hash which includes search paths and diagnostics.
Bigcheese updated this revision to Diff 223345.
Bigcheese retitled this revision from "[Implicit Modules] Add -cc1 option
-fmodules-strict-hash which includes search paths." to "[Implicit Modules] Add
-cc1 option -fmodules-strict-hash which includes search paths and diagnostics.".
Bigcheese added a comment.
Add diagnostics.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68528/new/
https://reviews.llvm.org/D68528
Files:
include/clang/Driver/CC1Options.td
include/clang/Lex/HeaderSearchOptions.h
lib/Frontend/CompilerInvocation.cpp
test/Modules/context-hash.c
Index: test/Modules/context-hash.c
===
--- /dev/null
+++ test/Modules/context-hash.c
@@ -0,0 +1,33 @@
+// 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: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -Weverything -internal-isystem \
+// RUN: %S/Inputs/System/usr/include -fmodules -fmodules-strict-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
+
+// 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'
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -2060,6 +2060,7 @@
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);
@@ -3519,6 +3520,7 @@
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
@@ -3573,6 +3575,24 @@
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);
+
+const DiagnosticOptions = 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);
Index: include/clang/Lex/HeaderSearchOptions.h
===
--- include/clang/Lex/HeaderSearchOptions.h
+++ include/clang/Lex/HeaderSearchOptions.h
@@ -11,6 +11,7 @@
#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
@@ -202,6 +203,12 @@
unsigned ModulesHashContent : 1;
+ /// Weather we should include all things that could impact the module in the
+ /// hash.
+ ///
+ /// This includes things like the full header
