[PATCH] D67010: [Modules] Move search paths from control block to unhashed control block

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno created this revision.
bruno added reviewers: dexonsmith, arphaman, rsmith.
Herald added subscribers: cfe-commits, ributzka, jkorous.
Herald added a project: clang.

Header search paths are currently ignored for the purpose of computing
`getModuleHash()` during a module build. This means that it doesn't
matter how much one changes header search paths in different clang
invocations, it will always use the same PCM.

This is not really correct but has been mostly "working" ever since.
However, when clang started signing the CONTROL_BLOCK section of the
PCM, which is where the header search paths live in the PCMs, we started
to get unexpected rebuilds when using implicit modules and `signature
mismatch` errors when those are rebuilt in the context of PCHs.

It's inconsistent that we ignore header search paths when selecting a
PCM to build/reuse but consider it as part of the signature of the PCM.
This patch proposes to move serialization of header search paths to
the UNHASHED_CONTROL_BLOCK instead, as we currently do for diagnostics.

Disconsidering the header search paths as part of the signature
shouldn't make it worse for correctness because if the header search
path would change anything, that would change the INPUT_FILE, which
*still is* part of the CONTROL_BLOCK, and therefore will trigger a
module rebuild.

Follow ups from this that might be interesting:

- Introduce -fmodules-strict-header-search, which does consider search paths in `getModuleHash()`. This would provide users a way out when they hit bugs related to header search paths, but with the downside of bigger module caches (in case those paths change often in their builds).
- Optimize the serialization of such paths to only consider the ones that actually contribute to the INPUT_LIST. This has the nice effect that a module only knows about the paths that are actually relevant to it, increase reusability.
- Add warnings or remarks when the header search paths in the PCM don't match the ones in the compiler.

There are no testcases for this change, as it would require using
llvm-bcanalyzer in clang tests, which we don't currently do.

rdar://problem/45567811


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D67010

Files:
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D67010.218130.patch
Type: text/x-patch
Size: 6700 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190830/81e9faa2/attachment.bin>