[clang][deps] Prevent PCH validation failures by padding minimized files

[jansvoboda11]

Summary

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%.

Depends on D104462.

[dexonsmith]

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.

I'm not sure this is the right approach.

• Don't PCHs sometimes validate based on hashes of inputs instead of size? At least, that has been discussed, and this patch would block making the change.
• Can PCHs embed inputs, like modules can? How would that work (or not) minimized sources generally?
• What happens if the PCH has a delayed diagnostic, triggered when parsing the next thing? Will the fix-it point in the wrong place? (Are there other things that depend on reading the original sources when reading a PCH?)

A few other ideas:

1. Disable PCH validation when minimizing. (Is that less robust than your current workaround? If so, can you explain why?)
2. Use the original PCH header in the scanning -cc1s (translate -include-pch to -include) and switch back in the generated -cc1s (back to -include-pch).
3. Embed instructions in the PCH for how to build it, and make a "minimized" version of the PCH.

[dexonsmith]

In D104465#2825343, @dexonsmith wrote:

A few other ideas:

1. Disable PCH validation when minimizing. (Is that less robust than your current workaround? If so, can you explain why?)
2. Use the original PCH header in the scanning -cc1s (translate -include-pch to -include) and switch back in the generated -cc1s (back to -include-pch).
3. Embed instructions in the PCH for how to build it, and make a "minimized" version of the PCH.

Two more options to consider:

4. If a compilation uses a PCH, use the original files for any input file transitively referenced by the PCH. Can be done by using a filesystem overlay that skips over the minimization layer for those files, or could tell the minimization layer not to minimize those files. You can figure out the files by adding a free function that preloads the PCH and extracts out all the input files.
5. Add support for building/using a PCH, and only support PCHes that are built this way. E.g., add a -include-pch-auto option or something. Scanner would use -include, spit out a command for building a PCH using explicit modules, and the generated -cc1s would use -include-pch to refer to it. This way all the modules are generated "in house".

[jansvoboda11]

Thanks for sharing your ideas! I've left my initial thoughts below, but I want to revisit this and think about it some more.

In D104465#2825343, @dexonsmith wrote:

I'm not sure this is the right approach.

• Don't PCHs sometimes validate based on hashes of inputs instead of size? At least, that has been discussed, and this patch would block making the change.

The hash-based validation only kicks in when modification times don't match. The size of input files is always* validated.

• Can PCHs embed inputs, like modules can? How would that work (or not) minimized sources generally?

I think PCHs can embed inputs the same way modules can. When reading an AST file, the embedded input files get registered in the current compilation along with their buffers via SourceManager::createFileID.
But it doesn't seem to be propagated to the FileManager level, which the input file size check uses. I might need to experiment with this a bit to fully understand how this works in practice.

• What happens if the PCH has a delayed diagnostic, triggered when parsing the next thing? Will the fix-it point in the wrong place? (Are there other things that depend on reading the original sources when reading a PCH?)

Can you point me to the code that handles delayed diagnostics? I can't find anything related in ASTWriter.

A few other ideas:

1. Disable PCH validation when minimizing. (Is that less robust than your current workaround? If so, can you explain why?)

I think PCH validation (and detection of out-of-date input files) is still useful for the dependency scanner as it might help debugging build failures (e.g. if the input files changed between explicit PCH build and TU dependency scan).
Compared to disabling PCH validation, padding of minimized files is fairly local change that isn't susceptible to concurrency bugs and integrates with the rest of the implicit build infrastructure fairly well IMO.

2. Use the original PCH header in the scanning -cc1s (translate -include-pch to -include) and switch back in the generated -cc1s (back to -include-pch).

The two separate dependency scans (one for PCH, one for TU) could end up using different context hashes and we don't have an easy way to detect this.
If they both use a common module A, the explicitly-built PCH would contains version 1 of A, while the TU would attempt to build with version 2. This is currently not supported in explicit builds AFAIK.
The current implementation works around this by always choosing the module present in the explicitly-built module. How could we make this situation work here?

3. Embed instructions in the PCH for how to build it, and make a "minimized" version of the PCH.

