[lldb-server] Report launch error in vRun packets

[labath]

Summary

Uses our existing "error string" extension to provide a better
indication of why the launch failed (the client does not make use of the
error yet).

Also, fix the way we obtain the launch error message (make sure we read
the whole message, and skip trailing garbage), and reduce the size of
TestLldbGdbServer by splitting some tests into a separate file.

[labath]

Add windows error msg

[mgorny]

lldb/test/API/tools/lldb-server/TestGdbRemoteLaunch.py

19

Since we no longer support Python 2, I'd rather prefer to work towards removing seven rather than making more use of it.

[labath]

lldb/test/API/tools/lldb-server/TestGdbRemoteLaunch.py

19

Is the problem with the name/location of the function or the functionality (string/byte conversion) itself?
Because, if it's the first, then that could easily be solved by renaming the module (now or later), but in order to avoid elaborate casts we'd have to make all code be byte/string correct. This is not a problem here (because of the fixed strings), but it becomes one once you start working with things that aren't necessarily valid utf8 strings.

[mgorny]

lldb/test/API/tools/lldb-server/TestGdbRemoteLaunch.py

19

Ok, I suppose this makes sense given how LLDB's Python API is screwed up :-(.

[rupprecht]

lldb/source/Host/posix/ProcessLauncherPosixFork.cpp

293

IIUC, this will overrun the buffer if there are >1000 bytes to read; whereas previously we just wouldn't have read everything.

Should each loop iteration grow the buffer by a certain amount? Otherwise I think we need to remove the loop.

[labath]

dynamically resize error buffer

[labath]

lldb/source/Host/posix/ProcessLauncherPosixFork.cpp

293

It won't overrun because I am passing the remaining part of the buffer as the size argument. However, I am not totally sure what would happen if we actually filled the buffer (and size becomes zero). That won't happen right now because the error string is coming from ExitWithError (line 50) and its length is limited by the longest errno message. That said, the dynamical resize is quite simple in this case, so I might as well do it.

lldb/test/API/tools/lldb-server/TestGdbRemoteLaunch.py

19

Just to clarify, I would like if we used the byte/string separation more (and I started that when I refactored the gdb server test harness), but it's fairly tricky because we're often working with things (e.g. memory contents, or stdout) that can represent arbitrary data in general, but which usually (for our own convenience and sanity) contain simple ASCII strings. So we have a problem where the API (and here I just mean the gdbserver test API, which we can change) really wants us to use bytes, but the the most natural way to write the test is with strings. I haven't come up with a solution to that yet.

[labath]

Use pos instead of the previous size in the resize expression.

[mgorny]

lldb/test/API/tools/lldb-server/TestGdbRemoteLaunch.py

19

Yeah, I know. On the plus side, as long as we're dealing with plain ASCII, using bytes is relatively easy and most of the string-like functionality just works. You just have to remember that you want to use byte-strings ;-).

[rupprecht]

lldb/source/Host/posix/ProcessLauncherPosixFork.cpp

293

Thanks! I still think it's good to be dynamic in case this assumption changes one day, and the error string ends up being longer than 1000 bytes.

lldb/source/Host/posix/ProcessLauncherPosixFork.cpp

if (pid == 0) {
     // child process
     pipe.CloseReadFileDescriptor();
     ChildFunc(pipe.ReleaseWriteFileDescriptor(), fork_launch_info);
   }
 
   // parent process
 
   pipe.CloseWriteFileDescriptor();
-  char buf[1000];
-  int r = read(pipe.GetReadFileDescriptor(), buf, sizeof buf);
+  llvm::SmallString<0> buf;
+  size_t pos = 0;
+  ssize_t r = 0;
+  do {
+    pos += r;
+    buf.resize_for_overwrite(pos + 100);
+    r = llvm::sys::RetryAfterSignal(-1, read, pipe.GetReadFileDescriptor(),
+                                    buf.begin() + pos, buf.size() - pos);
+  } while (r > 0);
+  assert(r != -1);
 
-  if (r == 0)
+  buf.resize(pos);
+  if (buf.empty())
     return HostProcess(pid); // No error. We're done.
 
   error.SetErrorString(buf);
 
   llvm::sys::RetryAfterSignal(-1, waitpid, pid, nullptr, 0);
 
   return HostProcess();
 }

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

for (llvm::StringRef hex_arg : argv) {
     StringExtractor arg_ext{hex_arg};
     std::string arg;
     arg_ext.GetHexByteString(arg);
     m_process_launch_info.GetArguments().AppendArgument(arg);
     LLDB_LOGF(log, "LLGSPacketHandler::%s added arg: \"%s\"", __FUNCTION__,
               arg.c_str());
   }
 
