thakis created this revision.
thakis added reviewers: saar.raz, rsmith.
thakis marked an inline comment as done.
thakis added inline comments.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3645
// CityHash, but this will do for now.
hash_code code = hash_value(getClangFullRepositoryVersion());
----------------
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.
With LLVM_APPEND_VC_REV=NO, Modules/merge-lifetime-extended-temporary.cpp
would fail if it ran before a0f50d731639350c7a7
<https://reviews.llvm.org/rGa0f50d731639350c7a79f140f026c27a18215531> (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.
2. 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
<https://reviews.llvm.org/rGa0f50d731639350c7a79f140f026c27a18215531> 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.)
https://reviews.llvm.org/D73202
Files:
clang/include/clang/Serialization/ASTBitCodes.h
clang/lib/Frontend/CompilerInvocation.cpp
clang/test/Modules/diagnostics.modulemap
clang/test/Modules/exception-spec.cpp
clang/test/Modules/merge-lifetime-extended-temporary.cpp
clang/test/Modules/objc-method-redecl.m
clang/test/Modules/using-decl-inheritance.cpp
Index: clang/test/Modules/using-decl-inheritance.cpp
===================================================================
--- clang/test/Modules/using-decl-inheritance.cpp
+++ clang/test/Modules/using-decl-inheritance.cpp
@@ -1,3 +1,4 @@
+// RUN: rm -rf %t
// RUN: %clang_cc1 -x c++ -fmodules -fmodules-local-submodule-visibility
-fmodules-cache-path=%t %s -verify
// RUN: %clang_cc1 -x c++ -fmodules -fmodules-cache-path=%t %s -verify
Index: clang/test/Modules/objc-method-redecl.m
===================================================================
--- clang/test/Modules/objc-method-redecl.m
+++ clang/test/Modules/objc-method-redecl.m
@@ -1,3 +1,4 @@
+// RUN: rm -rf %t
// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -x objective-c-header
-emit-pch %S/Inputs/objc-method-redecl.h -o %t.pch -Wno-objc-root-class
// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -x objective-c
-include-pch %t.pch %s -verify -Wno-objc-root-class
// expected-no-diagnostics
Index: clang/test/Modules/merge-lifetime-extended-temporary.cpp
===================================================================
--- clang/test/Modules/merge-lifetime-extended-temporary.cpp
+++ clang/test/Modules/merge-lifetime-extended-temporary.cpp
@@ -1,3 +1,4 @@
+// RUN: rm -rf %t
// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -x
c++ -I%S/Inputs/merge-lifetime-extended-temporary -verify -std=c++11 %s
-DORDER=1
// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -x
c++ -I%S/Inputs/merge-lifetime-extended-temporary -verify -std=c++11 %s
-DORDER=2
Index: clang/test/Modules/exception-spec.cpp
===================================================================
--- clang/test/Modules/exception-spec.cpp
+++ clang/test/Modules/exception-spec.cpp
@@ -1,3 +1,4 @@
+// RUN: rm -rf %t
// RUN: %clang_cc1 -x c++ -std=c++17 -fmodules
-fmodules-local-submodule-visibility -fmodules-cache-path=%t %s -verify
// expected-no-diagnostics
Index: clang/test/Modules/diagnostics.modulemap
===================================================================
--- clang/test/Modules/diagnostics.modulemap
+++ clang/test/Modules/diagnostics.modulemap
@@ -1,3 +1,4 @@
+// RUN: rm -rf %t
// RUN: not %clang_cc1 -fmodules -fmodules-cache-path=%t
-fmodule-map-file=%S/Inputs/diagnostics-aux.modulemap -fmodule-map-file=%s
-fsyntax-only -x c++ /dev/null 2>&1 | FileCheck %s --implicit-check-not error:
// CHECK: In file included from {{.*}}diagnostics-aux.modulemap:3:
Index: clang/lib/Frontend/CompilerInvocation.cpp
===================================================================
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -41,6 +41,7 @@
#include "clang/Lex/HeaderSearchOptions.h"
#include "clang/Lex/PreprocessorOptions.h"
#include "clang/Sema/CodeCompleteOptions.h"
+#include "clang/Serialization/ASTBitCodes.h"
#include "clang/Serialization/ModuleFileExtension.h"
#include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
#include "llvm/ADT/APInt.h"
@@ -3643,6 +3644,11 @@
// CityHash, but this will do for now.
hash_code code = hash_value(getClangFullRepositoryVersion());
+ // Also include the serialization version, in case LLVM_APPEND_VC_REV is off
+ // and getClangFullRepositoryVersion() doesn't include git revision.
+ code = hash_combine(code, serialization::VERSION_MAJOR,
+ serialization::VERSION_MINOR);
+
// Extend the signature with the language options
#define LANGOPT(Name, Bits, Default, Description) \
code = hash_combine(code, LangOpts->Name);
Index: clang/include/clang/Serialization/ASTBitCodes.h
===================================================================
--- clang/include/clang/Serialization/ASTBitCodes.h
+++ clang/include/clang/Serialization/ASTBitCodes.h
@@ -41,7 +41,7 @@
/// Version 4 of AST files also requires that the version control branch
and
/// revision match exactly, since there is no backward compatibility of
/// AST files at this time.
- const unsigned VERSION_MAJOR = 8;
+ const unsigned VERSION_MAJOR = 9;
/// AST file minor version number supported by this version of
/// Clang.
Index: clang/test/Modules/using-decl-inheritance.cpp
===================================================================
--- clang/test/Modules/using-decl-inheritance.cpp
+++ clang/test/Modules/using-decl-inheritance.cpp
@@ -1,3 +1,4 @@
+// RUN: rm -rf %t
// RUN: %clang_cc1 -x c++ -fmodules -fmodules-local-submodule-visibility -fmodules-cache-path=%t %s -verify
// RUN: %clang_cc1 -x c++ -fmodules -fmodules-cache-path=%t %s -verify
Index: clang/test/Modules/objc-method-redecl.m
===================================================================
--- clang/test/Modules/objc-method-redecl.m
+++ clang/test/Modules/objc-method-redecl.m
@@ -1,3 +1,4 @@
+// RUN: rm -rf %t
// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -x objective-c-header -emit-pch %S/Inputs/objc-method-redecl.h -o %t.pch -Wno-objc-root-class
// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -x objective-c -include-pch %t.pch %s -verify -Wno-objc-root-class
// expected-no-diagnostics
Index: clang/test/Modules/merge-lifetime-extended-temporary.cpp
===================================================================
--- clang/test/Modules/merge-lifetime-extended-temporary.cpp
+++ clang/test/Modules/merge-lifetime-extended-temporary.cpp
@@ -1,3 +1,4 @@
+// RUN: rm -rf %t
// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -x c++ -I%S/Inputs/merge-lifetime-extended-temporary -verify -std=c++11 %s -DORDER=1
// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -x c++ -I%S/Inputs/merge-lifetime-extended-temporary -verify -std=c++11 %s -DORDER=2
Index: clang/test/Modules/exception-spec.cpp
===================================================================
--- clang/test/Modules/exception-spec.cpp
+++ clang/test/Modules/exception-spec.cpp
@@ -1,3 +1,4 @@
+// RUN: rm -rf %t
// RUN: %clang_cc1 -x c++ -std=c++17 -fmodules -fmodules-local-submodule-visibility -fmodules-cache-path=%t %s -verify
// expected-no-diagnostics
Index: clang/test/Modules/diagnostics.modulemap
===================================================================
--- clang/test/Modules/diagnostics.modulemap
+++ clang/test/Modules/diagnostics.modulemap
@@ -1,3 +1,4 @@
+// RUN: rm -rf %t
// RUN: not %clang_cc1 -fmodules -fmodules-cache-path=%t -fmodule-map-file=%S/Inputs/diagnostics-aux.modulemap -fmodule-map-file=%s -fsyntax-only -x c++ /dev/null 2>&1 | FileCheck %s --implicit-check-not error:
// CHECK: In file included from {{.*}}diagnostics-aux.modulemap:3:
Index: clang/lib/Frontend/CompilerInvocation.cpp
===================================================================
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -41,6 +41,7 @@
#include "clang/Lex/HeaderSearchOptions.h"
#include "clang/Lex/PreprocessorOptions.h"
#include "clang/Sema/CodeCompleteOptions.h"
+#include "clang/Serialization/ASTBitCodes.h"
#include "clang/Serialization/ModuleFileExtension.h"
#include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
#include "llvm/ADT/APInt.h"
@@ -3643,6 +3644,11 @@
// CityHash, but this will do for now.
hash_code code = hash_value(getClangFullRepositoryVersion());
+ // Also include the serialization version, in case LLVM_APPEND_VC_REV is off
+ // and getClangFullRepositoryVersion() doesn't include git revision.
+ code = hash_combine(code, serialization::VERSION_MAJOR,
+ serialization::VERSION_MINOR);
+
// Extend the signature with the language options
#define LANGOPT(Name, Bits, Default, Description) \
code = hash_combine(code, LangOpts->Name);
Index: clang/include/clang/Serialization/ASTBitCodes.h
===================================================================
--- clang/include/clang/Serialization/ASTBitCodes.h
+++ clang/include/clang/Serialization/ASTBitCodes.h
@@ -41,7 +41,7 @@
/// Version 4 of AST files also requires that the version control branch and
/// revision match exactly, since there is no backward compatibility of
/// AST files at this time.
- const unsigned VERSION_MAJOR = 8;
+ const unsigned VERSION_MAJOR = 9;
/// AST file minor version number supported by this version of
/// Clang.
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
