Page MenuHomePhabricator

[VirtualFileSystem] Make the RedirectingFileSystem hold on to its own working directory.
ClosedPublic

Authored by JDevlieghere on Aug 2 2019, 2:29 PM.

Details

Summary

For reproducers it is useful to encode the notion of the current working directory in the VFS mapping. This enables us to handle relative files transparently, as long as the current working directory doesn't change over time. This change is motivated by a current bug in LLDB's reproducers. When a relative path to a binary is passed to the debugger, we fail to resolve it during reproducer replay, because the resolved (absolute) path doesn't match any entry in the VFS.

Diff Detail

Event Timeline

JDevlieghere created this revision.Aug 2 2019, 2:29 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 2 2019, 2:29 PM
sammccall accepted this revision.Aug 5 2019, 5:33 AM

It seems conceptually a little strange to have the working directory be part of a serialized "FS", as it's fundamentally a property of a process and only transiently a property of the VFS.
I don't know this is fatal (not sure what else the RedirectingFileSystem might be used for.

What's the reason to store this as part of the VFS rather than as a separate attribute of the larger reproducer structure?

This revision is now accepted and ready to land.Aug 5 2019, 5:33 AM
sammccall requested changes to this revision.Aug 5 2019, 5:34 AM

Oops, didn't mean to mark as accepted.

This revision now requires changes to proceed.Aug 5 2019, 5:34 AM

It seems conceptually a little strange to have the working directory be part of a serialized "FS", as it's fundamentally a property of a process and only transiently a property of the VFS.
I don't know this is fatal (not sure what else the RedirectingFileSystem might be used for.

What's the reason to store this as part of the VFS rather than as a separate attribute of the larger reproducer structure?

From the reproducer's perspective I would argue that it is a property of the VFS. More often than not, the working directory won't exist during replay. That needs to be modeled somewhere, and VFS is the obvious candidate. Indeed, the VFS already deals with the working directories today. You need special handling for this scenario though, because you cannot rely on the underlying (external) FS to do the right thing and might have side effects. At the same time, you might need to change the current working directory, which is fully supported with the current implementation as well. Also note that this functionality is completely optional. If no current working directory is set in the yaml mapping, things behave like they do today.

bruno added a comment.Aug 5 2019, 7:58 PM

It seems conceptually a little strange to have the working directory be part of a serialized "FS", as it's fundamentally a property of a process and only transiently a property of the VFS.

I tend to agree with @sammccall, it's odd that something that's usually transient to be considered an intrinsic part of the described filesystem. However, I also understand that the current way things are modeled in LLVM might not help with the big picture here. I'd like to know if you tried other approaches and why they failed. Would it be reasonable to instead have a property that list something resembling "preferred search paths" for the VFS in question? Seems more natural that the VFS could have the notion of where to look at things first (which can also be a list) instead of encoding a property that is usually transient.

From the reproducer's perspective I would argue that it is a property of the VFS. More often than not, the working directory won't exist during replay. That needs to be modeled somewhere, and VFS is the obvious candidate.

What happens when besides a current-working-directory in the YAML, the user also passes -working-directory to clang? What's the expected semantics? On the LLDB side, could the reproducer be passing something like clang does with -working-directory for the purpose of having extra ways to find things? I'd imagine such mechanism to prepend some path to relative searches, which would yield a virtual absolute path to lookup at the VFS?

labath added a subscriber: labath.Aug 5 2019, 11:12 PM

I just wanted to note that the CWD is not the only thing about a filesystem that's transient -- it's contents can change over time too. This is particularly important for a long-lived process like lldb, which explicitly supports things like rebuilding an executable and then relaunching it within the same debugger session, and it also creates and deletes some files itself. So, I expect that lldb reproducers will, sooner or later, need to learn to deal with filesystems which change over time, but AFAIK the current yaml vfs thingy does not support anything like that.

Now, I am not familiar with the design principles or the implementation of the VFS classes, so I have no idea whether this speaks for or against this change, but I though it may be interesting to note that..

I'd like to know if you tried other approaches and why they failed.

+1. In particular, what goes wrong if you try to make the working directory a sibling of VFS (within the reproducer container) rather than a child of it (within shared infrastructure)?

