Page MenuHomePhabricator

[VFS] RedirectingFileSystem only replace path if not already mapped
ClosedPublic

Authored by bnbarham on Mar 27 2022, 2:25 PM.

Details

Summary

If the ExternalFS has already remapped a path then the
RedirectingFileSystem should not change it to the originally provided
path. This fixes the original path always being used if multiple VFS
overlays were provided and the path wasn't found in the highest (ie.
first in the chain).

This also renames IsVFSMapped to ExposesExternalVFSPath and only
sets it if UseExternalName is true. This flag then represents that the
Status has an external path that's different from its virtual path.
Right now the contained path is still the external path, but further PRs
will change this to *always* be the virtual path. Clients that need the
external can then request it specifically.

Note that even though ExposesExternalVFSPath isn't set for all
VFS-mapped paths, IsVFSMapped was only being used by a hack in
FileManager that was specific to module searching. In that case
UseExternalNames is always true and so that hack still applies.

Resolves rdar://90578880 and llvm-project#53306.

Diff Detail

Event Timeline

bnbarham created this revision.Mar 27 2022, 2:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2022, 2:25 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
bnbarham requested review of this revision.Mar 27 2022, 2:25 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 27 2022, 2:25 PM

This looks nice!

clang/lib/Basic/FileManager.cpp
290–295

I'd suggest updating this text to match the new FIXME next to IsVFSMapped (instead of "maybe", list the plan).

317–319

It's not obvious to me that the redirection case above depends on this logic sticking around.

  • If that's what you mean, can you explain in more detail why the redirection above depends on this UFE.Dir logic?
  • If you're just talking about IsVFSMapped having to stick around, I think that part should be covered by the comment for the redirection.
llvm/include/llvm/Support/VirtualFileSystem.h
57–71

I think "mapped to another path" has a broader definition in my mind than you're using here. IMO, all things in a redirecting filesystem map from one path to another. The question is whether the mapping is leaked/available due to use-external-names.

I think this comment be more precise, and the note should be a FIXME that's part of the main comment, not something that comes later.

I also think it good to change the field name ~now (or in an immediately following NFC patch) to reflect the new meaning. I like "exposes" or "leaks" because that is clear that this indicates whether information about the mapping is available / exposed in the abstraction, whereas "mapped" and "mapping" sound to me like they're just talking about whether something was redirected. And I like the word "external" because it ties back to use-external-names.

Suggested text (feel free to change entirely, but I think it covers a couple of important points):

/// Whether this entity has an external path different from the virtual path, and
/// the external path is exposed by leaking it through the abstraction. For example,
/// a RedirectingFileSystem will set this for paths where UseExternalName is true.
///
/// FIXME: Currently the external path is exposed by replacing the virtual path
/// in this Status object. Instead, we should leave the path in the Status intact
/// (matching the requested virtual path), and just use this flag as hint that a
/// new API, FileSystem::getExternalVFSPath(), will not return `None`. Clients can
/// then request the external path only where needed (e.g., for diagnostics, or to
/// report discovered dependencies to external tools).
bool ExposesExternalVFSPath = false;

clang-apply-replacements/relative-paths.cpp is failing, I haven't looked into it but my guess would be that it's from the Status.getName() == Filename -> !Status.IsVFSMapped change. That seems very odd to me.

clang/lib/Basic/FileManager.cpp
290–295

Will do

317–319

I actually meant the reverse - the UFE.Dir logic being removed depends on the redirection logic above being removed. That's because for this to be removed, anywhere that cares about the requested path needs to be changed to use Refs. But even if they change to use Refs, the redirection logic above would itself replace the path with the external one. So it needs to be removed as well.

llvm/include/llvm/Support/VirtualFileSystem.h
57–71

That's fair. I suppose I was thinking that without use-external-names: true then nothing should be aware that it is "mapped" and thus "mapped" == "external". But I can see how that could be confusing.

I also thought that renaming now would be a little odd since eg. "HasExternalPath" seems a little different to "the actual path is currently the external path" (which is what it is right now). But with the "exposes" naming I think this could still make sense (especially with the FIXME you have). So I'll rename 👍

clang-apply-replacements/relative-paths.cpp is failing, I haven't looked into it but my guess would be that it's from the Status.getName() == Filename -> !Status.IsVFSMapped change. That seems very odd to me.

Is it just failing on Windows? I wonder (rather speculatively...) whether https://reviews.llvm.org/D121733 would help.

clang/lib/Basic/FileManager.cpp
317–319

Okay, I suggest just making the link between them more clear.

clang-apply-replacements/relative-paths.cpp is failing, I haven't looked into it but my guess would be that it's from the Status.getName() == Filename -> !Status.IsVFSMapped change. That seems very odd to me.

Is it just failing on Windows? I wonder (rather speculatively...) whether https://reviews.llvm.org/D121733 would help.

No, also debian. Not sure why it isn't saying debian failed in the review.

dexonsmith added inline comments.Mar 28 2022, 11:31 AM
clang/lib/Basic/FileManager.cpp
277

An incremental patch you could try would be:

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:

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.

bnbarham updated this revision to Diff 418721.Mar 28 2022, 4:05 PM
bnbarham edited the summary of this revision. (Show Details)

Rename IsVFSMapped as suggested by Duncan. Update comments.

bnbarham marked 4 inline comments as done.Mar 28 2022, 4:15 PM
bnbarham added inline comments.
clang/lib/Basic/FileManager.cpp
277

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.

bnbarham marked an inline comment as not done.Mar 28 2022, 4:15 PM
bnbarham added inline comments.Mar 28 2022, 6:25 PM
clang/lib/Basic/FileManager.cpp
277

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.

dexonsmith added inline comments.Mar 28 2022, 6:51 PM
clang/lib/Basic/FileManager.cpp
277

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.

bnbarham marked an inline comment as not done.Mar 29 2022, 12:30 PM
bnbarham added inline comments.
clang/lib/Basic/FileManager.cpp
277

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.

bnbarham updated this revision to Diff 418959.Mar 29 2022, 12:42 PM

Keep using the redirection hack for the time being

This revision is now accepted and ready to land.Mar 30 2022, 11:14 AM
This revision was landed with ongoing or failed builds.Mar 30 2022, 11:56 AM
This revision was automatically updated to reflect the committed changes.