Update current working directory to avoid test crashes

[tatyana-krasnukha]

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.

[labath]

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.

[JDevlieghere]

In 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?

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?

[labath]

In D55827#1335207, @JDevlieghere wrote:

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

[labath]

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

[tatyana-krasnukha]

It is fixed.

include/lldb/API/SBHostOS.h

public:
   static lldb::SBFileSpec GetProgramFileSpec();
 
   static lldb::SBFileSpec GetLLDBPythonPath();
 
   static lldb::SBFileSpec GetLLDBPath(lldb::PathType path_type);
 
   static lldb::SBFileSpec GetUserHomeDirectory();
 
+  static bool SetCurrentWorkingDirectory(const char *cwd);
+
   static void ThreadCreated(const char *name);
 
   static lldb::thread_t ThreadCreate(const char *name,
                                      lldb::thread_func_t thread_function,
                                      void *thread_arg, lldb::SBError *err);
 
   static bool ThreadCancel(lldb::thread_t thread, lldb::SBError *err);
 

include/lldb/Host/FileSystem.h

public:
   /// @}
 
   /// Resolve path to make it canonical.
   /// @{
   void Resolve(llvm::SmallVectorImpl<char> &path);
   void Resolve(FileSpec &file_spec);
   /// @}
 
+  /// Set current working directory.
+  /// @{
+  bool SetCurrentWorkingDirectory(const llvm::Twine &cwd);
+  /// @}
+
   //// Create memory buffer from path.
   /// @{
   std::shared_ptr<DataBufferLLVM> CreateDataBuffer(const llvm::Twine &path,
                                                    uint64_t size = 0,
                                                    uint64_t offset = 0);
   std::shared_ptr<DataBufferLLVM> CreateDataBuffer(const FileSpec &file_spec,
                                                    uint64_t size = 0,
                                                    uint64_t offset = 0);

packages/Python/lldbsuite/test/lldbtest.py

def setUpClass(cls):
         # See also dotest.py which sets up ${LLDB_TEST}.
         if ("LLDB_TEST" in os.environ):
             full_dir = os.path.join(os.environ["LLDB_TEST"],
                                     cls.mydir)
             if traceAlways:
                 print("Change dir to:", full_dir, file=sys.stderr)
             os.chdir(full_dir)
 
+        if not lldb.SBHostOS.SetCurrentWorkingDirectory(full_dir):
+            raise Exception("Failed to re-write cached cwd value.")
+
         # Set platform context.
         cls.platformContext = lldbplatformutil.createPlatformContext()
 
     @classmethod
     def tearDownClass(cls):
         """
         Python unittest framework class teardown fixture.
         Do class-wide cleanup.

scripts/interface/SBHostOS.i

public:
     static lldb::SBFileSpec
     GetLLDBPythonPath ();
     
     static lldb::SBFileSpec
     GetLLDBPath (lldb::PathType path_type);
 
     static lldb::SBFileSpec
     GetUserHomeDirectory ();
         
+    static bool
+    SetCurrentWorkingDirectory(const char *cwd);
+
     static void
     ThreadCreated (const char *name);
 
     static lldb::thread_t
     ThreadCreate (const char *name,
                   lldb::thread_func_t,
                   void *thread_arg,
                   lldb::SBError *err);

source/API/SBHostOS.cpp

SBFileSpec SBHostOS::GetUserHomeDirectory() {
   llvm::sys::path::home_directory(home_dir_path);
   FileSpec homedir(home_dir_path.c_str());
   FileSystem::Instance().Resolve(homedir);
 
   sb_fspec.SetFileSpec(homedir);
   return sb_fspec;
 }
 
+bool SBHostOS::SetCurrentWorkingDirectory(const char *cwd) {
+  return cwd ? FileSystem::Instance().SetCurrentWorkingDirectory(cwd) : false;
+}
+
 lldb::thread_t SBHostOS::ThreadCreate(const char *name,
                                       lldb::thread_func_t thread_function,
                                       void *thread_arg, SBError *error_ptr) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
 
   if (log)
     log->Printf(
         "SBHostOS::ThreadCreate (name=\"%s\", thread_function=%p, "

source/Host/common/FileSystem.cpp

void FileSystem::Resolve(FileSpec &file_spec) {
   // Resolve the path.
   Resolve(path);
 
   // Update the FileSpec with the resolved path.
   file_spec.SetPath(path);
   file_spec.SetIsResolved(true);
 }
 
+bool FileSystem::SetCurrentWorkingDirectory(const llvm::Twine &cwd) {
+  std::error_code error = m_fs->setCurrentWorkingDirectory(cwd);
+  return !error;
+}
+
 std::shared_ptr<DataBufferLLVM>
 FileSystem::CreateDataBuffer(const llvm::Twine &path, uint64_t size,
                              uint64_t offset) {
   const bool is_volatile = !IsLocal(path);
 
   std::unique_ptr<llvm::WritableMemoryBuffer> buffer;
   if (size == 0) {
     auto buffer_or_error =