After some brainstorming I've identified a few other approaches that should better reflect the transience of the current working directory:

  • We can modify the VFS to have a notion of search paths. The adjustPath function could iterate over the search paths until it finds an absolute path that exists. If none exist we'd return the same path we return today. This would be the most general approach.
  • We could create a new virtual file system that we put on top of the RedirectingFileSystem which does something similar to what I've described above. This would require us to override every function that calls adjustPath, so it would be pretty heavyweight and rather inflexible.

I'd like to get your feedback before I start implementing these. What do you think? Is there another approach that's worth considering?

JDevlieghere retitled this revision from [VirtualFileSystem] Support encoding a current working directory in a VFS mapping. to [VirtualFileSystem] Support encoding working directories in a VFS mapping..

Support multiple working directories.

After some brainstorming I've identified a few other approaches that should better reflect the transience of the current working directory:

  • We can modify the VFS to have a notion of search paths. The adjustPath function could iterate over the search paths until it finds an absolute path that exists. If none exist we'd return the same path we return today. This would be the most general approach.
  • We could create a new virtual file system that we put on top of the RedirectingFileSystem which does something similar to what I've described above. This would require us to override every function that calls adjustPath, so it would be pretty heavyweight and rather inflexible.

    I'd like to get your feedback before I start implementing these. What do you think? Is there another approach that's worth considering?

I'm really sorry for missing this comment, I was out sick and missed the email.

I'd like to get your feedback before I start implementing these.

Honestly, this still seems way too complicated and this doesn't seem like a feature that needs to be part of VFS.

What do you think? Is there another approach that's worth considering?

Per my previous comment, what goes wrong if you try to make the working directory a sibling of VFS (within the reproducer container) rather than a child of it (within shared infrastructure)?

After some brainstorming I've identified a few other approaches that should better reflect the transience of the current working directory:

  • We can modify the VFS to have a notion of search paths. The adjustPath function could iterate over the search paths until it finds an absolute path that exists. If none exist we'd return the same path we return today. This would be the most general approach.
  • We could create a new virtual file system that we put on top of the RedirectingFileSystem which does something similar to what I've described above. This would require us to override every function that calls adjustPath, so it would be pretty heavyweight and rather inflexible.

    I'd like to get your feedback before I start implementing these. What do you think? Is there another approach that's worth considering?

I'm really sorry for missing this comment, I was out sick and missed the email.

I'd like to get your feedback before I start implementing these.

Honestly, this still seems way too complicated and this doesn't seem like a feature that needs to be part of VFS.

It's funny you say that, because the code to resolve relative paths is almost identical to the thing you added for supporting different working directories in different threads. :-)

What do you think? Is there another approach that's worth considering?

Per my previous comment, what goes wrong if you try to make the working directory a sibling of VFS (within the reproducer container) rather than a child of it (within shared infrastructure)?

Oops, seems like I didn't see your question either :-) Please clarify what you mean by sibling and child. Do you mean that the working directories are part of the mapping or that the redirecting file system knows about it? I don't care where we store the entries, I'm happy to have a separate YAML mapping that only the LLDB reproducers know about. However, I do need the underlying logic to be part of the (redirecting) VFS. Just like clang, LLDB is agnostic to the VFS, so this whole thing should be transparent. The only reason I didn't keep them separate is because then you need a way to tell the redirecting file system about the working directories, which means you need to get the concrete VFS, i.e. casting the VFS to a RedirectingVFS, which I try to avoid.

After some brainstorming I've identified a few other approaches that should better reflect the transience of the current working directory:

  • We can modify the VFS to have a notion of search paths. The adjustPath function could iterate over the search paths until it finds an absolute path that exists. If none exist we'd return the same path we return today. This would be the most general approach.
  • We could create a new virtual file system that we put on top of the RedirectingFileSystem which does something similar to what I've described above. This would require us to override every function that calls adjustPath, so it would be pretty heavyweight and rather inflexible.

    I'd like to get your feedback before I start implementing these. What do you think? Is there another approach that's worth considering?

I'm really sorry for missing this comment, I was out sick and missed the email.

I'd like to get your feedback before I start implementing these.

Honestly, this still seems way too complicated and this doesn't seem like a feature that needs to be part of VFS.

It's funny you say that, because the code to resolve relative paths is almost identical to the thing you added for supporting different working directories in different threads. :-)

Right! I think the key distinction is that there wasn't any functional change to the APIs, because the abstraction didn't change.
(There was a second factory function, which basically lets callers choose between the two implementations that have different sets of bugs)

