[lit] Split test_set_working_dir TestProcessLaunch into two tests and fix it on Windows

[stella.stamenova]

Summary

test_set_working_dir was testing two scenario: failure to set the working dir because of a non existent directory and succeeding to set the working directory. Since the negative case fails on both Linux and Windows, the positive case was never tested. I split the test into two which allows us to always run both the negative and positive cases. The positive case now succeeds on Linux and the negative case still fails.
During the investigation, it turned out that lldbtest.py will try to execute a process launch command up to 3 times if the command failed. This means that we could be covering up intermittent failures by running any test that does process launch multiple times without ever realizing it. I've changed the counter to 1 (though it can still be overwritten with the environment variable).
This change also fixes both the positive and negative cases on Windows. There were a few issues:

1. In ProcessLauncherWindows::LaunchProcess, the error was not retrieved until CloseHandle was possibly called. Since CloseHandle is also a system API, its success would overwrite any existing error that could be retrieved using GetLastError. So by the time the error was retrieved, it was now a success.
2. In DebuggerThread::StopDebugging TerminateProcess was called on the process handle regardless of whether it was a valid handle. This was causing the process to crash when the handle was LLDB_INVALID_PROCESS (0xFFFFFFFF).
3. In ProcessWindows::DoLaunch we need to check that the working directory exists before launching the process to have the same behavior as other platforms which first check the directory and then launch process. This way we also control the exact error string.

[zturner]

Interestingly, I've been working on LLVM's low level process launching APIs to make it so we can hopefully replace some of this code with that. I don't think it's quite there yet, but hopefully we can converge soon. I'll keep this test case in mind.

[labath]

The change looks good. The only potentially risky thing is the maxLaunchCount change, but the fact that it is overridable by an env var should mitigate that. It looks like a very weird hack that seems to have been here since 2010. I hope we can remove it altogether at some point, and the requirement for configurations (if any) which require it to explicitly request that behavior seems like good step towards that.

source/Plugins/Process/Windows/Common/ProcessWindows.cpp

237–246

Checking the directory this way makes the launching subject to race conditions (cwd removed between this check and the actual launch). It's not very important because the launch will presumably still fail, just probably with a less helpful error message. However, if there is a way to detect this scenario from the CreateProcess error code, it would be better to do it that way.

packages/Python/lldbsuite/test/functionalities/process_launch/TestProcessLaunch.py

def test_io(self):
         if not success:
             self.fail(err_msg)
 
     # rdar://problem/9056462
     # The process launch flag '-w' for setting the current working directory
     # not working?
     @not_remote_testsuite_ready
     @expectedFailureAll(oslist=["linux"], bugnumber="llvm.org/pr20265")
-    def test_set_working_dir(self):
+    def test_set_working_dir_nonexisting(self):
+        """Test that '-w dir' fails to set the working dir when running the inferior with a dir which doesn't exist."""
+        d = {'CXX_SOURCES': 'print_cwd.cpp'}
+        self.build(dictionary=d)
+        self.setTearDownCleanup(d)
+        exe = self.getBuildArtifact("a.out")
+        self.runCmd("file " + exe)
+
+        mywd = 'my_working_dir'
+        out_file_name = "my_working_dir_test.out"
+        err_file_name = "my_working_dir_test.err"
+
+        my_working_dir_path = self.getBuildArtifact(mywd)
+        out_file_path = os.path.join(my_working_dir_path, out_file_name)
+        err_file_path = os.path.join(my_working_dir_path, err_file_name)
+
+        # Check that we get an error when we have a nonexisting path
+        invalid_dir_path = mywd + 'z'
+        launch_command = "process launch -w %s -o %s -e %s" % (
+            invalid_dir_path, out_file_path, err_file_path)
+
+        self.expect(
+            launch_command, error=True, patterns=[
+                "error:.* No such file or directory: %s" %
+                invalid_dir_path])
+
+    @not_remote_testsuite_ready
+    def test_set_working_dir_existing(self):
         """Test that '-w dir' sets the working dir when running the inferior."""
         d = {'CXX_SOURCES': 'print_cwd.cpp'}
         self.build(dictionary=d)
         self.setTearDownCleanup(d)
         exe = self.getBuildArtifact("a.out")
         self.runCmd("file " + exe)
 
         mywd = 'my_working_dir'
         out_file_name = "my_working_dir_test.out"
         err_file_name = "my_working_dir_test.err"
 
         my_working_dir_path = self.getBuildArtifact(mywd)
         lldbutil.mkdir_p(my_working_dir_path)
         out_file_path = os.path.join(my_working_dir_path, out_file_name)
         err_file_path = os.path.join(my_working_dir_path, err_file_name)
 
         # Make sure the output files do not exist before launching the process
         try:
             os.remove(out_file_path)
             os.remove(err_file_path)
         except OSError:
             pass
 
-        # Check that we get an error when we have a nonexisting path
-        launch_command = "process launch -w %s -o %s -e %s" % (
-            my_working_dir_path + 'z', out_file_path, err_file_path)
-
-        self.expect(
-            launch_command, error=True, patterns=[
-                "error:.* No such file or directory: %sz" %
-                my_working_dir_path])
-
-        # Really launch the process
         launch_command = "process launch -w %s -o %s -e %s" % (
             my_working_dir_path, out_file_path, err_file_path)
 
         self.expect(launch_command,
                     patterns=["Process .* launched: .*a.out"])
 
         success = True
         err_msg = ""

packages/Python/lldbsuite/test/lldbtest.py

class TestBase(Base):
     """
 
     # Subclasses can set this to true (if they don't depend on debug info) to avoid running the
     # test multiple times with various debug info types.
     NO_DEBUG_INFO_TESTCASE = False
 
     # Maximum allowed attempts when launching the inferior process.
     # Can be overridden by the LLDB_MAX_LAUNCH_COUNT environment variable.
-    maxLaunchCount = 3
+    maxLaunchCount = 1
 
     # Time to wait before the next launching attempt in second(s).
     # Can be overridden by the LLDB_TIME_WAIT_NEXT_LAUNCH environment variable.
     timeWaitNextLaunch = 1.0
 
     def generateSource(self, source):
         template = source + '.template'
         temp = os.path.join(self.getSourceDir(), template)

source/Host/windows/ProcessLauncherWindows.cpp

llvm::ConvertUTF8toWide(launch_info.GetWorkingDirectory().GetCString(),
                           wworkingDirectory);
 
   wcommandLine.resize(PATH_MAX); // Needs to be over-allocated because
                                  // CreateProcessW can modify it
   BOOL result = ::CreateProcessW(
       wexecutable.c_str(), &wcommandLine[0], NULL, NULL, TRUE, flags, env_block,
       wworkingDirectory.size() == 0 ? NULL : wworkingDirectory.c_str(),
       &startupinfo, &pi);
+
+  if (!result) {
+    // Call GetLastError before we make any other system calls.
+    error.SetError(::GetLastError(), eErrorTypeWin32);
+  }
+
   if (result) {
     // Do not call CloseHandle on pi.hProcess, since we want to pass that back
     // through the HostProcess.
     ::CloseHandle(pi.hThread);
   }
 
   if (stdin_handle)
     ::CloseHandle(stdin_handle);
   if (stdout_handle)
     ::CloseHandle(stdout_handle);
   if (stderr_handle)
     ::CloseHandle(stderr_handle);
 
   if (!result)
-    error.SetError(::GetLastError(), eErrorTypeWin32);
+    return HostProcess();
+
   return HostProcess(pi.hProcess);
 }
 
 HANDLE
 ProcessLauncherWindows::GetStdioHandle(const ProcessLaunchInfo &launch_info,
                                        int fd) {
   const FileAction *action = launch_info.GetFileActionForFD(fd);
   if (action == nullptr)

source/Plugins/Process/Windows/Common/DebuggerThread.cpp

Status DebuggerThread::StopDebugging(bool terminate) {
 
   // Make a copy of the process, since the termination sequence will reset
   // DebuggerThread's internal copy and it needs to remain open for the Wait
   // operation.
   HostProcess process_copy = m_process;
   lldb::process_t handle = m_process.GetNativeProcess().GetSystemHandle();
 
   if (terminate) {
+    if (handle != nullptr && handle != LLDB_INVALID_PROCESS) {
       // Initiate the termination before continuing the exception, so that the
       // next debug event we get is the exit process event, and not some other
       // event.
       BOOL terminate_suceeded = TerminateProcess(handle, 0);
       LLDB_LOG(log,
                "calling TerminateProcess({0}, 0) (inferior={1}), success={2}",
                handle, pid, terminate_suceeded);
+    } else {
+      LLDB_LOG(log,
+               "NOT calling TerminateProcess because the inferior is not valid ({0}, 0) (inferior={1})",
+               handle, pid);
+    }
   }
 
   // If we're stuck waiting for an exception to continue (e.g. the user is at a
   // breakpoint messing around in the debugger), continue it now.  But only
   // AFTER calling TerminateProcess to make sure that the very next call to
   // WaitForDebugEvent is an exit process event.
   if (m_active_exception.get()) {
     LLDB_LOG(log, "masking active exception");

source/Plugins/Process/Windows/Common/ProcessWindows.cpp

 Status ProcessWindows::DoLaunch(Module *exe_module,
                                 ProcessLaunchInfo &launch_info) {
   // Even though m_session_data is accessed here, it is before a debugger
   // thread has been kicked off.  So there's no race conditions, and it
   // shouldn't be necessary to acquire the mutex.
 
   Log *log = ProcessWindowsLog::GetLogIfAny(WINDOWS_LOG_PROCESS);
   Status result;
+
+  FileSpec working_dir = launch_info.GetWorkingDirectory();
+  namespace fs = llvm::sys::fs;
+  if (working_dir && (!working_dir.ResolvePath() ||
+                      !fs::is_directory(working_dir.GetPath()))) {
+    result.SetErrorStringWithFormat("No such file or directory: %s",
+                                   working_dir.GetCString());
+    return result;
+  }
+
   if (!launch_info.GetFlags().Test(eLaunchFlagDebug)) {
     StreamString stream;
     stream.Printf("ProcessWindows unable to launch '%s'.  ProcessWindows can "
                   "only be used for debug launches.",
                   launch_info.GetExecutableFile().GetPath().c_str());
     std::string message = stream.GetString();
     result.SetErrorString(message.c_str());