I like seeing the windows debugger plug-in getting fixes. LLDB currently encourages everyone to add native debugging support to lldb-server. Then you get remote debugging support for free. It seems like you are putting in some time on the windows fixes, so I thought I would pass this along. The ProcessGDBRemote is the most tested protocol in LLDB currently so I am guessing things might be easier in the long run if you go that route.
FYI: ds2 is an open source GDB server that actually already supports windows: https://github.com/facebook/ds2 It is a separate GDB remote server that was not built using any LLDB sources. It would be interesting to possibly port the windows changes over into LLDB's lldb-server. Let me know what you think, Greg Clayton > On Jun 13, 2018, at 12:02 PM, Stella Stamenova via lldb-commits > <lldb-commits@lists.llvm.org> wrote: > > Author: stella.stamenova > Date: Wed Jun 13 12:02:44 2018 > New Revision: 334642 > > URL: http://llvm.org/viewvc/llvm-project?rev=334642&view=rev > Log: > [lit] Split test_set_working_dir TestProcessLaunch into two tests and fix it > on Windows > > 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. > > Reviewers: labath, zturner, asmith, jingham > > Reviewed By: labath > > Subscribers: llvm-commits > > Differential Revision: https://reviews.llvm.org/D48050 > > Modified: > > lldb/trunk/packages/Python/lldbsuite/test/functionalities/process_launch/TestProcessLaunch.py > lldb/trunk/packages/Python/lldbsuite/test/lldbtest.py > lldb/trunk/source/Host/windows/ProcessLauncherWindows.cpp > lldb/trunk/source/Plugins/Process/Windows/Common/DebuggerThread.cpp > lldb/trunk/source/Plugins/Process/Windows/Common/ProcessWindows.cpp > > Modified: > lldb/trunk/packages/Python/lldbsuite/test/functionalities/process_launch/TestProcessLaunch.py > URL: > http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/process_launch/TestProcessLaunch.py?rev=334642&r1=334641&r2=334642&view=diff > ============================================================================== > --- > lldb/trunk/packages/Python/lldbsuite/test/functionalities/process_launch/TestProcessLaunch.py > (original) > +++ > lldb/trunk/packages/Python/lldbsuite/test/functionalities/process_launch/TestProcessLaunch.py > Wed Jun 13 12:02:44 2018 > @@ -85,7 +85,34 @@ class ProcessLaunchTestCase(TestBase): > # 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) > @@ -109,16 +136,6 @@ class ProcessLaunchTestCase(TestBase): > 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) > > > Modified: lldb/trunk/packages/Python/lldbsuite/test/lldbtest.py > URL: > http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/lldbtest.py?rev=334642&r1=334641&r2=334642&view=diff > ============================================================================== > --- lldb/trunk/packages/Python/lldbsuite/test/lldbtest.py (original) > +++ lldb/trunk/packages/Python/lldbsuite/test/lldbtest.py Wed Jun 13 12:02:44 > 2018 > @@ -1833,7 +1833,7 @@ class TestBase(Base): > > # 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. > > Modified: lldb/trunk/source/Host/windows/ProcessLauncherWindows.cpp > URL: > http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/windows/ProcessLauncherWindows.cpp?rev=334642&r1=334641&r2=334642&view=diff > ============================================================================== > --- lldb/trunk/source/Host/windows/ProcessLauncherWindows.cpp (original) > +++ lldb/trunk/source/Host/windows/ProcessLauncherWindows.cpp Wed Jun 13 > 12:02:44 2018 > @@ -96,6 +96,12 @@ ProcessLauncherWindows::LaunchProcess(co > 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. > @@ -110,7 +116,8 @@ ProcessLauncherWindows::LaunchProcess(co > ::CloseHandle(stderr_handle); > > if (!result) > - error.SetError(::GetLastError(), eErrorTypeWin32); > + return HostProcess(); > + > return HostProcess(pi.hProcess); > } > > > Modified: lldb/trunk/source/Plugins/Process/Windows/Common/DebuggerThread.cpp > URL: > http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/Windows/Common/DebuggerThread.cpp?rev=334642&r1=334641&r2=334642&view=diff > ============================================================================== > --- lldb/trunk/source/Plugins/Process/Windows/Common/DebuggerThread.cpp > (original) > +++ lldb/trunk/source/Plugins/Process/Windows/Common/DebuggerThread.cpp Wed > Jun 13 12:02:44 2018 > @@ -181,13 +181,19 @@ Status DebuggerThread::StopDebugging(boo > lldb::process_t handle = m_process.GetNativeProcess().GetSystemHandle(); > > if (terminate) { > - // 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); > + 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 > > Modified: lldb/trunk/source/Plugins/Process/Windows/Common/ProcessWindows.cpp > URL: > http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/Windows/Common/ProcessWindows.cpp?rev=334642&r1=334641&r2=334642&view=diff > ============================================================================== > --- lldb/trunk/source/Plugins/Process/Windows/Common/ProcessWindows.cpp > (original) > +++ lldb/trunk/source/Plugins/Process/Windows/Common/ProcessWindows.cpp Wed > Jun 13 12:02:44 2018 > @@ -234,6 +234,16 @@ Status ProcessWindows::DoLaunch(Module * > > 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 " > > > _______________________________________________ > lldb-commits mailing list > lldb-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