That could work. This makes me think if we could emit a minimized PCH during the PCH scan and just reuse it in the TU scan (similarly how we're currently reusing the explicitly-built PCH, but with the file size issues gone). Though passing state from one dependency scan to another through the filesystem might be unreliable.

In D104465#2825786, @dexonsmith wrote:

Two more options to consider:

4. If a compilation uses a PCH, use the original files for any input file transitively referenced by the PCH. Can be done by using a filesystem overlay that skips over the minimization layer for those files, or could tell the minimization layer not to minimize those files. You can figure out the files by adding a free function that preloads the PCH and extracts out all the input files.

I tend to prefer this option, since it integrates fairly well with the current way we build PCHs in XCBuild.
I have a WIP patch that implements this: D104536. I plan to run some benchmarks to see the performance impact compared to minification + padding.

5. Add support for building/using a PCH, and only support PCHes that are built this way. E.g., add a -include-pch-auto option or something. Scanner would use -include, spit out a command for building a PCH using explicit modules, and the generated -cc1s would use -include-pch to refer to it. This way all the modules are generated "in house".

This would be the ideal solution IMO, but needs build system support.

---

BTW: I noticed the file size stored in AST also plays role when deciding whether to import or include header. HeaderSearch::findModuleForHeader calls HeaderSearch::getExistingFileInfo which pulls information (such as the file size) from ExternalSource (a.k.a. ASTReader).

clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h

public:
   /// Create an entry that represents an opened source file with minimized or
   /// original contents.
   ///
   /// The filesystem opens the file even for `stat` calls open to avoid the
   /// issues with stat + open of minimized files that might lead to a
   /// 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);
 
   /// \returns True if the entry is valid.
   bool isValid() const { return !MaybeStat || MaybeStat->isStatusKnown(); }
 
   /// \returns True if the current entry points to a directory.
public:
 
   llvm::ErrorOr<llvm::vfs::Status> status(const Twine &Path) override;
   llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
   openFileForRead(const Twine &Path) override;
 
   /// 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;
     (void)IsInserted;
     assert(IsInserted && "local cache is updated more than once");
   }
 
   const CachedFileSystemEntry *getCachedEntry(StringRef Filename) {

clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

 #include "clang/Lex/DependencyDirectivesSourceMinimizer.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Threading.h"
 
 using namespace clang;
 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);
   if (!MaybeFile)
     return MaybeFile.getError();
   llvm::ErrorOr<llvm::vfs::Status> Stat = (*MaybeFile)->status();
   if (!Stat)
     return Stat.getError();
 
   llvm::vfs::File &F = **MaybeFile;
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> MaybeBuffer =
       F.getBuffer(Stat->getName());
   if (!MaybeBuffer)
     return MaybeBuffer.getError();
 
   llvm::SmallString<1024> MinimizedFileContents;
   // Minimize the file down to directives that might affect the dependencies.
   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.
     CachedFileSystemEntry Result;
     Result.MaybeStat = std::move(*Stat);
     Result.Contents.reserve(Buffer->getBufferSize() + 1);
     Result.Contents.append(Buffer->getBufferStart(), Buffer->getBufferEnd());
     // Implicitly null terminate the contents for Clang's lexer.
if (!CacheEntry.isValid()) {
           return MaybeStatus.getError();
         else
           CacheEntry = CachedFileSystemEntry(MaybeStatus.getError());
       } else if (MaybeStatus->isDirectory())
         CacheEntry = CachedFileSystemEntry::createDirectoryEntry(
             std::move(*MaybeStatus));
       else
         CacheEntry = CachedFileSystemEntry::createFileEntry(
-            Filename, FS, !KeepOriginalSource);
+            Filename, FS, !KeepOriginalSource, PadMinimizedToOriginalSize);
     }
 
     Result = &CacheEntry;
   }
 
   // Store the result in the local cache.
   setCachedEntry(Filename, Result);
   return Result;

clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

if (DepFS) {
       for (const auto &Entry : CI.getHeaderSearchOpts().VFSOverlayFiles)
         DepFS->IgnoredFiles.insert(Entry);
 
       // Support for virtual file system overlays on top of the caching
       // filesystem.
       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)
         Compiler.getPreprocessorOpts()
             .ExcludedConditionalDirectiveSkipMappings = PPSkipMappings;
     }
 
     FileMgr->getFileSystemOpts().WorkingDir = std::string(WorkingDirectory);

clang/test/ClangScanDeps/modules-pch.c

 // RUN: rm -rf %t && mkdir %t
 // RUN: cp %S/Inputs/modules-pch/* %t
 
 // Scan dependencies of the PCH:
 //
 // 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.
 // RUN: not cat %/t/pch.h.gch
 //
 // CHECK-PCH:      -[[PREFIX:.*]]
 // CHECK-PCH-NEXT: {
 // CHECK-PCH-NEXT:   "modules": [

 // RUN: %clang @%t/mod_pch.cc1.rsp
 // RUN: %clang -x c-header %t/pch.h -fmodules -gmodules -fimplicit-module-maps \
 // RUN:   -fmodules-cache-path=%t/cache -o %t/pch.h.gch @%t/pch.rsp
 
 // Scan dependencies of the TU:
 //
 // 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:.*]]
 // CHECK-TU-NEXT: {
 // CHECK-TU-NEXT:   "modules": [
 // CHECK-TU-NEXT:     {
 // CHECK-TU-NEXT:       "clang-module-deps": [],
 // CHECK-TU-NEXT:       "clang-modulemap-file": "[[PREFIX]]/module.modulemap",

 // RUN: %clang -fsyntax-only %t/tu.c -fmodules -gmodules -fimplicit-module-maps \
 // RUN:   -fmodules-cache-path=%t/cache -include %t/pch.h -o %t/tu.o @%t/tu.rsp
 
 // Scan dependencies of the TU that has common modules with the PCH:
 //
 // 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:.*]]
 // CHECK-TU-WITH-COMMON-NEXT: {
 // CHECK-TU-WITH-COMMON-NEXT:   "modules": [
 // CHECK-TU-WITH-COMMON-NEXT:     {
 // CHECK-TU-WITH-COMMON-NEXT:       "clang-module-deps": [],
 // CHECK-TU-WITH-COMMON-NEXT:       "clang-modulemap-file": "[[PREFIX]]/module.modulemap",