Skip to content

[clang][modules] Fix filesystem races in ModuleManager#131354

Closed
jansvoboda11 wants to merge 1 commit intollvm:mainfrom
jansvoboda11:fix-module-manager-race
Closed

[clang][modules] Fix filesystem races in ModuleManager#131354
jansvoboda11 wants to merge 1 commit intollvm:mainfrom
jansvoboda11:fix-module-manager-race

Conversation

@jansvoboda11
Copy link
Contributor

@jansvoboda11 jansvoboda11 commented Mar 14, 2025

The ModuleManager uses FileEntry objects to uniquely identify module files. This requires first consulting the FileManager (and therefore the file system) when loading PCM files. This is problematic, as this might load a different PCM file to what's already in the InMemoryModuleCache and fail the size and mtime checks. (This is a bit artificial, but that could happen when using implicit modules without PCM signatures, with FileManager sharing disabled, and without a caching VFS.)

This PR changes things so that module files are identified by their file system path. This removes the need of knowing the file entry at the start and allows us to fix the race. The downside is that we no longer get the FileManager inode-based uniquing, meaning symlinks in the module cache no longer work. I think this is fine, since Clang itself never creates symlinks in that directory. Moreover, we already had to work around filesystems recycling inode numbers, so this actually seems like a win to me (and resolves a long-standing FIXME-like comment).

Note that this change also requires ModuleManager to use consistent paths to refer to module files. This might be problematic if there are concurrent Clang processes operating on the same module cache directory but with different spellings of its path - the IMPORT records in PCM files will be valid and consistent within single process, but may break uniquing in another process. We currently only do path-based canonicalization instead of more robust solution of something like real paths.

@hahnjo
Copy link
Member

hahnjo commented Mar 18, 2025

Thanks for the PR, testing for the use case described in #130795 it seems to fix the issue!

tl;dr I'm building LLVM+Clang+libcxx with

-DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS=clang -DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi;libunwind"

and then use the built clang with libc++ to configure LLVM with LLVM_ENABLE_MODULES=ON:

-DCMAKE_BUILD_TYPE=Release -DCMAKE_C_COMPILER=/path/to/clang -DCMAKE_CXX_COMPILER=/path/to/clang++ -DCMAKE_CXX_FLAGS="-Xclang -fno-implicit-modules-use-lock" -DLLVM_ENABLE_LIBCXX=ON -DLLVM_ENABLE_MODULES=ON

Using -Xclang -fno-implicit-modules-use-lock as described in #130795 (comment) instead of my manual changes still fails (almost) instantly with main, but passed two times the complete build with this PR 👏

@jansvoboda11 jansvoboda11 force-pushed the fix-module-manager-race branch from 7e8e883 to aec5c19 Compare March 19, 2025 17:05
@jansvoboda11 jansvoboda11 force-pushed the fix-module-manager-race branch 2 times, most recently from a3e3fd5 to 0054f8a Compare January 23, 2026 21:19
@github-actions
Copy link

github-actions bot commented Jan 23, 2026

✅ With the latest revision this PR passed the C/C++ code formatter.

@jansvoboda11 jansvoboda11 force-pushed the fix-module-manager-race branch from 0054f8a to 59263d8 Compare January 23, 2026 21:27
@jansvoboda11 jansvoboda11 force-pushed the fix-module-manager-race branch from 59263d8 to eb814bf Compare January 23, 2026 21:28
@jansvoboda11
Copy link
Contributor Author

Superseded by #185765 that doesn't rely on path normalization.

@jansvoboda11 jansvoboda11 deleted the fix-module-manager-race branch March 10, 2026 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants