[Lldb-commits] [PATCH] D55827: Update current working directory to avoid test crashes

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D55827#1335207 <https://reviews.llvm.org/D55827#1335207>, @JDevlieghere wrote:

> In D55827#1334671 <https://reviews.llvm.org/D55827#1334671>, @labath wrote:
>
> > Hm... this seems like an important issue in the `RealFileSystem` (or our usage of it), and I'm not sure it should be papered over like that. I mean, lldb is a library, and requiring every call to `chdir` in the whole process go through "our" implementation is not very friendly towards everyone else we happen to be sharing a process with.
>
>
> On the other hand (playing the devil's advocate) it could be considered nice that lldb as a library isn't affected by that if we explicitly set the cwd?


Yes, I can see how that could be nice, particularly if we could make the CWD local to each debugger instance so that they are truly independent. However, I think that should be a conscious decision for us to make (and discuss), and I don't think we were aware of this quirk (I certainly wasn't) of the VFS switch. If we go down this road, we should prepare a coherent story we will tell our users, as this can have some surprising side-effects. e.g., if the user uses relative paths in a python script (pretty printer, breakpoint stop-hook, etc.), those paths can end up having a different meaning once they make it into lldb C++ code because their notions of CWD may differ.

> 
> 
>> It sounds like we want a VFS that is slightly more "real" than the RealFileSystem, and which shares the notion of the CWD with the OS.
> 
> Did you have a look at the comment in `VirtualFileSystem.cpp` that explains why they went this route?
> 
>   // FIXME: chdir is thread hostile; on the other hand, creating the same
>   // behavior as chdir is complex: chdir resolves the path once, thus
>   // guaranteeing that all subsequent relative path operations work
>   // on the same path the original chdir resulted in. This makes a
>   // difference for example on network filesystems, where symlinks might be
>   // switched during runtime of the tool. Fixing this depends on having a
>   // file system abstraction that allows openat() style interactions.
>    
> 
> Wouldn't we encounter the same problem?

I don't think this comment matters for us. The way I read this, is that it is written from the POV of someone who wanted to make the CWD completely local to a `RealFileSystem` instance, and then complained that this is hard due to reasons outlined above (which it certainly is).

This is exactly the opposite of what I am proposing to do. :) That is, to make the RealFileSystem use the "real" CWD, whatever it happens to be at the moment of the call (the same behavior as the real os syscalls). It sounds like this behavior could be achieved by just deleting the CWDCache variable and having `getCurrentWorkingDirectory` always return the actual CWD. This may run contrary to what the original design goals of VFS were, but I would argue that this at least makes the RealFileSystem class self-consistent. Right now it has this weird behavior that all relative paths are still treated as relative to the "real" CWD, but then when someone asks to retrieve the CWD, it just returns some random cached value (which means that we don't even get the CWD isolation that we were speaking about in the first paragraph).


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55827/new/

https://reviews.llvm.org/D55827