What do you think? Is there another approach that's worth considering?

Per my previous comment, what goes wrong if you try to make the working directory a sibling of VFS (within the reproducer container) rather than a child of it (within shared infrastructure)?

Oops, seems like I didn't see your question either :-) Please clarify what you mean by sibling and child. Do you mean that the working directories are part of the mapping or that the redirecting file system knows about it? I don't care where we store the entries, I'm happy to have a separate YAML mapping that only the LLDB reproducers know about. However, I do need the underlying logic to be part of the (redirecting) VFS. Just like clang, LLDB is agnostic to the VFS, so this whole thing should be transparent. The only reason I didn't keep them separate is because then you need a way to tell the redirecting file system about the working directories, which means you need to get the concrete VFS, i.e. casting the VFS to a RedirectingVFS, which I try to avoid.

I mean why can't you just call setCurrentWorkingDirectory before starting? something like this:

struct Reproducer { string FSYaml; string CWD; ... };
void run(Reproducer &R) {
  auto* FS = RedirectingFileSystem::create(R.FSYaml, ...);
  FS->setCurrentWorkingDirectory(R.CWD);
  runLLDBWith(FS, ...);
}

It's funny you say that, because the code to resolve relative paths is almost identical to the thing you added for supporting different working directories in different threads. :-)

Right! I think the key distinction is that there wasn't any functional change to the APIs, because the abstraction didn't change.
(There was a second factory function, which basically lets callers choose between the two implementations that have different sets of bugs)

What do you think? Is there another approach that's worth considering?

Per my previous comment, what goes wrong if you try to make the working directory a sibling of VFS (within the reproducer container) rather than a child of it (within shared infrastructure)?

Oops, seems like I didn't see your question either :-) Please clarify what you mean by sibling and child. Do you mean that the working directories are part of the mapping or that the redirecting file system knows about it? I don't care where we store the entries, I'm happy to have a separate YAML mapping that only the LLDB reproducers know about. However, I do need the underlying logic to be part of the (redirecting) VFS. Just like clang, LLDB is agnostic to the VFS, so this whole thing should be transparent. The only reason I didn't keep them separate is because then you need a way to tell the redirecting file system about the working directories, which means you need to get the concrete VFS, i.e. casting the VFS to a RedirectingVFS, which I try to avoid.

I mean why can't you just call setCurrentWorkingDirectory before starting? something like this:

struct Reproducer { string FSYaml; string CWD; ... };
void run(Reproducer &R) {
  auto* FS = RedirectingFileSystem::create(R.FSYaml, ...);
  FS->setCurrentWorkingDirectory(R.CWD);
  runLLDBWith(FS, ...);
}

That doesn't work for the reproducer because the working directory likely doesn't exist during replay. The redirecting file system just forwards the setCurrentWorkingDirectory call to it's underlying (real) FS, so this becomes a no-op.

