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
Index: clang/lib/Serialization/ASTWriter.cpp
===================================================================
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1327,6 +1327,7 @@
BLOCK(UNHASHED_CONTROL_BLOCK);
RECORD(SIGNATURE);
RECORD(DIAGNOSTIC_OPTIONS);
+ RECORD(HEADER_SEARCH_PATHS);
RECORD(DIAG_PRAGMA_MAPPINGS);
#undef RECORD
@@ -1446,6 +1447,29 @@
// are generally transient files and will almost always be overridden.
Stream.EmitRecord(DIAGNOSTIC_OPTIONS, Record);
+ // Header search paths.
+ Record.clear();
+ const HeaderSearchOptions &HSOpts
+ = PP.getHeaderSearchInfo().getHeaderSearchOpts();
+
+ // Include entries.
+ Record.push_back(HSOpts.UserEntries.size());
+ for (unsigned I = 0, N = HSOpts.UserEntries.size(); I != N; ++I) {
+ const HeaderSearchOptions::Entry &Entry = HSOpts.UserEntries[I];
+ AddString(Entry.Path, Record);
+ Record.push_back(static_cast<unsigned>(Entry.Group));
+ Record.push_back(Entry.IsFramework);
+ Record.push_back(Entry.IgnoreSysRoot);
+ }
+
+ // System header prefixes.
+ Record.push_back(HSOpts.SystemHeaderPrefixes.size());
+ for (unsigned I = 0, N = HSOpts.SystemHeaderPrefixes.size(); I != N; ++I) {
+ AddString(HSOpts.SystemHeaderPrefixes[I].Prefix, Record);
+ Record.push_back(HSOpts.SystemHeaderPrefixes[I].IsSystemHeader);
+ }
+ Stream.EmitRecord(HEADER_SEARCH_PATHS, Record);
+
// Write out the diagnostic/pragma mappings.
WritePragmaDiagnosticMappings(Diags, /* isModule = */ WritingModule);
@@ -1649,25 +1673,8 @@
Record.clear();
const HeaderSearchOptions &HSOpts
= PP.getHeaderSearchInfo().getHeaderSearchOpts();
- AddString(HSOpts.Sysroot, Record);
-
- // Include entries.
- Record.push_back(HSOpts.UserEntries.size());
- for (unsigned I = 0, N = HSOpts.UserEntries.size(); I != N; ++I) {
- const HeaderSearchOptions::Entry &Entry = HSOpts.UserEntries[I];
- AddString(Entry.Path, Record);
- Record.push_back(static_cast<unsigned>(Entry.Group));
- Record.push_back(Entry.IsFramework);
- Record.push_back(Entry.IgnoreSysRoot);
- }
-
- // System header prefixes.
- Record.push_back(HSOpts.SystemHeaderPrefixes.size());
- for (unsigned I = 0, N = HSOpts.SystemHeaderPrefixes.size(); I != N; ++I) {
- AddString(HSOpts.SystemHeaderPrefixes[I].Prefix, Record);
- Record.push_back(HSOpts.SystemHeaderPrefixes[I].IsSystemHeader);
- }
+ AddString(HSOpts.Sysroot, Record);
AddString(HSOpts.ResourceDir, Record);
AddString(HSOpts.ModuleCachePath, Record);
AddString(HSOpts.ModuleUserBuildPath, Record);
Index: clang/lib/Serialization/ASTReader.cpp
===================================================================
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -4642,6 +4642,13 @@
Result = OutOfDate; // Don't return early. Read the signature.
break;
}
+ case HEADER_SEARCH_PATHS: {
+ bool Complain = (ClientLoadCapabilities & ARR_ConfigurationMismatch) == 0;
+ if (!AllowCompatibleConfigurationMismatch &&
+ ParseHeaderSearchPaths(Record, Complain, *Listener))
+ Result = ConfigurationMismatch;
+ break;
+ }
case DIAG_PRAGMA_MAPPINGS:
if (!F)
break;
@@ -5706,6 +5713,27 @@
HeaderSearchOptions HSOpts;
unsigned Idx = 0;
HSOpts.Sysroot = ReadString(Record, Idx);
+ HSOpts.ResourceDir = ReadString(Record, Idx);
+ HSOpts.ModuleCachePath = ReadString(Record, Idx);
+ HSOpts.ModuleUserBuildPath = ReadString(Record, Idx);
+ HSOpts.DisableModuleHash = Record[Idx++];
+ HSOpts.ImplicitModuleMaps = Record[Idx++];
+ HSOpts.ModuleMapFileHomeIsCwd = Record[Idx++];
+ HSOpts.UseBuiltinIncludes = Record[Idx++];
+ HSOpts.UseStandardSystemIncludes = Record[Idx++];
+ HSOpts.UseStandardCXXIncludes = Record[Idx++];
+ HSOpts.UseLibcxx = Record[Idx++];
+ std::string SpecificModuleCachePath = ReadString(Record, Idx);
+
+ return Listener.ReadHeaderSearchOptions(HSOpts, SpecificModuleCachePath,
+ Complain);
+}
+
+bool ASTReader::ParseHeaderSearchPaths(const RecordData &Record,
+ bool Complain,
+ ASTReaderListener &Listener) {
+ HeaderSearchOptions HSOpts;
+ unsigned Idx = 0;
// Include entries.
for (unsigned N = Record[Idx++]; N; --N) {
@@ -5725,20 +5753,8 @@
HSOpts.SystemHeaderPrefixes.emplace_back(std::move(Prefix), IsSystemHeader);
}
- HSOpts.ResourceDir = ReadString(Record, Idx);
- HSOpts.ModuleCachePath = ReadString(Record, Idx);
- HSOpts.ModuleUserBuildPath = ReadString(Record, Idx);
- HSOpts.DisableModuleHash = Record[Idx++];
- HSOpts.ImplicitModuleMaps = Record[Idx++];
- HSOpts.ModuleMapFileHomeIsCwd = Record[Idx++];
- HSOpts.UseBuiltinIncludes = Record[Idx++];
- HSOpts.UseStandardSystemIncludes = Record[Idx++];
- HSOpts.UseStandardCXXIncludes = Record[Idx++];
- HSOpts.UseLibcxx = Record[Idx++];
- std::string SpecificModuleCachePath = ReadString(Record, Idx);
-
- return Listener.ReadHeaderSearchOptions(HSOpts, SpecificModuleCachePath,
- Complain);
+ // TODO: implement checking and warnings for path mismatches.
+ return false;
}
bool ASTReader::ParsePreprocessorOptions(const RecordData &Record,
Index: clang/include/clang/Serialization/ASTReader.h
===================================================================
--- clang/include/clang/Serialization/ASTReader.h
+++ clang/include/clang/Serialization/ASTReader.h
@@ -1321,6 +1321,8 @@
ASTReaderListener &Listener);
static bool ParseHeaderSearchOptions(const RecordData &Record, bool Complain,
ASTReaderListener &Listener);
+ static bool ParseHeaderSearchPaths(const RecordData &Record, bool Complain,
+ ASTReaderListener &Listener);
static bool ParsePreprocessorOptions(const RecordData &Record, bool Complain,
ASTReaderListener &Listener,
std::string &SuggestedPredefines);
Index: clang/include/clang/Serialization/ASTBitCodes.h
===================================================================
--- clang/include/clang/Serialization/ASTBitCodes.h
+++ clang/include/clang/Serialization/ASTBitCodes.h
@@ -365,6 +365,9 @@
/// Record code for the diagnostic options table.
DIAGNOSTIC_OPTIONS,
+ /// Record code for the headers search paths.
+ HEADER_SEARCH_PATHS,
+
/// Record code for \#pragma diagnostic mappings.
DIAG_PRAGMA_MAPPINGS,
};
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
