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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
