[lldb] [debugserver] fix qLaunchSuccess error, add QErrorStringInPacketSupported #82593

jasonmolenda · 2024-02-22T06:58:40Z

Pavel added an extension to lldb's gdb remote serial protocol that allows the debug stub to append an error message (ascii hex encoded) after an error response packet Exx. This was added in 2017 in https://reviews.llvm.org/D34945 . lldb sends the QErrorStringInPacketSupported packet and then the remote stub may add these error strings.

debugserver has two bugs in its use of extended error messages: the vAttach family would send the extended error string without checking if the mode had been enabled. And qLaunchSuccess would not properly format its error response packet (missing the hex digits, did not asciihex encode the string).

There is also a bug in the HandlePacket_D (detach) packet where the error packets did not include hex digits, but this one does not append an error string.

I'm adding a new RNBRemote::SendErrorPacket() and routing all error packet returns though this one method. It takes an optional second string which is the longer error message; it now handles appending it to the Exx response or not, depending on the QErrorStringInPacketSupported state. I updated all packets to send their errors via this method.

…etSupported Pavel added an extension to lldb's gdb remote serial protocol that allows the debug stub to append an error message (ascii hex encoded) after an error response packet Exx. This was added in 2017 in https://reviews.llvm.org/D34945 . lldb sends the QErrorStringInPacketSupported packet and then the remote stub may add these error strings. debugserver has added these strings to the vAttach family of packets to explain why an attach failed, without waiting to see the QErrorStringInPacketSupported packet to enable the mode. debugserver also has a bad implementation of this for the qLaunchSuccess packet, where "E" is sent followed by the literal characters of the error, instead of Exx;<asciihex string>, which lldb can have problems parsing. This patch moves three utility functions earlier in RNBRemote.cpp, it accepts the QErrorStringInPacketSupported packet and only appends the expanded error messages when that has been sent (lldb always sends it at the beginning of a connection), fixes the error string returned by qLaunchSuccess and changes the vAttach error returns to only add the error string when the mode is enabled.

llvmbot · 2024-02-22T06:59:11Z

@llvm/pr-subscribers-lldb

Author: Jason Molenda (jasonmolenda)

Changes

…etSupported

Pavel added an extension to lldb's gdb remote serial protocol that allows the debug stub to append an error message (ascii hex encoded) after an error response packet Exx. This was added in 2017 in https://reviews.llvm.org/D34945 . lldb sends the
QErrorStringInPacketSupported packet and then the remote stub may add these error strings.

debugserver has added these strings to the vAttach family of packets to explain why an attach failed, without waiting to see the QErrorStringInPacketSupported packet to enable the mode.

debugserver also has a bad implementation of this for the qLaunchSuccess packet, where "E" is sent followed by the literal characters of the error, instead of Exx;<asciihex string>, which lldb can have problems parsing.

This patch moves three utility functions earlier in RNBRemote.cpp, it accepts the QErrorStringInPacketSupported packet and only appends the expanded error messages when that has been sent (lldb always sends it at the beginning of a connection), fixes the error string returned by qLaunchSuccess and changes the vAttach error returns to only add the error string when the mode is enabled.

Full diff: https://github.com/llvm/llvm-project/pull/82593.diff

2 Files Affected:

(modified) lldb/tools/debugserver/source/RNBRemote.cpp (+98-59)
(modified) lldb/tools/debugserver/source/RNBRemote.h (+5)

diff --git a/lldb/tools/debugserver/source/RNBRemote.cpp b/lldb/tools/debugserver/source/RNBRemote.cpp
index feea4c914ec536..8338e4d0032870 100644
--- a/lldb/tools/debugserver/source/RNBRemote.cpp
+++ b/lldb/tools/debugserver/source/RNBRemote.cpp
@@ -143,6 +143,39 @@ uint64_t decode_uint64(const char *p, int base, char **end = nullptr,
   return addr;
 }
 
