mgorny updated this revision to Diff 366090.
mgorny added a comment.

Handle unsupported `qLaunchSuccess` gracefully on the client. This effectively 
makes it possible to launch programs via gdbserver.

Enhance the client tests to verify that the process has actually 'started'.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107931/new/

https://reviews.llvm.org/D107931

Files:
  lldb/include/lldb/Utility/StringExtractorGDBRemote.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/source/Utility/StringExtractorGDBRemote.cpp
  lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py
  lldb/test/API/functionalities/gdb_remote_client/gdbclientutils.py
  lldb/test/API/tools/lldb-server/TestLldbGdbServer.py

Index: lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
===================================================================
--- lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
+++ lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
@@ -10,6 +10,9 @@
 the initial set of tests implemented.
 """
 
+import binascii
+import itertools
+
 import unittest2
 import gdbremote_testcase
 import lldbgdbserverutils
@@ -1246,3 +1249,61 @@
             # Make sure we read back what we wrote.
             self.assertEqual(read_value, expected_reg_values[thread_index])
             thread_index += 1
+
+    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')
+
+    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()
+
+    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()
Index: lldb/test/API/functionalities/gdb_remote_client/gdbclientutils.py
===================================================================
--- lldb/test/API/functionalities/gdb_remote_client/gdbclientutils.py
+++ lldb/test/API/functionalities/gdb_remote_client/gdbclientutils.py
@@ -183,6 +183,10 @@
             return self.qPathComplete()
         if packet.startswith("vFile:"):
             return self.vFile(packet)
+        if packet.startswith("vRun;"):
+            return self.vRun(packet)
+        if packet.startswith("qLaunchSuccess"):
+            return self.qLaunchSuccess()
 
         return self.other(packet)
 
@@ -293,6 +297,12 @@
     def vFile(self, packet):
         return ""
 
+    def vRun(self, packet):
+        return ""
+
+    def qLaunchSuccess(self):
+        return ""
+
     """
     Raised when we receive a packet for which there is no default action.
     Override the responder class to implement behavior suitable for the test at
Index: lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py
===================================================================
--- lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py
+++ lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py
@@ -57,7 +57,6 @@
             def A(self, packet):
                 return "E47"
 
-        self.runCmd("log enable gdb-remote packets")
         self.server.responder = MyResponder()
 
         target = self.createTarget("a.yaml")
@@ -148,3 +147,114 @@
         self.assertGreater(numChildren, 0)
         for i in range(numChildren):
             operation(regSet.GetChildAtIndex(i))
+
+    def test_launch_A(self):
+        class MyResponder(MockGDBServerResponder):
+            def __init__(self, *args, **kwargs):
+                self.started = False
+                return super().__init__(*args, **kwargs)
+
+            def qC(self):
+                if self.started:
+                    return "QCp10.10"
+                else:
+                    return "E42"
+
+            def qfThreadInfo(self):
+                if self.started:
+                    return "mp10.10"
+                else:
+                   return "E42"
+
+            def qsThreadInfo(self):
+                return "l"
+
+            def A(self, packet):
+                self.started = True
+                return "OK"
+
+            def qLaunchSuccess(self):
+                if self.started:
+                    return "OK"
+                return "E42"
+
+        self.server.responder = MyResponder()
+
+        target = self.createTarget("a.yaml")
+        exe_path = self.getBuildArtifact("a")
+        exe_hex = binascii.b2a_hex(exe_path.encode()).decode()
+        process = self.connect(target)
+        lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
+                                      [lldb.eStateConnected])
+
+        target.Launch(lldb.SBListener(),
+                      ["arg1", "arg2", "arg3"],  # argv
+                      [],  # envp
+                      None,  # stdin_path
+                      None,  # stdout_path
+                      None,  # stderr_path
+                      None,  # working_directory
+                      0,  # launch_flags
+                      True,  # stop_at_entry
+                      lldb.SBError())  # error
+        self.assertTrue(process, PROCESS_IS_VALID)
+        self.assertEqual(process.GetProcessID(), 16)
+
+        self.assertPacketLogContains([
+          "A%d,0,%s,8,1,61726731,8,2,61726732,8,3,61726733" % (
+              len(exe_hex), exe_hex),
+        ])
+
+    def test_launch_vRun(self):
+        class MyResponder(MockGDBServerResponder):
+            def __init__(self, *args, **kwargs):
+                self.started = False
+                return super().__init__(*args, **kwargs)
+
+            def qC(self):
+                if self.started:
+                    return "QCp10.10"
+                else:
+                    return "E42"
+
+            def qfThreadInfo(self):
+                if self.started:
+                    return "mp10.10"
+                else:
+                   return "E42"
+
+            def qsThreadInfo(self):
+                return "l"
+
+            def vRun(self, packet):
+                self.started = True
+                return "T13"
+
+            def A(self, packet):
+                return "E28"
+
+        self.server.responder = MyResponder()
+
+        target = self.createTarget("a.yaml")
+        exe_path = self.getBuildArtifact("a")
+        exe_hex = binascii.b2a_hex(exe_path.encode()).decode()
+        process = self.connect(target)
+        lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
+                                      [lldb.eStateConnected])
+
+        process = target.Launch(lldb.SBListener(),
+                                ["arg1", "arg2", "arg3"],  # argv
+                                [],  # envp
+                                None,  # stdin_path
+                                None,  # stdout_path
+                                None,  # stderr_path
+                                None,  # working_directory
+                                0,  # launch_flags
+                                True,  # stop_at_entry
+                                lldb.SBError())  # error
+        self.assertTrue(process, PROCESS_IS_VALID)
+        self.assertEqual(process.GetProcessID(), 16)
+
+        self.assertPacketLogContains([
+          "vRun;%s;61726731;61726732;61726733" % (exe_hex,)
+        ])
Index: lldb/source/Utility/StringExtractorGDBRemote.cpp
===================================================================
--- lldb/source/Utility/StringExtractorGDBRemote.cpp
+++ lldb/source/Utility/StringExtractorGDBRemote.cpp
@@ -363,6 +363,8 @@
         return eServerPacketType_vCont;
       if (PACKET_MATCHES("vCont?"))
         return eServerPacketType_vCont_actions;
