jansvoboda11 created this revision.
jansvoboda11 added reviewers: Bigcheese, dexonsmith, arphaman.
jansvoboda11 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
This patch fixes dependency scanning of a TU with prebuilt modular/PCH
dependencies.
Such modules/PCH might have been built using non-minimized files, while
dependency scanner does use the minimized versions of source files. Implicit
build doesn't like the discrepancy in file sizes and errors out.
The issues is worked around by padding the minimized files with whitespace in
such scenarios. This approach throws away one advantage of source minimization
(the memory savings), but still keeps the benefits of faster
preprocessing/lexing.
The padding solution comes with a 14% runtime regression compared to not
padding the inputs. (I tested this by forcing padding and running
`clang-scan-deps -j 20` on LLVM's `compile_commands.json` with modules
disabled.) Interestingly, padding only with newlines (compared to spaces), the
regression is 39%.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D104465
Files:
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
clang/test/ClangScanDeps/modules-pch.c
Index: clang/test/ClangScanDeps/modules-pch.c
===================================================================
--- clang/test/ClangScanDeps/modules-pch.c
+++ clang/test/ClangScanDeps/modules-pch.c
@@ -6,7 +6,7 @@
// RUN: sed "s|DIR|%/t|g" %S/Inputs/modules-pch/cdb_pch.json > %t/cdb.json
// RUN: echo -%t > %t/result_pch.json
// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full \
-// RUN: -generate-modules-path-args -module-files-dir %t/build -mode preprocess >> %t/result_pch.json
+// RUN: -generate-modules-path-args -module-files-dir %t/build >> %t/result_pch.json
// RUN: cat %t/result_pch.json | sed 's:\\\\\?:/:g' | FileCheck %s -check-prefix=CHECK-PCH
//
// Check we didn't build the PCH during dependency scanning.
@@ -127,9 +127,8 @@
//
// RUN: sed "s|DIR|%/t|g" %S/Inputs/modules-pch/cdb_tu.json > %t/cdb.json
// RUN: echo -%t > %t/result_tu.json
-// FIXME: Make this work with '-mode preprocess-minimized-sources'.
// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full \
-// RUN: -generate-modules-path-args -module-files-dir %t/build -mode preprocess >> %t/result_tu.json
+// RUN: -generate-modules-path-args -module-files-dir %t/build >> %t/result_tu.json
// RUN: cat %t/result_tu.json | sed 's:\\\\\?:/:g' | FileCheck %s -check-prefix=CHECK-TU
//
// CHECK-TU: -[[PREFIX:.*]]
@@ -193,7 +192,7 @@
// RUN: sed "s|DIR|%/t|g" %S/Inputs/modules-pch/cdb_tu_with_common.json > %t/cdb.json
// RUN: echo -%t > %t/result_tu_with_common.json
// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full \
-// RUN: -generate-modules-path-args -module-files-dir %t/build -mode preprocess >> %t/result_tu_with_common.json
+// RUN: -generate-modules-path-args -module-files-dir %t/build >> %t/result_tu_with_common.json
// RUN: cat %t/result_tu_with_common.json | sed 's:\\\\\?:/:g' | FileCheck %s -check-prefix=CHECK-TU-WITH-COMMON
//
// CHECK-TU-WITH-COMMON: -[[PREFIX:.*]]
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===================================================================
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -139,6 +139,15 @@
FileMgr->setVirtualFileSystem(createVFSFromCompilerInvocation(
CI, Compiler.getDiagnostics(), DepFS));
+ // If we are about to import a PCH, make sure minimized files are padded
+ // to sizes of the originals. The PCH might have been built from
+ // non-minimized files and any disagreement on the file size causes issues
+ // in the implicit build.
+ // TODO: Consider padding only files that contributed to the PCH and its
+ // modules.
+ DepFS->PadMinimizedToOriginalSize =
+ !Compiler.getPreprocessorOpts().ImplicitPCHInclude.empty();
+
// Pass the skip mappings which should speed up excluded conditional block
// skipping in the preprocessor.
if (PPSkipMappings)
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
===================================================================
--- clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -15,8 +15,10 @@
using namespace tooling;
using namespace dependencies;
-CachedFileSystemEntry CachedFileSystemEntry::createFileEntry(
- StringRef Filename, llvm::vfs::FileSystem &FS, bool Minimize) {
+CachedFileSystemEntry
+CachedFileSystemEntry::createFileEntry(StringRef Filename,
+ llvm::vfs::FileSystem &FS, bool Minimize,
+ bool PadMinimizedToOriginalSize) {
// Load the file and its content from the file system.
llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>> MaybeFile =
FS.openFileForRead(Filename);
@@ -37,7 +39,8 @@
const auto &Buffer = *MaybeBuffer;
SmallVector<minimize_source_to_dependency_directives::Token, 64> Tokens;
if (!Minimize || minimizeSourceToDependencyDirectives(
- Buffer->getBuffer(), MinimizedFileContents, Tokens)) {
+ Buffer->getBuffer(), MinimizedFileContents, Tokens,
+ nullptr, SourceLocation(), PadMinimizedToOriginalSize)) {
// Use the original file unless requested otherwise, or
// if the minimization failed.
// FIXME: Propage the diagnostic if desired by the client.
@@ -185,7 +188,7 @@
std::move(*MaybeStatus));
else
CacheEntry = CachedFileSystemEntry::createFileEntry(
- Filename, FS, !KeepOriginalSource);
+ Filename, FS, !KeepOriginalSource, PadMinimizedToOriginalSize);
}
Result = &CacheEntry;
Index: clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
===================================================================
--- clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -46,9 +46,10 @@
/// mismatching size of the file. If file is not minimized, the full file is
/// read and copied into memory to ensure that it's not memory mapped to avoid
/// running out of file descriptors.
- static CachedFileSystemEntry createFileEntry(StringRef Filename,
- llvm::vfs::FileSystem &FS,
- bool Minimize = true);
+ static CachedFileSystemEntry
+ createFileEntry(StringRef Filename, llvm::vfs::FileSystem &FS,
+ bool Minimize = true,
+ bool PadMinimizedToOriginalSize = false);
/// Create an entry that represents a directory on the filesystem.
static CachedFileSystemEntry createDirectoryEntry(llvm::vfs::Status &&Stat);
@@ -156,6 +157,9 @@
/// The set of files that should not be minimized.
llvm::StringSet<> IgnoredFiles;
+ /// Whether to pad minimized files to the original size.
+ bool PadMinimizedToOriginalSize = false;
+
private:
void setCachedEntry(StringRef Filename, const CachedFileSystemEntry *Entry) {
bool IsInserted = Cache.try_emplace(Filename, Entry).second;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
