[all-commits] [llvm/llvm-project] ced077: [clang][deps] NFC: Simplify handling of cached FS ...

[Jan Svoboda via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: ced077e1ba52ec2937aab538e9b6fa5149f8c567
      https://github.com/llvm/llvm-project/commit/ced077e1ba52ec2937aab538e9b6fa5149f8c567
  Author: Jan Svoboda <jan_svoboda at apple.com>
  Date:   2022-01-21 (Fri, 21 Jan 2022)

  Changed paths:
    M clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
    M clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

  Log Message:
  -----------
  [clang][deps] NFC: Simplify handling of cached FS errors

The return types of some `CachedFileSystemEntry` member function are needlessly complex.

This patch attempts to simplify the code by unwrapping cached entries that represent errors early, and then asserting `!isError()`.

Reviewed By: dexonsmith

Differential Revision: https://reviews.llvm.org/D115935


  Commit: 5daeada33051aa85777593d3f69eb29f26e7fb2f
      https://github.com/llvm/llvm-project/commit/5daeada33051aa85777593d3f69eb29f26e7fb2f
  Author: Jan Svoboda <jan_svoboda at apple.com>
  Date:   2022-01-21 (Fri, 21 Jan 2022)

  Changed paths:
    M clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
    M clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
    M clang/unittests/Tooling/DependencyScannerTest.cpp

  Log Message:
  -----------
  [clang][deps] Ensure filesystem cache consistency

The minimizing filesystem used by the dependency scanner isn't great when it comes to the consistency of its caches. There are two problems that can be exposed by a filesystem that changes during dependency scan:
1. In-memory cache entries for original and minimized files are distinct, populated at different times using separate stat/open syscalls. This means that when a file is read with minimization disabled, its contents might be inconsistent when the same file is read with minimization enabled at later point (and vice versa).
2. In-memory cache entries are indexed by filename. This is problematic for symlinks, where the contents of the symlink might be inconsistent with contents of the original file (for the same reason as in problem 1).

This patch ensures consistency by always stating/reading a file exactly once. The original contents are always cached and minimized contents are derived from that on demand. The cache entries are now indexed by their `UniqueID` ensuring consistency for symlinks too. Moreover, the stat/read syscalls are now issued outside of critical section.

Depends on D115935.

Reviewed By: dexonsmith

Differential Revision: https://reviews.llvm.org/D114966


  Commit: 8cc2a137270462bc191377dbab97c739583814dd
      https://github.com/llvm/llvm-project/commit/8cc2a137270462bc191377dbab97c739583814dd
  Author: Jan Svoboda <jan_svoboda at apple.com>
  Date:   2022-01-21 (Fri, 21 Jan 2022)

  Changed paths:
    M clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
    M clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
    A clang/test/ClangScanDeps/modules-symlink.c

  Log Message:
  -----------
  [clang][deps] Handle symlinks in minimizing FS

The minimizing and caching filesystem used by the dependency scanner can be configured to **not** minimize some files. That's necessary when scanning a TU with prebuilt inputs (i.e. PCH) that refer to the original (non-minimized) files. Minimizing such files in the dependency scanner would cause discrepancy between the current perceived state of the filesystem and the file sizes stored in the AST file. By not minimizing such files, we avoid creating the discrepancy.

The problem with the current approach is that files that should not be minimized are identified by their path. This breaks down when the prebuilt input (PCH) and the current TU refer to the same file via different paths (i.e. symlinks). This patch switches from paths to `llvm::sys::fs::UniqueID` when identifying ignored files. This is consistent with how the rest of Clang treats files.

Depends on D114966.

Reviewed By: dexonsmith, arphaman

Differential Revision: https://reviews.llvm.org/D114971


Compare: https://github.com/llvm/llvm-project/compare/4727d29d908f...8cc2a1372704