arames added a comment. In D102943#2775131 <https://reviews.llvm.org/D102943#2775131>, @dexonsmith wrote:
> In D102943#2775115 <https://reviews.llvm.org/D102943#2775115>, @pcc wrote: > >> Isn't the bug here that module hashing is using `hash_code`? So shouldn't >> the correct fix be to use a specific hashing algorithm for module hashes? > > Yeah, I tend to agree. I thought modules already used MD5, but maybe just for > the AST signature. For reference, the issue I am trying to fix stems from the hash computed as part of CompilerInvocation::getModuleHash() <https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/CompilerInvocation.cpp#L4464>. Reading the doc around `llvm::hash_code`, my first reaction was indeed that modules may have to use a different hash. I thought the current proposal was something to consider, because: 1. Practically, everything is done on `uint64_t`, so I would expect switching from `size_t` to `uint64_t` to have little to no impact on non-64bit targets. 2. The current modules code kind of already assumes `hash_code` is stable over different executions. Plain simple use-case with implicit modules would be severely deficient if the hash was not stable across executions. To be fair, the assumption is different from the one I am trying to fix of 32bit vs 64bit. Alternatively, we could introduce something along the lines of `llvm::stable_hash_code` (other name suggestions welcome). We can probably restructure the code with very few changes. (Really, it looks like we only need a configurable storage type, and allow skipping the execution seed part.) Would you think this is a better approach ? Or do you have something else to suggest ? Cheers Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102943/new/ https://reviews.llvm.org/D102943 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