+      if (PACKET_STARTS_WITH("vRun;"))
+        return eServerPacketType_vRun;
     }
     break;
   case '_':
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
@@ -202,6 +202,8 @@
 
   PacketResult Handle_vAttachOrWait(StringExtractorGDBRemote &packet);
 
+  PacketResult Handle_vRun(StringExtractorGDBRemote &packet);
+
   PacketResult Handle_D(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_qThreadStopInfo(StringExtractorGDBRemote &packet);
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -182,6 +182,9 @@
   RegisterMemberFunctionHandler(
       StringExtractorGDBRemote::eServerPacketType_vCont_actions,
       &GDBRemoteCommunicationServerLLGS::Handle_vCont_actions);
+  RegisterMemberFunctionHandler(
+      StringExtractorGDBRemote::eServerPacketType_vRun,
+      &GDBRemoteCommunicationServerLLGS::Handle_vRun);
   RegisterMemberFunctionHandler(
       StringExtractorGDBRemote::eServerPacketType_x,
       &GDBRemoteCommunicationServerLLGS::Handle_memory_read);
@@ -3251,6 +3254,38 @@
   return SendStopReasonForState(m_current_process->GetState());
 }
 
+GDBRemoteCommunication::PacketResult
+GDBRemoteCommunicationServerLLGS::Handle_vRun(
+    StringExtractorGDBRemote &packet) {
+  Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS));
+
+  llvm::StringRef s = packet.GetStringRef();
+  if (!s.consume_front("vRun;"))
+    return SendErrorResponse(8);
+
+  llvm::SmallVector<llvm::StringRef, 16> argv;
+  s.split(argv, ';');
+
+  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())
+      return SendStopReasonForState(m_current_process->GetState());
+    LLDB_LOG(log, "failed to launch exe: {0}", m_process_launch_error);
+  }
+  return SendErrorResponse(8);
+}
+
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::Handle_D(StringExtractorGDBRemote &packet) {
   StopSTDIOForwarding();
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
@@ -598,7 +598,8 @@
       m_supports_qSymbol : 1, m_qSymbol_requests_done : 1,
       m_supports_qModuleInfo : 1, m_supports_jThreadsInfo : 1,
       m_supports_jModulesInfo : 1, m_supports_vFileSize : 1,
-      m_supports_vFileMode : 1, m_supports_vFileExists : 1;
+      m_supports_vFileMode : 1, m_supports_vFileExists : 1,
+      m_supports_vRun : 1;
 
   /// Current gdb remote protocol process identifier for all other operations
   lldb::pid_t m_curr_pid = LLDB_INVALID_PROCESS_ID;
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -66,7 +66,7 @@
       m_qSymbol_requests_done(false), m_supports_qModuleInfo(true),
       m_supports_jThreadsInfo(true), m_supports_jModulesInfo(true),
       m_supports_vFileSize(true), m_supports_vFileMode(true),
-      m_supports_vFileExists(true),
+      m_supports_vFileExists(true), m_supports_vRun(true),
 
       m_host_arch(), m_process_arch(), m_os_build(), m_os_kernel(),
       m_hostname(), m_gdb_server_name(), m_default_packet_timeout(0),
@@ -767,6 +767,11 @@
       PacketResult::Success) {
     if (response.IsOKResponse())
       return true;
+    // GDB does not implement qLaunchSuccess -- but if we used vRun,
+    // then we already received a successful launch indication via stop
+    // reason.
+    if (response.IsUnsupportedResponse() && m_supports_vRun)
+      return true;
     if (response.GetChar() == 'E') {
       // A string the describes what failed when launching...
       error_str = std::string(response.GetStringRef().substr(1));
@@ -805,6 +810,36 @@
     }
   }
   if (!argv.empty()) {
+    // try vRun first
+    if (m_supports_vRun) {
+      StreamString packet;
+      packet.PutCString("vRun");
+      for (const char *arg : argv) {
+        packet.PutChar(';');
+        packet.PutBytesAsRawHex8(arg, strlen(arg));
+      }
+
+      StringExtractorGDBRemote response;
+      if (SendPacketAndWaitForResponse(packet.GetString(), response) !=
+          PacketResult::Success)
+        return -1;
+
+      if (response.IsErrorResponse()) {
+        uint8_t error = response.GetError();
+        if (error)
+          return error;
+        return -1;
+      }
+      // vRun replies with a stop reason packet
+      // FIXME: right now we just discard the packet and LLDB queries
+      // for stop reason again
+      if (!response.IsUnsupportedResponse())
+        return 0;
+
+      m_supports_vRun = false;
+    }
+
+    // fallback to A
     StreamString packet;
     packet.PutChar('A');
     for (size_t i = 0, n = argv.size(); i < n; ++i) {
Index: lldb/include/lldb/Utility/StringExtractorGDBRemote.h
===================================================================
--- lldb/include/lldb/Utility/StringExtractorGDBRemote.h
+++ lldb/include/lldb/Utility/StringExtractorGDBRemote.h
@@ -136,6 +136,7 @@
     eServerPacketType_vAttachName,
     eServerPacketType_vCont,
     eServerPacketType_vCont_actions, // vCont?
+    eServerPacketType_vRun,
 
     eServerPacketType_stop_reason, // '?'
 
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to