[clang][deps] Fix race condition in implicit Clang module builds

[naveen-seth]

Fixes #194475 (comment).

The assert was thrown in the following scenario:

Say threads 1 and 2 both try to compile the same module and both concurrently call findOrCompileModuleAndReadAST() and both have a cache miss.
Both will then go and call compileModuleBehindLockOrRead().
If thread 1 completes the module compilation, locking and unlocking before thread 2 even tries to lock, thread 2 will also compile the module a second time.

This adds a check after acquiring the lock to prevent such situations.

Since the original bug occurred randomly, this fix was tested by running the failing test ~25 times while deleting the module cache after each run.
(Before the fix, the bug was reproduced within a few runs of the same).

[llvmbot]

@llvm/pr-subscribers-clang

Author: Naveen Seth Hanig (naveen-seth)

Changes

Fixes #194475 (comment).

This assert was thrown because of the following scenario:

Say threads 1 and 2 both try to compile the same module and both concurrently call findOrCompileModuleAndReadAST() and both have a cache miss.
Both will then go and call compileModuleBehindLockOrRead().
If thread 1 completes the module compilation, locking and unlocking before thread 2 even tries to lock, thread 2 will also compile the module a second time.

This adds a check after acquiring the lock to prevent such situations.

Since the original bug occurred randomly, this fix was tested by running the failing test ~20 times while deleting the module cache after each run.

---

Full diff: https://github.com/llvm/llvm-project/pull/194757.diff

1 Files Affected:

• (modified) clang/lib/Frontend/CompilerInstance.cpp (+11)

diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index f868e1e449325..690527713c651 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -1570,6 +1570,17 @@ compileModuleBehindLockOrRead(CompilerInstance &ImportingInstance,
       return CompileOrReadResult::Compiled;
     }
     if (Owned) {
+      // If another thread finished building the module before we acquired the
+      // lock, but after our initial cache miss, try to read it from the cache
+      // instead of rebuilding.
+      bool OutOfDate = false;
+      bool Missing = false;
+      if (readASTAfterCompileModule(ImportingInstance, ImportLoc, ModuleNameLoc,
+                                    Module, ModuleFileName, &OutOfDate,
+                                    &Missing))
+        return CompileOrReadResult::Read;
+      if (!OutOfDate && !Missing)
+        return CompileOrReadResult::FailedToRead;
       // We're responsible for building the module ourselves.
       if (!compileModuleImpl(ImportingInstance, ImportLoc, ModuleNameLoc,
                              Module, ModuleFileName))

[jansvoboda11]

I think this will slow down scanning. The race is rare, but this check will be performed for every module compile. At least in the scanner, two racing threads should produce the same module file, so I'd rather investigate the discrepancy in the produced PCMs (unless the only bug present is the one I fix in #194888).

[naveen-seth]

If I understand correctly, we’d remove the assert that motivated this PR and only detect any discrepancies from duplicate‑built PCMs? (This seems good to me too.)

Otherwise, could we do a cheaper check by storing the build state in ModuleCacheEntry?
(After locking, we'd then check the state).

[Bigcheese]

We really should detect cases where the PCM differs. Even if we rebuild the module in every scan the result should be the same, so if there's still a problem after #194888 we should look at why they are different.