[PATCH] D111720: [clang][deps] Ensure reported context hash is strict

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.


================
Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:56-58
+  // Ensure the reported context hash is strict.
+  CI.getHeaderSearchOpts().ModulesStrictContextHash = true;
+
----------------
dexonsmith wrote:
> IIUC, explicit modules don't really have/need a context hash. Can related options be stripped out when serializing to `-cc1` when `ImplicitModules` is false?
> 
> Basically, I'm asking if `ModulesStrictContextHash` is a no-op when `ImplicitModules` is false. If not, can we make it a no-op?
> (If we can, then maybe rename the field to `ImplicitModulesStrictContextHash` and audit that no one reads it when `ImplicitModules` is off...)
Let me clarify this a bit. You're right that when building explicit modules, we don't care about context hash.

We do care about using **strict** context hash during the scan though - it's an implementation detail through which we prevent mixing incompatible modules/TUs. (This strict context hash is enabled elsewhere in the dependency scanner.)

At the end of the scan, we take discovered modules and modify/prune their `CompilerInvocation` (in this function). This can essentially "merge" multiple versions of the same module into one, which is very desirable. But we still want to do it according to the **strict** context hash. We don't want to merge versions with different search paths for example (non-strict context hash). That's what this change ensures.

Note that we don't **need** to report context hashes to scanner clients. Any other identifier derived from a strict context hash would work.


================
Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:56-58
+  // Ensure the reported context hash is strict.
+  CI.getHeaderSearchOpts().ModulesStrictContextHash = true;
+
----------------
jansvoboda11 wrote:
> dexonsmith wrote:
> > IIUC, explicit modules don't really have/need a context hash. Can related options be stripped out when serializing to `-cc1` when `ImplicitModules` is false?
> > 
> > Basically, I'm asking if `ModulesStrictContextHash` is a no-op when `ImplicitModules` is false. If not, can we make it a no-op?
> > (If we can, then maybe rename the field to `ImplicitModulesStrictContextHash` and audit that no one reads it when `ImplicitModules` is off...)
> Let me clarify this a bit. You're right that when building explicit modules, we don't care about context hash.
> 
> We do care about using **strict** context hash during the scan though - it's an implementation detail through which we prevent mixing incompatible modules/TUs. (This strict context hash is enabled elsewhere in the dependency scanner.)
> 
> At the end of the scan, we take discovered modules and modify/prune their `CompilerInvocation` (in this function). This can essentially "merge" multiple versions of the same module into one, which is very desirable. But we still want to do it according to the **strict** context hash. We don't want to merge versions with different search paths for example (non-strict context hash). That's what this change ensures.
> 
> Note that we don't **need** to report context hashes to scanner clients. Any other identifier derived from a strict context hash would work.
I think the rename you're suggesting is valid.

We //could// strip the `ModulesStrictContextHash` in the scanner: after we generate the strict context hash and before we generate the command-line. I think that can be done in a NFC follow-up.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111720/new/

https://reviews.llvm.org/D111720