[Lldb-commits] [lldb] r334642 - [lit] Split test_set_working_dir TestProcessLaunch into two tests and fix it on Windows
Stella Stamenova via lldb-commits
lldb-commits at lists.llvm.org
Tue Jun 19 10:19:05 PDT 2018
Hey Greg,
I would be hesitant to make any big changes at the moment since it's my understanding that Zachary is working on a change that would impact all platforms (specifically for process launching).
That said, I've been looking at some of the other failing tests on Windows and I can compare notes with the implementation in ds2 to see if some of their windows changes can help with the issues that LLDB has. By the way, I did notice going through the ds2 sources that they have some TODOs in the windows code for some of the same features that are currently missing or not working correctly in LLDB (such as Wow64, floating point registers, etc.), so their implementation is not complete either.
Thank you,
-Stella
-----Original Message-----
From: Greg Clayton <clayborg at gmail.com>
Sent: Wednesday, June 13, 2018 1:10 PM
To: Stella Stamenova <stilis at microsoft.com>
Cc: lldb-commits at lists.llvm.org
Subject: Re: [Lldb-commits] [lldb] r334642 - [lit] Split test_set_working_dir TestProcessLaunch into two tests and fix it on Windows
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://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ffacebook%2Fds2&data=02%7C01%7Cstilis%40microsoft.com%7C6ac25ad6ce7941c2c79608d5d169abc1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C1%7C636645174145470302&sdata=HcSScsDUTELddsNkvDA4cdAqimL1gn9D3KmH9B6mAng%3D&reserved=0
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 at lists.llvm.org> wrote:
>
> Author: stella.stamenova
> Date: Wed Jun 13 12:02:44 2018
> New Revision: 334642
>
> URL:
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fllvm.o
> rg%2Fviewvc%2Fllvm-project%3Frev%3D334642%26view%3Drev&data=02%7C01%7C
> stilis%40microsoft.com%7C6ac25ad6ce7941c2c79608d5d169abc1%7C72f988bf86
> f141af91ab2d7cd011db47%7C1%7C1%7C636645174145470302&sdata=UtY53Z8TR6kp
> g6sPmO9vVqoM%2F%2BVNXI7QYHjBg9AspYg%3D&reserved=0
> 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://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Frevie
> ws.llvm.org%2FD48050&data=02%7C01%7Cstilis%40microsoft.com%7C6ac25ad6c
> e7941c2c79608d5d169abc1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C1%7C6
> 36645174145470302&sdata=5PJhjuRZ%2BKpipw0vuSaW5pEm2C3jZ688QhXYJeqKIOQ%
> 3D&reserved=0
>
> 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_laun
> ch/TestProcessLaunch.py
> URL:
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fllvm.o
> rg%2Fviewvc%2Fllvm-project%2Flldb%2Ftrunk%2Fpackages%2FPython%2Flldbsu
> ite%2Ftest%2Ffunctionalities%2Fprocess_launch%2FTestProcessLaunch.py%3
> Frev%3D334642%26r1%3D334641%26r2%3D334642%26view%3Ddiff&data=02%7C01%7
> Cstilis%40microsoft.com%7C6ac25ad6ce7941c2c79608d5d169abc1%7C72f988bf8
> 6f141af91ab2d7cd011db47%7C1%7C1%7C636645174145470302&sdata=u0Cn37YI1z%
> 2FIm9rcS6vxbgk%2FH7hhEl7r5QkuDMqkJ%2BY%3D&reserved=0
> ======================================================================
> ========
> ---
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/process_laun
> ch/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:
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fllvm.o
> rg%2Fviewvc%2Fllvm-project%2Flldb%2Ftrunk%2Fpackages%2FPython%2Flldbsu
> ite%2Ftest%2Flldbtest.py%3Frev%3D334642%26r1%3D334641%26r2%3D334642%26
> view%3Ddiff&data=02%7C01%7Cstilis%40microsoft.com%7C6ac25ad6ce7941c2c7
> 9608d5d169abc1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C1%7C6366451741
> 45470302&sdata=0AozZLJOiwufwlyEdaqBO0jhWVqEZV%2BrSNPVuaRd4ZQ%3D&reserv
> ed=0
> ======================================================================
> ========
> --- 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:
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fllvm.o
> rg%2Fviewvc%2Fllvm-project%2Flldb%2Ftrunk%2Fsource%2FHost%2Fwindows%2F
> ProcessLauncherWindows.cpp%3Frev%3D334642%26r1%3D334641%26r2%3D334642%
> 26view%3Ddiff&data=02%7C01%7Cstilis%40microsoft.com%7C6ac25ad6ce7941c2
> c79608d5d169abc1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C1%7C63664517
> 4145470302&sdata=bvyacoO59MeCLiHH8prSxr5UyO7A0Uk9FlWJukWmM74%3D&reserv
> ed=0
> ======================================================================
> ========
> --- 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:
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fllvm.o
> rg%2Fviewvc%2Fllvm-project%2Flldb%2Ftrunk%2Fsource%2FPlugins%2FProcess
> %2FWindows%2FCommon%2FDebuggerThread.cpp%3Frev%3D334642%26r1%3D334641%
> 26r2%3D334642%26view%3Ddiff&data=02%7C01%7Cstilis%40microsoft.com%7C6a
> c25ad6ce7941c2c79608d5d169abc1%7C72f988bf86f141af91ab2d7cd011db47%7C1%
> 7C1%7C636645174145470302&sdata=1OasJmFud0FM%2FesfIEVyi8d5WoXN0ZRKz%2Fj
> I9Kc5Isk%3D&reserved=0
> ======================================================================
> ========
> ---
> lldb/trunk/source/Plugins/Process/Windows/Common/DebuggerThread.cpp
> (original)
> +++ lldb/trunk/source/Plugins/Process/Windows/Common/DebuggerThread.cp
> +++ p 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:
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fllvm.o
> rg%2Fviewvc%2Fllvm-project%2Flldb%2Ftrunk%2Fsource%2FPlugins%2FProcess
> %2FWindows%2FCommon%2FProcessWindows.cpp%3Frev%3D334642%26r1%3D334641%
> 26r2%3D334642%26view%3Ddiff&data=02%7C01%7Cstilis%40microsoft.com%7C6a
> c25ad6ce7941c2c79608d5d169abc1%7C72f988bf86f141af91ab2d7cd011db47%7C1%
> 7C1%7C636645174145470302&sdata=2PUmZJ6Q9MIpvpMe5D2K%2FBWFqRh3h5%2BZHl2
> Hq8dUKrQ%3D&reserved=0
> ======================================================================
> ========
> ---
> lldb/trunk/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
> (original)
> +++ lldb/trunk/source/Plugins/Process/Windows/Common/ProcessWindows.cp
> +++ p 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 at lists.llvm.org
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.
> llvm.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Flldb-commits&data=02%7C01%7C
> stilis%40microsoft.com%7C6ac25ad6ce7941c2c79608d5d169abc1%7C72f988bf86
> f141af91ab2d7cd011db47%7C1%7C1%7C636645174145470302&sdata=fpV0WA2baOvb
> zp2aYfbLuKWwAQAMXxkooia5i4yZ2sM%3D&reserved=0
More information about the lldb-commits
mailing list