[PATCH] D122549: [VFS] RedirectingFileSystem only replace path if not already mapped

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

dexonsmith added inline comments.


================
Comment at: clang/lib/Basic/FileManager.cpp:273
 
-  if (Status.getName() == Filename) {
+  if (!Status.IsVFSMapped) {
     // The name matches. Set the FileEntry.
----------------
bnbarham wrote:
> bnbarham wrote:
> > dexonsmith wrote:
> > > An incremental patch you could try would be:
> > > ```
> > > lang=c++
> > > if (Status.getName() == Filename || !Status.IsVFSMapped)
> > > ```
> > > ... dropping all the other changes.
> > > 
> > > This limits the redirection hack to only apply when a RedirectingFS is involved (leaving until later the fine-tuning of when `IsVFSMapped` gets set). If this smaller change still causes a test failure, it might be easier to reason about why / how to fix it / be sure that the fix is sound.
> > > 
> > > Eventually we might want something like:
> > > ```
> > > lang=c++
> > > if (!Status.IsVFSMapped) {
> > >   assert(Status.getName() == Filename);
> > > ```
> > > I imagine that's not going to succeed yet due to the CWD behaviour in the VFSes, but as a speculative patch it might help track down whatever the problem is.
> > That assertion currently won't succeed for any relative path, since `getStatValue` does the absolute pathing for relative paths. So `Status.getName()` is guaranteed to be different to `Filename` in that case.
> > 
> > Possibly even more odd is that the failing test passes locally on MacOSX. I'll try the suggestion above and see if they fail again.
> Actually, thinking about this a bit more - the failing test doesn't use an overlay at all, so changing to the above wouldn't help at all.
The test was added in 8188484daa4195a2c8b5253765036fa2c6da7263 / https://reviews.llvm.org/D112647, with a change to do:
```
+    auto PrevWorkingDir = SM.getFileManager().getFileSystemOpts().WorkingDir;
+    if (BuildDir)
+      SM.getFileManager().getFileSystemOpts().WorkingDir = std::move(*BuildDir);
     if (auto Entry = SM.getFileManager().getFile(R.getFilePath())) {
       if (SourceTU) {
         auto &Replaces = DiagReplacements[*Entry];
@@ -170,18 +175,19 @@ groupReplacements(const TUReplacements &TUs, const TUDiagnostics &TUDs,
       errs() << "Described file '" << R.getFilePath()
              << "' doesn't exist. Ignoring...\n";
     }
+    SM.getFileManager().getFileSystemOpts().WorkingDir = PrevWorkingDir;
```

This looks incredibly suspicious. Changing the working directory mid-stream on the FileManager isn't sound. Consider:
```
/dir1/file1.h
/dir2/file1.h
/dir2/file2.h
```
if you do:
```
FM.WorkingDir = "/dir1";
FM.getFile("file1.h"); // returns /dir1/file1.h
FM.WorkingDir = "/dir2";
FM.getFile("file1.h"); // returns /dir1/file1.h !!!
FM.getFile("file2.h"); // returns /dir2/file2.h
```

That doesn't explain why it's not reproducing locally for you, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122549