-  if (!argv.empty()) {
-    m_process_launch_info.GetExecutableFile().SetFile(
-        m_process_launch_info.GetArguments()[0].ref(), FileSpec::Style::native);
-    m_process_launch_error = LaunchProcess();
-    if (m_process_launch_error.Success()) {
-      assert(m_current_process);
-      return SendStopReasonForState(*m_current_process,
-                                    m_current_process->GetState(),
-                                    /*force_synchronous=*/true);
-    }
-    LLDB_LOG(log, "failed to launch exe: {0}", m_process_launch_error);
-  }
-  return SendErrorResponse(8);
-}
+  if (argv.empty())
+    return SendErrorResponse(Status("No arguments"));
+  if (m_process_launch_error.Fail())
+    return SendErrorResponse(m_process_launch_error);
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::Handle_D(StringExtractorGDBRemote &packet) {
   Log *log = GetLog(LLDBLog::Process);
   if (!m_non_stop)
     StopSTDIOForwarding();
 
   lldb::pid_t pid = LLDB_INVALID_PROCESS_ID;

lldb/test/API/tools/lldb-server/TestGdbRemoteLaunch.py

+import itertools
+
+import gdbremote_testcase
+import lldbgdbserverutils
+from lldbsuite.support import seven
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+
+
+class GdbRemoteLaunchTestCase(gdbremote_testcase.GdbRemoteTestCaseBase):
+
+    @skipIfWindows # No pty support to test any inferior output
+    @add_test_categories(["llgs"])
+    def test_launch_via_A(self):
+        self.build()
+        exe_path = self.getBuildArtifact("a.out")
+        args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
+        hex_args = [seven.hexlify(x) for x in args]
+
+        server = self.connect_to_debug_monitor()
+        self.assertIsNotNone(server)
+        self.do_handshake()
+        # NB: strictly speaking we should use %x here but this packet
+        # is deprecated, so no point in changing lldb-server's expectations
+        self.test_sequence.add_log_lines(
+            ["read packet: $A %d,0,%s,%d,1,%s,%d,2,%s,%d,3,%s#00" %
+             tuple(itertools.chain.from_iterable(
+                 [(len(x), x) for x in hex_args])),
+             "send packet: $OK#00",
+             "read packet: $c#00",
+             "send packet: $W00#00"],
+            True)
+        context = self.expect_gdbremote_sequence()
+        self.assertEqual(context["O_content"],
+                         b'arg1\r\narg2\r\narg3\r\n')
+
+    @skipIfWindows # No pty support to test any inferior output
+    @add_test_categories(["llgs"])
+    def test_launch_via_vRun(self):
+        self.build()
+        exe_path = self.getBuildArtifact("a.out")
+        args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
+        hex_args = [seven.hexlify(x) for x in args]
+
+        server = self.connect_to_debug_monitor()
+        self.assertIsNotNone(server)
+        self.do_handshake()
+        self.test_sequence.add_log_lines(
+            ["read packet: $vRun;%s;%s;%s;%s#00" % tuple(hex_args),
+             {"direction": "send",
+              "regex": r"^\$T([0-9a-fA-F]+)"},
+             "read packet: $c#00",
+             "send packet: $W00#00"],
+            True)
+        context = self.expect_gdbremote_sequence()
+        self.assertEqual(context["O_content"],
+                         b'arg1\r\narg2\r\narg3\r\n')
+
+    @add_test_categories(["llgs"])
+    def test_launch_via_vRun_no_args(self):
+        self.build()
+        exe_path = self.getBuildArtifact("a.out")
+        hex_path = seven.hexlify(exe_path)
+
+        server = self.connect_to_debug_monitor()
+        self.assertIsNotNone(server)
+        self.do_handshake()
+        self.test_sequence.add_log_lines(
+            ["read packet: $vRun;%s#00" % (hex_path,),
+             {"direction": "send",
+              "regex": r"^\$T([0-9a-fA-F]+)"},
+             "read packet: $c#00",
+             "send packet: $W00#00"],
+            True)
+        self.expect_gdbremote_sequence()
+
+    @add_test_categories(["llgs"])
+    def test_launch_failure_via_vRun(self):
+        self.build()
+        exe_path = self.getBuildArtifact("a.out")
+        hex_path = seven.hexlify(exe_path)
+
+        server = self.connect_to_debug_monitor()
+        self.assertIsNotNone(server)
+        self.do_handshake()
+        self.test_sequence.add_log_lines(
+            ["read packet: $QEnableErrorStrings#00",
+             "send packet: $OK#00",
+             "read packet: $vRun;%s#00" % hex_path,
+             {"direction": "send", "regex": r"^\$E..;([0-9a-fA-F]+)#",
+                 "capture": {1: "msg"},
+             }],
+            True)
+        with open(exe_path, "ab") as exe_for_writing:
+            context = self.expect_gdbremote_sequence()
+        self.assertRegexpMatches(seven.unhexlify(context.get("msg")),
+                r"(execve failed: Text file busy|The process cannot access the file because it is being used by another process.)")
+
+    @skipIfWindows # No pty support to test any inferior output
+    @add_test_categories(["llgs"])
+    def test_QEnvironment(self):
+        self.build()
+        exe_path = self.getBuildArtifact("a.out")
+        env = {"FOO": "test", "BAR": "a=z"}
+        args = [exe_path, "print-env:FOO", "print-env:BAR"]
+        hex_args = [seven.hexlify(x) for x in args]
+
+        server = self.connect_to_debug_monitor()
+        self.assertIsNotNone(server)
+        self.do_handshake()
+
+        for key, value in env.items():
+            self.test_sequence.add_log_lines(
+                ["read packet: $QEnvironment:%s=%s#00" % (key, value),
+                 "send packet: $OK#00"],
+                True)
+        self.test_sequence.add_log_lines(
+            ["read packet: $vRun;%s#00" % (";".join(hex_args),),
+             {"direction": "send",
+              "regex": r"^\$T([0-9a-fA-F]+)"},
+             "read packet: $c#00",
+             "send packet: $W00#00"],
+            True)
+        context = self.expect_gdbremote_sequence()
+        self.assertEqual(context["O_content"], b"test\r\na=z\r\n")
+
+    @skipIfWindows # No pty support to test any inferior output
+    @add_test_categories(["llgs"])
+    def test_QEnvironmentHexEncoded(self):
+        self.build()
+        exe_path = self.getBuildArtifact("a.out")
+        env = {"FOO": "test", "BAR": "a=z", "BAZ": "a*}#z"}
+        args = [exe_path, "print-env:FOO", "print-env:BAR", "print-env:BAZ"]
+        hex_args = [seven.hexlify(x) for x in args]
+
+        server = self.connect_to_debug_monitor()
+        self.assertIsNotNone(server)
+        self.do_handshake()
+
+        for key, value in env.items():
+            hex_enc = seven.hexlify("%s=%s" % (key, value))
+            self.test_sequence.add_log_lines(
+                ["read packet: $QEnvironmentHexEncoded:%s#00" % (hex_enc,),
+                 "send packet: $OK#00"],
+                True)
+        self.test_sequence.add_log_lines(
+            ["read packet: $vRun;%s#00" % (";".join(hex_args),),
+             {"direction": "send",
+              "regex": r"^\$T([0-9a-fA-F]+)"},
+             "read packet: $c#00",
+             "send packet: $W00#00"],
+            True)
+        context = self.expect_gdbremote_sequence()
+        self.assertEqual(context["O_content"], b"test\r\na=z\r\na*}#z\r\n")

lldb/test/API/tools/lldb-server/TestLldbGdbServer.py

def test_P_and_p_thread_suffix_work(self):
             self.assertIsNotNone(p_response)
             read_value = lldbgdbserverutils.unpack_register_hex_unsigned(
                 endian, p_response)
 
             # Make sure we read back what we wrote.
             self.assertEqual(read_value, expected_reg_values[thread_index])
             thread_index += 1
 
-    @skipIfWindows # No pty support to test any inferior output
-    @add_test_categories(["llgs"])
-    def test_launch_via_A(self):
-        self.build()
-        exe_path = self.getBuildArtifact("a.out")
-        args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
-        hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
-
-        server = self.connect_to_debug_monitor()
-        self.assertIsNotNone(server)
-        self.do_handshake()
-        # NB: strictly speaking we should use %x here but this packet
-        # is deprecated, so no point in changing lldb-server's expectations
-        self.test_sequence.add_log_lines(
-            ["read packet: $A %d,0,%s,%d,1,%s,%d,2,%s,%d,3,%s#00" %
-             tuple(itertools.chain.from_iterable(
-                 [(len(x), x) for x in hex_args])),
-             "send packet: $OK#00",
-             "read packet: $c#00",
-             "send packet: $W00#00"],
-            True)
-        context = self.expect_gdbremote_sequence()
-        self.assertEqual(context["O_content"],
-                         b'arg1\r\narg2\r\narg3\r\n')
-
-    @skipIfWindows # No pty support to test any inferior output
-    @add_test_categories(["llgs"])
-    def test_launch_via_vRun(self):
-        self.build()
-        exe_path = self.getBuildArtifact("a.out")
-        args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
-        hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
-
-        server = self.connect_to_debug_monitor()
-        self.assertIsNotNone(server)
-        self.do_handshake()
-        self.test_sequence.add_log_lines(
-            ["read packet: $vRun;%s;%s;%s;%s#00" % tuple(hex_args),
-             {"direction": "send",
-              "regex": r"^\$T([0-9a-fA-F]+)"},
-             "read packet: $c#00",
-             "send packet: $W00#00"],
-            True)
-        context = self.expect_gdbremote_sequence()
-        self.assertEqual(context["O_content"],
-                         b'arg1\r\narg2\r\narg3\r\n')
-
-    @add_test_categories(["llgs"])
-    def test_launch_via_vRun_no_args(self):
-        self.build()
-        exe_path = self.getBuildArtifact("a.out")
-        hex_path = binascii.b2a_hex(exe_path.encode()).decode()
-
-        server = self.connect_to_debug_monitor()
-        self.assertIsNotNone(server)
-        self.do_handshake()
-        self.test_sequence.add_log_lines(
-            ["read packet: $vRun;%s#00" % (hex_path,),
-             {"direction": "send",
-              "regex": r"^\$T([0-9a-fA-F]+)"},
-             "read packet: $c#00",
-             "send packet: $W00#00"],
-            True)
-        self.expect_gdbremote_sequence()
-
-    @skipIfWindows # No pty support to test any inferior output
-    @add_test_categories(["llgs"])
-    def test_QEnvironment(self):
-        self.build()
-        exe_path = self.getBuildArtifact("a.out")
-        env = {"FOO": "test", "BAR": "a=z"}
-        args = [exe_path, "print-env:FOO", "print-env:BAR"]
-        hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
-
-        server = self.connect_to_debug_monitor()
-        self.assertIsNotNone(server)
-        self.do_handshake()
-
-        for key, value in env.items():
-            self.test_sequence.add_log_lines(
-                ["read packet: $QEnvironment:%s=%s#00" % (key, value),
-                 "send packet: $OK#00"],
-                True)
-        self.test_sequence.add_log_lines(
-            ["read packet: $vRun;%s#00" % (";".join(hex_args),),
-             {"direction": "send",
-              "regex": r"^\$T([0-9a-fA-F]+)"},
-             "read packet: $c#00",
-             "send packet: $W00#00"],
-            True)
-        context = self.expect_gdbremote_sequence()
-        self.assertEqual(context["O_content"], b"test\r\na=z\r\n")
-
-    @skipIfWindows # No pty support to test any inferior output
-    @add_test_categories(["llgs"])
-    def test_QEnvironmentHexEncoded(self):
-        self.build()
-        exe_path = self.getBuildArtifact("a.out")
-        env = {"FOO": "test", "BAR": "a=z", "BAZ": "a*}#z"}
-        args = [exe_path, "print-env:FOO", "print-env:BAR", "print-env:BAZ"]
-        hex_args = [binascii.b2a_hex(x.encode()).decode() for x in args]
-
-        server = self.connect_to_debug_monitor()
-        self.assertIsNotNone(server)
-        self.do_handshake()
-
-        for key, value in env.items():
-            hex_enc = binascii.b2a_hex(("%s=%s" % (key, value)).encode()).decode()
-            self.test_sequence.add_log_lines(
-                ["read packet: $QEnvironmentHexEncoded:%s#00" % (hex_enc,),
-                 "send packet: $OK#00"],
-                True)
-        self.test_sequence.add_log_lines(
-            ["read packet: $vRun;%s#00" % (";".join(hex_args),),
-             {"direction": "send",
-              "regex": r"^\$T([0-9a-fA-F]+)"},
-             "read packet: $c#00",
-             "send packet: $W00#00"],
-            True)
-        context = self.expect_gdbremote_sequence()
-        self.assertEqual(context["O_content"], b"test\r\na=z\r\na*}#z\r\n")
-
     @skipUnlessPlatform(oslist=["freebsd", "linux"])
     @add_test_categories(["llgs"])
     def test_qXfer_siginfo_read(self):
         self.build()
         self.set_inferior_startup_launch()
         procs = self.prep_debug_monitor_and_inferior(
                 inferior_args=["thread:segfault", "thread:new", "sleep:10"])
         self.test_sequence.add_log_lines(["read packet: $c#63"], True)