[Lldb-commits] [PATCH] D134754: [lldb/gdb-server] Better reporting of launch errors

[Pavel Labath via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8d1de7b34af4: [lldb/gdb-server] Better reporting of launch 
errors (authored by labath).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134754

Files:
  lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py

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
@@ -86,7 +86,30 @@
 error = lldb.SBError()
 target.Launch(lldb.SBListener(), None, None, None, None, None,
 None, 0, True, error)
-self.assertEquals("'A' packet returned an error: 71", error.GetCString())
+self.assertRegex(error.GetCString(), "Cannot launch '.*a': Error 71")
+
+def test_launch_rich_error(self):
+class MyResponder(MockGDBServerResponder):
+def qC(self):
+return "E42"
+
+def qfThreadInfo(self):
+return "OK" # No threads.
+
+# Then, when we are asked to attach, error out.
+def vRun(self, packet):
+return "Eff;" + seven.hexlify("I'm a teapot")
+
+self.server.responder = MyResponder()
+
+target = self.createTarget("a.yaml")
+process = self.connect(target)
+lldbutil.expect_state_changes(self, self.dbg.GetListener(), process, [lldb.eStateConnected])
+
+error = lldb.SBError()
+target.Launch(lldb.SBListener(), None, None, None, None, None,
+None, 0, True, error)
+self.assertRegex(error.GetCString(), "Cannot launch '.*a': I'm a teapot")
 
 def test_read_registers_using_g_packets(self):
 """Test reading registers using 'g' packets (default behavior)"""
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -84,6 +84,7 @@
 
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/Support/FormatAdapters.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/raw_ostream.h"
 
@@ -799,17 +800,17 @@
   GDBRemoteCommunication::ScopedTimeout timeout(m_gdb_comm,
 std::chrono::seconds(10));
 
-  int arg_packet_err = m_gdb_comm.SendArgumentsPacket(launch_info);
-  if (arg_packet_err == 0) {
-std::string error_str;
-if (m_gdb_comm.GetLaunchSuccess(error_str)) {
-  SetID(m_gdb_comm.GetCurrentProcessID());
-} else {
-  error.SetErrorString(error_str.c_str());
-}
+  // Since we can't send argv0 separate from the executable path, we need to
+  // make sure to use the actual executable path found in the launch_info...
+  Args args = launch_info.GetArguments();
+  if (FileSpec exe_file = launch_info.GetExecutableFile())
+args.ReplaceArgumentAtIndex(0, exe_file.GetPath(false));
+  if (llvm::Error err = m_gdb_comm.LaunchProcess(args)) {
+error.SetErrorStringWithFormatv("Cannot launch '{0}': {1}",
+args.GetArgumentAtIndex(0),
+llvm::fmt_consume(std::move(err)));
   } else {
-error.SetErrorStringWithFormat("'A' packet returned an error: %i",
-   arg_packet_err);
+SetID(m_gdb_comm.GetCurrentProcessID());
   }
 }
 
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
@@ -80,8 +80,6 @@
 
   lldb::pid_t GetCurrentProcessID(bool allow_lazy = true);
 
-  bool GetLaunchSuccess(std::string _str);
-
   bool LaunchGDBServer(const char *remote_accept_hostname, lldb::pid_t ,
uint16_t , std::string _name);
 
@@ -90,19 +88,11 @@
 
   bool KillSpawnedProcess(lldb::pid_t pid);
 
-  /// Sends a GDB remote protocol 'A' packet that delivers program
-  /// arguments to the remote server.
-  ///
-  /// \param[in] launch_info
-  /// A NULL terminated array of const C strings 

[Lldb-commits] [PATCH] D134754: [lldb/gdb-server] Better reporting of launch errors

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: mgorny, jasonmolenda.
Herald added a project: All.
labath requested review of this revision.
Herald added a project: LLDB.

Use our "rich error" facility to propagate error reported by the stub to
the user. lldb-server reports rich launch errors as of D133352 
.

To make this easier to implement, and reduce code duplication, I have
moved the vRun/A/qLaunchSuccess handling into a single
GDBRemoteCommunicationClient function.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134754

Files:
  lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py

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
@@ -86,7 +86,30 @@
 error = lldb.SBError()
 target.Launch(lldb.SBListener(), None, None, None, None, None,
 None, 0, True, error)
-self.assertEquals("'A' packet returned an error: 71", error.GetCString())
+self.assertRegex(error.GetCString(), "Cannot launch '.*a': Error 71")
+
+def test_launch_rich_error(self):
+class MyResponder(MockGDBServerResponder):
+def qC(self):
+return "E42"
+
+def qfThreadInfo(self):
+return "OK" # No threads.
+
+# Then, when we are asked to attach, error out.
+def vRun(self, packet):
+return "Eff;" + seven.hexlify("I'm a teapot")
+
+self.server.responder = MyResponder()
+
+target = self.createTarget("a.yaml")
+process = self.connect(target)
+lldbutil.expect_state_changes(self, self.dbg.GetListener(), process, [lldb.eStateConnected])
+
+error = lldb.SBError()
+target.Launch(lldb.SBListener(), None, None, None, None, None,
+None, 0, True, error)
+self.assertRegex(error.GetCString(), "Cannot launch '.*a': I'm a teapot")
 
 def test_read_registers_using_g_packets(self):
 """Test reading registers using 'g' packets (default behavior)"""
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -84,6 +84,7 @@
 
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/Support/FormatAdapters.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/raw_ostream.h"
 
@@ -799,17 +800,17 @@
   GDBRemoteCommunication::ScopedTimeout timeout(m_gdb_comm,
 std::chrono::seconds(10));
 
-  int arg_packet_err = m_gdb_comm.SendArgumentsPacket(launch_info);
-  if (arg_packet_err == 0) {
-std::string error_str;
-if (m_gdb_comm.GetLaunchSuccess(error_str)) {
-  SetID(m_gdb_comm.GetCurrentProcessID());
-} else {
-  error.SetErrorString(error_str.c_str());
-}
+  // Since we can't send argv0 separate from the executable path, we need to
+  // make sure to use the actual executable path found in the launch_info...
+  Args args = launch_info.GetArguments();
+  if (FileSpec exe_file = launch_info.GetExecutableFile())
+args.ReplaceArgumentAtIndex(0, exe_file.GetPath(false));
+  if (llvm::Error err = m_gdb_comm.LaunchProcess(args)) {
+error.SetErrorStringWithFormatv("Cannot launch '{0}': {1}",
+args.GetArgumentAtIndex(0),
+llvm::fmt_consume(std::move(err)));
   } else {
-error.SetErrorStringWithFormat("'A' packet returned an error: %i",
-   arg_packet_err);
+SetID(m_gdb_comm.GetCurrentProcessID());
   }
 }
 
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
@@ -80,8 +80,6 @@
 
   lldb::pid_t GetCurrentProcessID(bool allow_lazy = true);
 
-  bool GetLaunchSuccess(std::string _str);
-
   bool LaunchGDBServer(const char *remote_accept_hostname, lldb::pid_t ,
uint16_t , std::string _name);
 
@@ -90,19 +88,11 @@
 
   bool KillSpawnedProcess(lldb::pid_t pid);
 
-  /// Sends