This is an archive of the discontinued LLVM Phabricator instance.

Make AST reading work better with LLVM_APPEND_VC_REV=NO
ClosedPublic

Authored by thakis on Jan 22 2020, 7:56 AM.

Details

Summary

With LLVM_APPEND_VC_REV=NO, Modules/merge-lifetime-extended-temporary.cpp
would fail if it ran before a0f50d731639350c7a7 (which changed
the serialization format) and then after, for these reasons:

  1. With LLVM_APPEND_VC_REV=NO, the module hash before and after the change was the same.
  1. Modules/merge-lifetime-extended-temporary.cpp is the only test we have that uses -fmodule-cache-path=%t that a) actually writes to the cache path b) doesn't do rm -rf %t at the top of the test

So the old run would write a module file, and then the new run would
try to load it, but the serialized format changed.

Do several things to fix this:

  1. Include clang::serialization::VERSION_MAJOR/VERSION_MINOR in the module hash, so that when the AST format changes (...and we remember to bump these), we use a different module cache dir.
  2. Bump VERSION_MAJOR, since a0f50d731639350c7a7 changed the on-disk format in a way that a gch file written before that change can't be read after that change.
  3. Add rm -rf %t to all tests that pass -fmodule-cache-path=%t. This is unneccessary from a correctness PoV after 1 and 2, but makes it so that we don't amass many cache dirs over time. (Arguably, it also makes it so that the test suite doesn't catch when we change the serialization format but don't bump clang::serialization::VERSION_MAJOR/VERSION_MINOR; oh well.)

Diff Detail

Event Timeline

thakis marked an inline comment as done.Jan 22 2020, 7:59 AM
thakis added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
3637

Arguably, we should omit the full repo version from the hash: There's no reason to use a new cache dir just because someone fixed a typo in the (say) mlir docs. We'd also be better about remembering to bump clang::serialization::VERSION_MAJOR/MINOR in that case.

thakis added a reviewer: rnk.Jan 27 2020, 6:55 AM

rnk, could you maybe take a look?

rnk accepted this revision.Jan 27 2020, 4:10 PM

lgtm

This revision is now accepted and ready to land.Jan 27 2020, 4:10 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2020, 7:12 PM