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