Skip to content

Exclude RedirectingFileSystem with null OverlayFileDir in VFSUsage #128267

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 25, 2025

Conversation

hjyamauchi
Copy link
Contributor

This is to avoid assertion failures like the following when RedirectingFileSystem's are created and used outside createVFSFromOverlayFiles.

Assertion failed: VFSUsage.size() == getHeaderSearchOpts().VFSOverlayFiles.size() && "A different number of RedirectingFileSystem's were present than " "-ivfsoverlay options passed to Clang!", file S:\SourceCache\llvm-project\clang\lib\Lex\HeaderSearch.cpp, line 162

@hjyamauchi
Copy link
Contributor Author

An assertion failure like the above was encountered in the swift toolchain in the context of this PR swiftlang/swift#78184 which uses a module map VFS overlay on top of the read-only Windows MSVC/ucrt include paths.

@@ -149,11 +149,16 @@ std::vector<bool> HeaderSearch::collectVFSUsageAndClear() const {

llvm::vfs::FileSystem &RootFS = FileMgr.getVirtualFileSystem();
// TODO: This only works if the `RedirectingFileSystem`s were all created by
Copy link
Contributor Author

@hjyamauchi hjyamauchi Feb 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This PR partially addresses the limitation described by this comment.

@hjyamauchi hjyamauchi marked this pull request as ready for review February 22, 2025 01:54
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Feb 22, 2025
@hjyamauchi hjyamauchi requested a review from compnerd February 22, 2025 01:54
@llvmbot
Copy link
Member

llvmbot commented Feb 22, 2025

@llvm/pr-subscribers-clang

Author: Hiroshi Yamauchi (hjyamauchi)

Changes

This is to avoid assertion failures like the following when RedirectingFileSystem's are created and used outside createVFSFromOverlayFiles.

Assertion failed: VFSUsage.size() == getHeaderSearchOpts().VFSOverlayFiles.size() &amp;&amp; "A different number of RedirectingFileSystem's were present than " "-ivfsoverlay options passed to Clang!", file S:\SourceCache\llvm-project\clang\lib\Lex\HeaderSearch.cpp, line 162

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

1 Files Affected:

  • (modified) clang/lib/Lex/HeaderSearch.cpp (+8-3)
diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp
index bf8fe44e4ca9c..f1e011627d499 100644
--- a/clang/lib/Lex/HeaderSearch.cpp
+++ b/clang/lib/Lex/HeaderSearch.cpp
@@ -149,11 +149,16 @@ std::vector<bool> HeaderSearch::collectVFSUsageAndClear() const {
 
   llvm::vfs::FileSystem &RootFS = FileMgr.getVirtualFileSystem();
   // TODO: This only works if the `RedirectingFileSystem`s were all created by
-  //       `createVFSFromOverlayFiles`.
+  //       `createVFSFromOverlayFiles`. But at least exclude the ones with null
+  //       OverlayFileDir.
   RootFS.visit([&](llvm::vfs::FileSystem &FS) {
     if (auto *RFS = dyn_cast<llvm::vfs::RedirectingFileSystem>(&FS)) {
-      VFSUsage.push_back(RFS->hasBeenUsed());
-      RFS->clearHasBeenUsed();
+      // Skip a `RedirectingFileSystem` with null OverlayFileDir which indicates
+      // that they aren't created by `createVFSFromOverlayFiles`.
+      if (!RFS->getOverlayFileDir().empty()) {
+        VFSUsage.push_back(RFS->hasBeenUsed());
+        RFS->clearHasBeenUsed();
+      }
     }
   });
   assert(VFSUsage.size() == getHeaderSearchOpts().VFSOverlayFiles.size() &&

@compnerd
Copy link
Member

CC: @Bigcheese

Copy link
Contributor

@Bigcheese Bigcheese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think this is fine, although it would be nice if there was a tighter connection between the -ivfsoverlay options and this. I don't think that needs to happen here though.

This is to avoid assertion failures like the following when
RedirectingFileSystem's are created and used outside
createVFSFromOverlayFiles.

```
Assertion failed: VFSUsage.size() == getHeaderSearchOpts().VFSOverlayFiles.size() && "A different number of RedirectingFileSystem's were present than " "-ivfsoverlay options passed to Clang!", file S:\SourceCache\llvm-project\clang\lib\Lex\HeaderSearch.cpp, line 162
```
@hjyamauchi hjyamauchi merged commit f58fde5 into llvm:main Feb 25, 2025
11 checks passed
hjyamauchi added a commit to hjyamauchi/llvm-project that referenced this pull request Feb 25, 2025
This is to avoid assertion failures like the following when
RedirectingFileSystem's are created and used outside
createVFSFromOverlayFiles.

```
Assertion failed: VFSUsage.size() == getHeaderSearchOpts().VFSOverlayFiles.size() && "A different number of RedirectingFileSystem's were present than " "-ivfsoverlay options passed to Clang!", file S:\SourceCache\llvm-project\clang\lib\Lex\HeaderSearch.cpp, line 162
```

Cherrypick commit llvm#128267
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants