Page MenuHomePhabricator

Update current working directory to avoid test crashes
AbandonedPublic

Authored by tatyana-krasnukha on Dec 18 2018, 8:05 AM.

Details

Summary

Running dotest.py with a path to tests directory results in:

terminate called after throwing an instance of 'std::logic_error'
              what():  basic_string::_M_construct null not valid
Aborted

for some tests. For example, when a test executes command "script import relative_path.py" the exception happens in ScriptInterpreterPython::LoadScriptingModule at the line

std::string directory = target_file.GetDirectory().GetCString();

because GetCString returns nullptr.

The reason of such behavior is that llvm::RealFileSystem returns cached current working directory value until it will be updated with setCurrentWorkingDirectory. So, we permanently have cwd == directory of the first test, FileSystem::Resolve doesn't find the file and just returns the original value.

P.S. What about FileSpec::GetDirectory and FileSpec::GetFileName those never return nullptr? There is a lot of code that doesn't check result on nullptr (look at llvm.org/pr37054, for example). I made a patch where these functions return empty strings instead of nullptr, but some tests fail as they expect the result is None. Other API users can be broken also.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

labath added a subscriber: labath.Dec 18 2018, 8:21 AM

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.

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.

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?

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?

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).

serge-sans-paille resigned from this revision.Jan 14 2019, 12:36 AM

I think this problem ought to be fixed now (since D56545).

tatyana-krasnukha abandoned this revision.Jan 14 2019, 8:23 AM

It is fixed.