[PATCH] D67249: [Modules][PCH] Hash input files content

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno marked 2 inline comments as done.
bruno added a comment.



> Nice! Are you planning to address the FIXME's in a later update of this patch?

The FIXME's are just replaying what the code around does, both error dropping and `FileEntryRef` are recent changes that didn't make all the way through yet. I should help improving that at some point cause it will overall be good for modules, but those changes are orthogonal to this patch.



================
Comment at: clang/lib/Serialization/ASTWriter.cpp:1767
   bool IsTopLevelModuleMap;
+  uint32_t ContentHash[2];
 };
----------------
aprantl wrote:
> Why is this not a uint64_t?
Serializing a `uint64_t` directly instead of two `uint32_t` gives me a slightly bigger final cache. One could argue that the value is negligible (+800 bytes for a 40MB cache), but here's the rationale :)


================
Comment at: clang/test/Modules/validate-file-content.m:14
+// RUN: %clang_cc1 -emit-pch -fmodules-cache-path=%t/cache -fmodules -fimplicit-module-maps -o %t/a.pch -I %t -x objective-c-header %t/a.h -fvalidate-ast-input-files-content
+// RUN: touch -m -a -A 01 %t/m.h
+// RUN: %clang_cc1 -fsyntax-only -fmodules-cache-path=%t/cache -fmodules -fimplicit-module-maps -I %t -include-pch %t/a.pch %s -verify -fvalidate-ast-input-files-content
----------------
aprantl wrote:
> Are there other tests that use touch and file timestamps in general? I wonder if this could cause random issues when building on NFS or filesystems with a really low timestamp granularity. I don't have a better suggestion either since the mtime is part of the code being tested here.
> Are there other tests that use touch and file timestamps in general?

Yes, I found quite a few around the tests. However, I just tested `touch -m -a -A 01` on linux and it seems that `-A` isn't supported, maybe I'll just change the entire mtime for some time in the future, that's likely more portable.

> I wonder if this could cause random issues when building on NFS or filesystems with a really low timestamp granularity. I don't have a better suggestion either since the mtime is part of the code being tested here.

Right, perhaps changing the mtime completely might help here.





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67249