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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM, if you expand the comment (see inline).



================
Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:56-58
+  // Ensure the reported context hash is strict.
+  CI.getHeaderSearchOpts().ModulesStrictContextHash = true;
+
----------------
jansvoboda11 wrote:
> 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.
I think I understand the problem now. I hadn't really put together that the implicit modules context hash machinery was being used to decide the artifact location for the explicit module.

I'm concerned this is too subtle and fragile. I'm wondering if the following more naive solution would work:
- Prune/canonicalize the CompilerInvocation (as now).
- Write/modify any fields to use placeholders for fields the client has control over. (Besides `OutputFile`, what else is there?)
- Generate the `-cc1` and hash it. That's now the context hash. Return it to the client.

But maybe that's the whole point of the strict context hash, and if there are bugs where this would behave differently, we should fix the strict context hash?

...

Stepping back, please go ahead and commit this incremental improvement, after expanding the comment to:
1. More fully explain the context: that the context hash will be generated from the CompilerInvocation and sent to the client, which then uses it to decide where to store the artifact. We need to make sure it's strict.
2. Explain why `assert(ModulesStrictContextHash)` would fail, even though the scan used a strict context hash. (Maybe it'd makes sense to make that change in a follow-up...?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111720