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

[Ben Barham via Phabricator via cfe-commits]

bnbarham marked an inline comment as not done.
bnbarham added inline comments.


================
Comment at: clang/lib/Basic/FileManager.cpp:273
 
-  if (Status.getName() == Filename) {
+  if (!Status.IsVFSMapped) {
     // The name matches. Set the FileEntry.
----------------
dexonsmith wrote:
> 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.
I ended up building in a docker container and tracked it down.

The underlying problem is two fold:
1. What you've said (shouldn't change WorkingDir)
2. clang-apply-replacements stores the FileEntry and uses that to read the file later. Because relative paths are absolute pathed inside `getStatValue`, the path in the FileEntryRef remains relative (without the redirection hack).

The reason I'm not seeing this test fail on MacOS is down to the iteration order of files. On MacOS we process `file2.yaml` and then `file1.yaml`. On others it looks like we're processing `file1.yaml` and then `file2.yaml`.

If `file1.yaml` is processed last, its last lookup is an absolute path and thus everything works fine. But if `file2.yaml` is processed last, its last lookup is relative and then the file can't be found.

This would normally be fine since the relative path would be resolved using the working directory, but because that's continually changed... 💥 

Another quirk here is that if `getBufferForFile(FileEntry)` was used and `getFile` originally passed `openFile=true` then this would work, because it would be cached. And wouldn't work if `openFile=false` was passed, which is the default case.

TLDR: I don't think we should modify the redirection-hack condition in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122549