[PATCH] D114966: [clang][deps] Split stat and file content caches

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.


================
Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:200-210
+          // FIXME: This read can fail.
+          // In that case, we should probably update `CacheEntry::MaybeStat`.
+          // However, that is concurrently read at the start of this function
+          // (`needsUpdate` -> `isReadable`), leading to a data race. If we try
+          // to avoid the data race by returning an `error_code` here (without
+          // updating the entry stat value, we can end up attempting to read the
+          // file again in the future, which breaks the consistency guarantees
----------------
I'm not quite following this logic. I think it's safe (and important!) to modify MaybeStat if `read()` fails.

We're in a critical section that either creates and partially initializes the entry, or incrementally updates it.

In the "creates and partially initializes" case:
- All other workers will get nullptr for `Cache.getEntry()` and try to enter the critical section.
- We have just seen a successful MaybeStat value.
- `needsRead()` will be `true` since we have not read contents before. We will immediately try to read.
- `read()` should open the original contents and can safely:
    - on success, update the value for MaybeStat to match the observed size
    - on failure, drop the value and set the error for MaybeStat to the observed error
- When we leave the critical section, either:
    - MaybeStat stores an error; no thread will enter the critical section again
    - OriginalContents are initialized and `needsRead()` returns false

In the "incrementally updates" case:
- `needsRead()` returns false so `read()` will not be called



================
Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:211
+          // `true` on the first read and prevent future reads of the same file.
+          Contents->read(CacheEntry.getName(), getUnderlyingFS());
+
----------------
Seems like the existing stat value should be passed into `read()` and the second stat there removed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114966/new/

https://reviews.llvm.org/D114966