[PATCH] D129389: [clang][deps] Override dependency and serialized diag files for modules

Tue Jul 12 08:34:57 PDT 2022

benlangmuir added inline comments.

================
Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:341-342
+      !MDC.OriginalInvocation.getDependencyOutputOpts().OutputFile.empty();
+  // FIXME: HadSerializedDiagnostics and HadDependencyFile should be included in
+  // the context hash since it can affect the command-line.
   MD.ID.ContextHash = MD.BuildInvocation.getModuleHash();
----------------
jansvoboda11 wrote:
> This is a bit unfortunate but I don't see a better alternative.
> 
> Ideally, we would compute the hash with the `.d` and `.dia` paths already filled in. That would ensure the command line we end up reporting to the build system really **does** have the context hash associated with the module. (We'd need to include every field set in `getCanonicalCommandLine()` too.) But for the path lookup, we already need some kind of (currently partial) context hash.
The other things added in getCanonicalCommandLine are currently:
* module map path - I'm planning to include this in my changes to the context hash since it is significant exactly how the path is spelled. This is already part of the implicit module build's notion of identity.
* pcm paths - these are sound as long as we always get the same paths returned in the callback during a build (across separate builds it would be fine to change them as long as you're going to rebuild anything whose path changed, and anything downstream of that).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129389