(I replied by email, it seems phabricator doesn't pick it up.)

It's funny you say that, because the code to resolve relative paths is almost identical to the thing you added for supporting different working directories in different threads. :-)

Right! I think the key distinction is that there wasn't any functional change to the APIs, because the abstraction didn't change.
(There was a second factory function, which basically lets callers choose between the two implementations that have different sets of bugs)

What do you think? Is there another approach that's worth considering?

Per my previous comment, what goes wrong if you try to make the working directory a sibling of VFS (within the reproducer container) rather than a child of it (within shared infrastructure)?

Oops, seems like I didn't see your question either :-) Please clarify what you mean by sibling and child. Do you mean that the working directories are part of the mapping or that the redirecting file system knows about it? I don't care where we store the entries, I'm happy to have a separate YAML mapping that only the LLDB reproducers know about. However, I do need the underlying logic to be part of the (redirecting) VFS. Just like clang, LLDB is agnostic to the VFS, so this whole thing should be transparent. The only reason I didn't keep them separate is because then you need a way to tell the redirecting file system about the working directories, which means you need to get the concrete VFS, i.e. casting the VFS to a RedirectingVFS, which I try to avoid.

I mean why can't you just call setCurrentWorkingDirectory before starting? something like this:

struct Reproducer { string FSYaml; string CWD; ... };
void run(Reproducer &R) {
  auto* FS = RedirectingFileSystem::create(R.FSYaml, ...);
  FS->setCurrentWorkingDirectory(R.CWD);
  runLLDBWith(FS, ...);
}

That doesn't work for the reproducer because the working directory likely doesn't exist during replay. The redirecting file system just forwards the setCurrentWorkingDirectory call to it's underlying (real) FS, so this becomes a no-op.

Can we fix that instead? The RedirectingVFS already knows about virtual directory entries, being able to cd to them makes sense.

This seems mostly like fixing a bug/limitation that wouldn't involve API changes.

Is there a part of the problem that needs the new search path concept/multiple paths?

JDevlieghere retitled this revision from [VirtualFileSystem] Support encoding working directories in a VFS mapping. to [VirtualFileSystem] Make the RedirectingFileSystem hold on to its own working directory..
JDevlieghere edited the summary of this revision. (Show Details)

Make the RedirectingFileSystem hold on to its own working directory.

Though I am not very familiar VFS, this seems like the most intuitive solution out of everything that we had so far. But... shouldn't you also check that the directory you're chdir-ing into "exists" before you actually change the cwd? Also, the chdir operation should probably follow the usual semantics of a non-absolute chdir path being treated as relative to the previous cwd. And lastly :), what is the initial cwd value? Since previously the class shared the cwd with the underlying filesystem, one option might be to fetch the initial cwd from there (though I don't know if that is actually desired). Another option might be to just pick the first directory in the yaml file or something...

Thanks for the feedback, Pavel.

Adding Michael Spencer as he was doing some work for a working directory in VFS.

labath added a comment.Sep 6 2019, 5:34 AM

Thanks Jonas. This looks fine to me, but someone who actually works on VFS (/me looks at @sammccall) will need to ack that.

sammccall added inline comments.
llvm/lib/Support/VirtualFileSystem.cpp
1052

This is a change in behavior, in that the WD of the underlying filesystem is no longer set, so e.g. in overlay mode RedirectingFileSystem::status() will look for non-overlaid relative paths in the wrong directory.

e.g. if we're using an overlay VFS with no files mapped and external WD is /foo, cd /bar; stat baz reads /bar/baz before this patch and reads /foo/baz after it. The old behavior seems more logical.

I think the right thing to do is to have setCurrentWorkingDirectory() call ExternalFS->setCurrentWorkingDirectory() if overlay mode is on. It shouldn't propagate failure, but rather record it bool ExternalFSValidWD, and only allow fallthrough operations when this flag is true.
This means cd into a dir which is invalid in the underlying FS would put it into a "broken" state where it's no longer consulted, but an (absolute) cd to a valid directory would fix it. (Variations are possible, depending on how you want to handle relative cd when the CWD is invalid in the underlying FS).

If you would rather just change the behavior here, you probably need to talk to someone familiar with the existing clients. It seems mostly used by clang's -ivfsoverlay, which was added by @benlangmuir way back when. I don't really know what it's used for.

BTW, This approach does look much better to me, it fits well into the VFS abstraction and I'd argue is fixing a bug in RedirectingFileSystem.
We do need to be careful about introducing new bugs, though. (or avoid the generality and just implement the parts LLDB needs, as a separate vfs::FS)

Implement Sam's suggestion for changing the external file system's working directory.

Mostly LG, just a couple of possible logic bugs.

Apologies, I was out on vacation and hoped someone else would see this.

llvm/include/llvm/Support/VirtualFileSystem.h
650

this name seems less than ideal:

  • it's very similar to IsFallthrough but has different semantics
  • "fallthrough" itself is not a very clear description of this functionality IMO
  • it's spelled wrong per the style guide

I'd suggest shouldUseExternalFS() or so

llvm/lib/Support/VirtualFileSystem.cpp
1056

this seems backwards - error_code converts to true if it's an *error*

add tests?

1060

this seems like it should go at the top? cd to a nonexistent directory should avoid changing state and return an error (which means not marking ExternalFSValidWD as false, I think)

1064

makeAbsolute already does this check

JDevlieghere marked 4 inline comments as done.
bruno accepted this revision.Mon, Oct 14, 10:08 AM

This approach looks overall much better! Unless @sammccall has any extra comments, it LGTM.

sammccall accepted this revision.Tue, Oct 15, 9:01 AM
This revision is now accepted and ready to land.Tue, Oct 15, 9:01 AM
This revision was automatically updated to reflect the committed changes.