+void append_hex_value(std::ostream &ostrm, const void *buf, size_t buf_size,
+                      bool swap) {
+  int i;
+  const uint8_t *p = (const uint8_t *)buf;
+  if (swap) {
+    for (i = static_cast<int>(buf_size) - 1; i >= 0; i--)
+      ostrm << RAWHEX8(p[i]);
+  } else {
+    for (size_t i = 0; i < buf_size; i++)
+      ostrm << RAWHEX8(p[i]);
+  }
+}
+
+std::string cstring_to_asciihex_string(const char *str) {
+  std::string hex_str;
+  hex_str.reserve(strlen(str) * 2);
+  while (str && *str) {
+    uint8_t c = *str++;
+    char hexbuf[5];
+    snprintf(hexbuf, sizeof(hexbuf), "%02x", c);
+    hex_str += hexbuf;
+  }
+  return hex_str;
+}
+
+void append_hexified_string(std::ostream &ostrm, const std::string &string) {
+  size_t string_size = string.size();
+  const char *string_buf = string.c_str();
+  for (size_t i = 0; i < string_size; i++) {
+    ostrm << RAWHEX8(*(string_buf + i));
+  }
+}
+
 extern void ASLLogCallback(void *baton, uint32_t flags, const char *format,
                            va_list args);
 
@@ -171,7 +204,8 @@ RNBRemote::RNBRemote()
       m_extended_mode(false), m_noack_mode(false),
       m_thread_suffix_supported(false), m_list_threads_in_stop_reply(false),
       m_compression_minsize(384), m_enable_compression_next_send_packet(false),
