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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
