Page MenuHomePhabricator

[lldb] Fix fallthrough with strictly virtual working directory.
AbandonedPublic

Authored by JDevlieghere on Jan 15 2021, 10:56 AM.

Details

Summary

LLDB needs to change the virtual current working directory without it
breaking fallthrough. Most reproducers will contain a virtual working
directory that does not exist in the real file system during replay.

Currently, calling setCurrentWorkingDirectory on RedirectingFileSystem
will attempt to change the working directory for the external FS and if
that fails, it will set ExternalFSValidWD to false which prevents
fallthrough.

Diff Detail

Event Timeline

JDevlieghere created this revision.Jan 15 2021, 10:56 AM
JDevlieghere requested review of this revision.Jan 15 2021, 10:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2021, 10:56 AM

I'm wondering about this:

Currently, calling setCurrentWorkingDirectory on RedirectingFileSystem
will attempt to change the working directory for the external FS and if
that fails, it will set ExternalFSValidWD to false which prevents
fallthrough.

I'm worried having inconsistent working directories could be worse than blocking fallthrough. Consider:

# physical
/a/b/c/d.c
/c/README

# virtual
/x/README

What if someone does setWorkingDirectory() for /a/b/c/d.c followed by /x, and then looks up the relative path d.c? Or changes directory to ../c?

I wonder if there's a way to split up shouldUseExternalFS() (or refactor other code) to allow the external FS to be partially used without adding this kind of inconsistency.

  • Absolute paths should still work just fine.
  • Relative paths can be made absolute by the virtual FS first (if the WD isn't valid), then passed through.
llvm/include/llvm/Support/VirtualFileSystem.h
764

I think this could use a comment to explain its behaviour since it's not obvious.

llvm/lib/Support/VirtualFileSystem.cpp
1079

This comment looks out-of-date.

I missed a few words:

What if someone does setWorkingDirectory() for /a/b/c/d.c followed by

"changing directory to"

/x, and then looks up the relative path d.c? Or changes directory to ../c?

JDevlieghere added a comment.EditedJan 15 2021, 3:19 PM

Personally I'd just like to get rid of ExternalFSValidWD inside of shouldUseExternalFS and let the underlying FS take care of it, which I believe would better fit the abstraction but would be a change in behavior (as pointed out in https://reviews.llvm.org/D65677#inline-604694).

Personally I'd just like to get rid of ExternalFSValidWD inside of shouldUseExternalFS and let the underlying FS take care of it, which I believe would better fit the abstraction but would be a change in behavior (as pointed out in https://reviews.llvm.org/D65677#inline-604694).

I'm not quite following why that would work; when the underlying FS fails to cd, then subsequent calls could fall through to the wrong place.

I just checked the code in OverlayFileSystem and it doesn't seem much better about this:

std::error_code
OverlayFileSystem::setCurrentWorkingDirectory(const Twine &Path) {
  for (auto &FS : FSList)
    if (std::error_code EC = FS->setCurrentWorkingDirectory(Path))
      return EC;
  return {};
}

That'll leave things horribly inconsistent as well.

The abstraction as I see it is that the caller sees the OverlayFileSystem / RedirectingFileSystem as the *only* filesystem, a single thing that can be cd'ed around in. A cd to any directory that exists in that virtual view should work as if that were true. Subsequent calls to setWorkingDirectory() or getFile() should behave as if that were the case.

I think one way to get that, at least for your use case, is to extract out RealFileSystem's handling for a custom working directory and reuse it.

  • getRealFileSystem() still returns a RealFileSystem, but that class gets stripped down.
  • WorkingDirectoryFileSystem<BaseT> gets that support, which can be layered on top of anything.
  • getPhysicalFileSystem() returns a WorkingDirectoryFileSystem<RealFileSystem>.
  • LLDB uses a WorkingDirectoryFileSystem<ProxyFileSystem> on top of the last VFS overlay.

Probably clang could take advantage of this abstraction as well, but that seems outside the scope of what you're trying to do.

WDYT?

JDevlieghere abandoned this revision.Jan 22 2021, 2:17 PM

Superseded by D95188