-      m_compression_mode(compression_types::none) {
+      m_compression_mode(compression_types::none),
+      m_enable_error_strings(false) {
   DNBLogThreadedIf(LOG_RNB_REMOTE, "%s", __PRETTY_FUNCTION__);
   CreatePacketTable();
 }
@@ -365,6 +399,11 @@ void RNBRemote::CreatePacketTable() {
   t.push_back(Packet(
       query_symbol_lookup, &RNBRemote::HandlePacket_qSymbol, NULL, "qSymbol:",
       "Notify that host debugger is ready to do symbol lookups"));
+  t.push_back(Packet(enable_error_strings,
+                     &RNBRemote::HandlePacket_QEnableErrorStrings, NULL,
+                     "QEnableErrorStrings",
+                     "Tell " DEBUGSERVER_PROGRAM_NAME
+                     " it can append descriptive error messages in replies."));
   t.push_back(Packet(json_query_thread_extended_info,
                      &RNBRemote::HandlePacket_jThreadExtendedInfo, NULL,
                      "jThreadExtendedInfo",
@@ -1566,11 +1605,13 @@ rnb_err_t RNBRemote::HandlePacket_qLaunchSuccess(const char *p) {
   if (m_ctx.HasValidProcessID() || m_ctx.LaunchStatus().Status() == 0)
     return SendPacket("OK");
   std::ostringstream ret_str;
-  std::string status_str;
-  std::string error_quoted = binary_encode_string
-               (m_ctx.LaunchStatusAsString(status_str));
-  ret_str << "E" << error_quoted;
-
+  ret_str << "E89";
+  if (m_enable_error_strings) {
+    std::string status_str;
+    std::string error_quoted =
+        cstring_to_asciihex_string(m_ctx.LaunchStatusAsString(status_str));
+    ret_str << ";" << error_quoted;
+  }
   return SendPacket(ret_str.str());
 }
 
@@ -2534,39 +2575,6 @@ rnb_err_t RNBRemote::HandlePacket_QSetProcessEvent(const char *p) {
   return SendPacket("OK");
 }
 
-void append_hex_value(std::ostream &ostrm, const void *buf, size_t buf_size,
-                      bool swap) {
-  int i;
-  const uint8_t *p = (const uint8_t *)buf;
-  if (swap) {
-    for (i = static_cast<int>(buf_size) - 1; i >= 0; i--)
-      ostrm << RAWHEX8(p[i]);
-  } else {
-    for (size_t i = 0; i < buf_size; i++)
-      ostrm << RAWHEX8(p[i]);
-  }
-}
-
-std::string cstring_to_asciihex_string(const char *str) {
-  std::string hex_str;
-  hex_str.reserve (strlen (str) * 2);
-  while (str && *str) {
-    uint8_t c = *str++;
-    char hexbuf[5];
-    snprintf (hexbuf, sizeof(hexbuf), "%02x", c);
-    hex_str += hexbuf;
-  }
-  return hex_str;
-}
-
-void append_hexified_string(std::ostream &ostrm, const std::string &string) {
-  size_t string_size = string.size();
-  const char *string_buf = string.c_str();
-  for (size_t i = 0; i < string_size; i++) {
-    ostrm << RAWHEX8(*(string_buf + i));
-  }
-}
-
 void register_value_in_hex_fixed_width(std::ostream &ostrm, nub_process_t pid,
                                        nub_thread_t tid,
                                        const register_map_entry_t *reg,
@@ -3908,10 +3916,13 @@ rnb_err_t RNBRemote::HandlePacket_v(const char *p) {
     if (attach_pid == INVALID_NUB_PROCESS_ARCH) {
       DNBLogError("debugserver is x86_64 binary running in translation, attach "
                   "failed.");
-      std::string return_message = "E96;";
-      return_message +=
-          cstring_to_asciihex_string("debugserver is x86_64 binary running in "
-                                     "translation, attach failed.");
+      std::string return_message = "E96";
+      if (m_enable_error_strings) {
+        return_message += ";";
+        return_message += cstring_to_asciihex_string(
+            "debugserver is x86_64 binary running in "
+            "translation, attach failed.");
+      }
       SendPacket(return_message.c_str());
       return rnb_err;
     }
@@ -3944,15 +3955,22 @@ rnb_err_t RNBRemote::HandlePacket_v(const char *p) {
         // The order of these checks is important.  
         if (process_does_not_exist (pid_attaching_to)) {
           DNBLogError("Tried to attach to pid that doesn't exist");
-          std::string return_message = "E96;";
-          return_message += cstring_to_asciihex_string("no such process.");
+          std::string return_message = "E96";
+          if (m_enable_error_strings) {
+            return_message += ";";
+            return_message += cstring_to_asciihex_string("no such process.");
+          }
           return SendPacket(return_message);
         }
         if (process_is_already_being_debugged (pid_attaching_to)) {
           DNBLogError("Tried to attach to process already being debugged");
-          std::string return_message = "E96;";
-          return_message += cstring_to_asciihex_string("tried to attach to "
+          std::string return_message = "E96";
+          if (m_enable_error_strings) {
+            return_message += ";";
+            return_message +=
+                cstring_to_asciihex_string("tried to attach to "
                                            "process already being debugged");
+          }
           return SendPacket(return_message);
         }
         uid_t my_uid, process_uid;
@@ -3969,30 +3987,43 @@ rnb_err_t RNBRemote::HandlePacket_v(const char *p) {
             process_username = pw->pw_name;
           }
           DNBLogError("Tried to attach to process with uid mismatch");
-          std::string return_message = "E96;";
-          std::string msg = "tried to attach to process as user '" 
-                            + my_username + "' and process is running "
-                            "as user '" + process_username + "'";
-          return_message += cstring_to_asciihex_string(msg.c_str());
+          std::string return_message = "E96";
+          if (m_enable_error_strings) {
+            return_message += ";";
+            std::string msg = "tried to attach to process as user '" +
+                              my_username +
+                              "' and process is running "
+                              "as user '" +
+                              process_username + "'";
+            return_message += cstring_to_asciihex_string(msg.c_str());
+          }
           return SendPacket(return_message);
         }
         if (!login_session_has_gui_access() && !developer_mode_enabled()) {
           DNBLogError("Developer mode is not enabled and this is a "
                       "non-interactive session");
-          std::string return_message = "E96;";
-          return_message += cstring_to_asciihex_string("developer mode is "
+          std::string return_message = "E96";
+          if (m_enable_error_strings) {
+            return_message += ";";
+            return_message +=
+                cstring_to_asciihex_string("developer mode is "
                                            "not enabled on this machine "
                                            "and this is a non-interactive "
                                            "debug session.");
+          }
           return SendPacket(return_message);
         }
         if (!login_session_has_gui_access()) {
           DNBLogError("This is a non-interactive session");
-          std::string return_message = "E96;";
-          return_message += cstring_to_asciihex_string("this is a "
+          std::string return_message = "E96";
+          if (m_enable_error_strings) {
+            return_message += ";";
+            return_message +=
+                cstring_to_asciihex_string("this is a "
                                            "non-interactive debug session, "
                                            "cannot get permission to debug "
                                            "processes.");
+          }
           return SendPacket(return_message);
         }
       }
@@ -4013,9 +4044,12 @@ rnb_err_t RNBRemote::HandlePacket_v(const char *p) {
         error_explainer += err_str;
         error_explainer += ")";
       }
-      std::string default_return_msg = "E96;";
-      default_return_msg += cstring_to_asciihex_string 
-                              (error_explainer.c_str());
+      std::string default_return_msg = "E96";
+      if (m_enable_error_strings) {
+        default_return_msg += ";";
+        default_return_msg +=
+            cstring_to_asciihex_string(error_explainer.c_str());
+      }
       SendPacket (default_return_msg);
       DNBLogError("Attach failed: \"%s\".", err_str);
       return rnb_err;
@@ -6195,6 +6229,11 @@ rnb_err_t RNBRemote::HandlePacket_qSymbol(const char *command) {
   }
 }
 
+rnb_err_t RNBRemote::HandlePacket_QEnableErrorStrings(const char *p) {
+  m_enable_error_strings = true;
+  return SendPacket("OK");
+}
+
 // Note that all numeric values returned by qProcessInfo are hex encoded,
 // including the pid and the cpu type.
 
diff --git a/lldb/tools/debugserver/source/RNBRemote.h b/lldb/tools/debugserver/source/RNBRemote.h
index dad390ae0b6320..91f04fd6b40e11 100644
--- a/lldb/tools/debugserver/source/RNBRemote.h
+++ b/lldb/tools/debugserver/source/RNBRemote.h
@@ -137,6 +137,7 @@ class RNBRemote {
     set_detach_on_error,                // 'QSetDetachOnError:'
     query_transfer,                     // 'qXfer:'
     json_query_dyld_process_state,      // 'jGetDyldProcessState'
+    enable_error_strings,               // 'QEnableErrorStrings'
     unknown_type
   };
 
@@ -196,6 +197,7 @@ class RNBRemote {
   rnb_err_t HandlePacket_qGDBServerVersion(const char *p);
   rnb_err_t HandlePacket_qProcessInfo(const char *p);
   rnb_err_t HandlePacket_qSymbol(const char *p);
+  rnb_err_t HandlePacket_QEnableErrorStrings(const char *p);
   rnb_err_t HandlePacket_QStartNoAckMode(const char *p);
   rnb_err_t HandlePacket_QThreadSuffixSupported(const char *p);
   rnb_err_t HandlePacket_QSetLogging(const char *p);
@@ -405,6 +407,9 @@ class RNBRemote {
   bool m_enable_compression_next_send_packet;
 
   compression_types m_compression_mode;
+
+  bool m_enable_error_strings; // Whether we can append asciihex error strings
+                               // after Exx error replies
 };
 
 /* We translate the /usr/include/mach/exception_types.h exception types

github-actions · 2024-02-22T07:00:58Z

✅ With the latest revision this PR passed the C/C++ code formatter.

bulbazord

LGTM, seems like a good fix. One question/suggestion.

bulbazord · 2024-02-22T18:21:51Z

lldb/tools/debugserver/source/RNBRemote.cpp

@@ -3944,15 +3955,22 @@ rnb_err_t RNBRemote::HandlePacket_v(const char *p) {
        // The order of these checks is important.  
        if (process_does_not_exist (pid_attaching_to)) {
          DNBLogError("Tried to attach to pid that doesn't exist");
-          std::string return_message = "E96;";
-          return_message += cstring_to_asciihex_string("no such process.");
+          std::string return_message = "E96";


I see this pattern in lots of places, is it worth abstracting out? Something like:

std::string CreateAttachError(const char *additional_context = nullptr) { std::string message = "E96"; if (additional_context) message += ";" + cstring_to_asciihex_string(additional_context); return message; }

What do you think?

It might be nice to have a member function of RNBRemote that creates an error packet:

std::string RNBRemote::CreateEXXPacket(const char *exx_str, const char *error_detail) { std::string packet(exx_str); if (m_enable_error_strings) packet += ";" + cstring_to_asciihex_string(error_detail); return packet; }

Similar to what Alex suggested above, but making a member function allows access to the m_enable_error_strings ivar.

Then this line becomes:

std::string return_message = CreateEXXPacket("E96", "no such process");

I'm thinking of having a SendErrorPacket() instead of SendPacket() with an error string optional argument, and having all error packets route through that, and having all SendPacket("Exx")'s go through that instead. @bulbazord @clayborg what do you think? I'm having trouble deciding if I want to have a method that constructs the error string and the caller passes it to SendPacket or if I want to have a method to do both.

I'm thinking of having a SendErrorPacket() instead of SendPacket() with an error string optional argument, and having all error packets route through that, and having all SendPacket("Exx")'s go through that instead. @bulbazord @clayborg what do you think? I'm having trouble deciding if I want to have a method that constructs the error string and the caller passes it to SendPacket or if I want to have a method to do both.

I like that idea!

Sounds great to me 😄

clayborg · 2024-02-22T18:37:34Z

lldb/tools/debugserver/source/RNBRemote.cpp

@@ -3944,15 +3955,22 @@ rnb_err_t RNBRemote::HandlePacket_v(const char *p) {
        // The order of these checks is important.  
        if (process_does_not_exist (pid_attaching_to)) {
          DNBLogError("Tried to attach to pid that doesn't exist");
-          std::string return_message = "E96;";
-          return_message += cstring_to_asciihex_string("no such process.");
+          std::string return_message = "E96";


It might be nice to have a member function of RNBRemote that creates an error packet:

std::string RNBRemote::CreateEXXPacket(const char *exx_str, const char *error_detail) { std::string packet(exx_str); if (m_enable_error_strings) packet += ";" + cstring_to_asciihex_string(error_detail); return packet; }

Similar to what Alex suggested above, but making a member function allows access to the m_enable_error_strings ivar.

clayborg · 2024-02-22T18:39:57Z

lldb/tools/debugserver/source/RNBRemote.cpp

@@ -3944,15 +3955,22 @@ rnb_err_t RNBRemote::HandlePacket_v(const char *p) {
        // The order of these checks is important.  
        if (process_does_not_exist (pid_attaching_to)) {
          DNBLogError("Tried to attach to pid that doesn't exist");
-          std::string return_message = "E96;";
-          return_message += cstring_to_asciihex_string("no such process.");
+          std::string return_message = "E96";


Then this line becomes:

std::string return_message = CreateEXXPacket("E96", "no such process");

clayborg · 2024-02-22T18:40:38Z

lldb/tools/debugserver/source/RNBRemote.cpp

+          std::string return_message = "E96";
+          if (m_enable_error_strings) {
+            return_message += ";";
+            return_message += cstring_to_asciihex_string("no such process.");


we probably want all error details to be ended with a '.' or all of them to not have a '.' at the end. See other error message below that don't have a '.' character

clayborg · 2024-02-22T18:41:44Z

lldb/tools/debugserver/source/RNBRemote.cpp

+                cstring_to_asciihex_string("tried to attach to "
                                           "process already being debugged");


not ended with a '.' here, we pick how we want to do it and do it consistently

clayborg · 2024-02-22T18:41:52Z

lldb/tools/debugserver/source/RNBRemote.cpp

+            std::string msg = "tried to attach to process as user '" +
+                              my_username +
+                              "' and process is running "
+                              "as user '" +
+                              process_username + "'";


not ended with a '.' here, we pick how we want to do it and do it consistently

clayborg · 2024-02-22T18:42:46Z

lldb/tools/debugserver/source/RNBRemote.cpp

+                cstring_to_asciihex_string("developer mode is "
                                           "not enabled on this machine "
                                           "and this is a non-interactive "
                                           "debug session.");


this is ended with a '.', we pick how we want to do it and do it consistently

clayborg · 2024-02-22T18:42:53Z

lldb/tools/debugserver/source/RNBRemote.cpp

+                cstring_to_asciihex_string("this is a "
                                           "non-interactive debug session, "
                                           "cannot get permission to debug "
                                           "processes.");


this is ended with a '.', we pick how we want to do it and do it consistently

jasonmolenda · 2024-02-22T21:58:01Z

CreateEXXPacket("E96", "no such process"); that's a good idea, I'll rewrite to use a helper method like that.

Have all error packet replies go through `SendErrorPacket()` with an optional string explaining the error, and send the optional string if extended error message reporting is enabled. Also found that RNBRemote::HandlePacket_D was not sending properly formatted error packets if there was an error; fixed that.

jasonmolenda · 2024-03-01T01:01:38Z

Going back to this PR, @bulbazord @clayborg and I agreed that having a single method for sending error reply packets, which correctly encode the error string it if is available and extended-error-responses has been enabled, would be worth modifying all the error returns, so I did that. I found an additional bad error response in HandlePacket_D where it did not include hex digits on its error response.

I think we're good to land this now.

bulbazord

Looks great!!! 😄 Thanks for fixing the bug and gardening!

jasonmolenda requested a review from JDevlieghere as a code owner February 22, 2024 06:58

llvmbot added the lldb label Feb 22, 2024

DavidSpickett changed the title ~~[lldb] [debugserver] fix qLaunchSuccess error, add QErrorStringInPack…~~ [lldb] [debugserver] fix qLaunchSuccess error, add QErrorStringInPacketSupported Feb 22, 2024

bulbazord approved these changes Feb 22, 2024

View reviewed changes

clayborg reviewed Feb 22, 2024

View reviewed changes

jasonmolenda added 2 commits February 29, 2024 16:37

Keep the RNBRemote::PacketEnum table hand-laid-out

f7bada9

bulbazord approved these changes Mar 1, 2024

View reviewed changes

Merge branch 'main' into fix-error-string-returns-from-debugserver

b57a578

jasonmolenda merged commit 4551f53 into llvm:main Mar 1, 2024
4 checks passed

jasonmolenda deleted the fix-error-string-returns-from-debugserver branch March 1, 2024 04:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lldb] [debugserver] fix qLaunchSuccess error, add QErrorStringInPacketSupported #82593

[lldb] [debugserver] fix qLaunchSuccess error, add QErrorStringInPacketSupported #82593

jasonmolenda commented Feb 22, 2024 •

edited

llvmbot commented Feb 22, 2024

github-actions bot commented Feb 22, 2024 •

edited

bulbazord left a comment

bulbazord Feb 22, 2024

clayborg Feb 22, 2024

clayborg Feb 22, 2024

jasonmolenda Feb 23, 2024

clayborg Feb 24, 2024

bulbazord Feb 24, 2024

clayborg Feb 22, 2024

clayborg Feb 22, 2024

clayborg Feb 22, 2024

clayborg Feb 22, 2024

clayborg Feb 22, 2024

clayborg Feb 22, 2024

clayborg Feb 22, 2024

jasonmolenda commented Feb 22, 2024

jasonmolenda commented Mar 1, 2024

bulbazord left a comment

		cstring_to_asciihex_string("tried to attach to "
		"process already being debugged");

[lldb] [debugserver] fix qLaunchSuccess error, add QErrorStringInPacketSupported #82593

[lldb] [debugserver] fix qLaunchSuccess error, add QErrorStringInPacketSupported #82593

Conversation

jasonmolenda commented Feb 22, 2024 • edited

llvmbot commented Feb 22, 2024

github-actions bot commented Feb 22, 2024 • edited

bulbazord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmolenda commented Feb 22, 2024

jasonmolenda commented Mar 1, 2024

bulbazord left a comment

Choose a reason for hiding this comment

jasonmolenda commented Feb 22, 2024 •

edited

github-actions bot commented Feb 22, 2024 •

edited