Adding Support for Error Strings in Remote Packets

Summary:
This patch adds support for sending strings along with
error codes in the reply packets. The implementation is
based on the feedback recieved in the lldb-dev mailing
list. The patch also adds an extra packet for the client
to query if the server has the capability to provide
strings along with error replys.

Reviewers: labath, jingham, sas, lldb-commits, clayborg

Reviewed By: labath, clayborg

Differential Revision: https://reviews.llvm.org/D34945

llvm-svn: 307768

lldb/docs/lldb-gdb-remote.txt

@@ -125,6 +125,32 @@ read packet: $OK#00
 
 This packet can be sent one or more times _prior_ to sending a "A" packet.
 
+//----------------------------------------------------------------------
+// "QEnableErrorStrings"
+//
+// BRIEF
+//  This packet enables reporting of Error strings in remote packet
+//  replies from the server to client. If the server supports this
+//  feature, it should send an OK response. The client can expect the
+//  following error replies if this feature is enabled in the server ->
+//
+//  EXX;AAAAAAAAA
+//
+//  where AAAAAAAAA will be a hex encoded ASCII string.
+//  XX is hex encoded byte number.
+//
+//  It must be noted that even if the client has enabled reporting
+//  strings in error replies, it must not expect error strings to all
+//  error replies.
+// 
+// PRIORITY TO IMPLEMENT
+//  Low. Only needed if the remote target wants to provide strings that
+//  are human readable along with an error code.
+//----------------------------------------------------------------------
+
+send packet: $QErrorStringInPacketSupported
+read packet: $OK#00
+
 //----------------------------------------------------------------------
 // "QSetSTDIN:<ascii-hex-path>"
 // "QSetSTDOUT:<ascii-hex-path>"
@@ -250,11 +276,12 @@ read packet: OK
 //
 //  Each tracing instance is identified by a trace id which is returned
 //  as the reply to this packet. In case the tracing failed to begin an
-//  error code is returned instead.
+//  error code along with a hex encoded ASCII message is returned
+//  instead.
 //----------------------------------------------------------------------
 
 send packet: jTraceStart:{"type":<type>,"buffersize":<buffersize>}]
-read packet: <trace id>/E<error code>
+read packet: <trace id>/E<error code>;AAAAAAAAA
 
 //----------------------------------------------------------------------
 // jTraceStop:
@@ -283,12 +310,12 @@ read packet: <trace id>/E<error code>
 //                  to stop tracing on that thread.
 //  ==========      ====================================================
 //
-//  An OK response is sent in case of success else an error code is
-//  returned.
+//  An OK response is sent in case of success else an error code along
+//  with a hex encoded ASCII message is returned.
 //----------------------------------------------------------------------
 
 send packet: jTraceStop:{"traceid":<trace id>}]
-read packet: <OK response>/E<error code>
+read packet: <OK response>/E<error code>;AAAAAAAAA
 
 //----------------------------------------------------------------------
 // jTraceBufferRead:
@@ -317,11 +344,11 @@ read packet: <OK response>/E<error code>
 //  ==========      ====================================================
 //
 //  The trace data is sent as raw binary data if the read was successful
-//  else an error code is sent.
+//  else an error code along with a hex encoded ASCII message is sent.
 //----------------------------------------------------------------------
 
 send packet: jTraceBufferRead:{"traceid":<trace id>,"offset":<byteoffset>,"buffersize":<byte_count>}]
-read packet: <binary trace data>/E<error code>
+read packet: <binary trace data>/E<error code>;AAAAAAAAA
 
 //----------------------------------------------------------------------
 // jTraceMetaRead:
@@ -359,11 +386,11 @@ read packet: <binary trace data>/E<error code>
 //  gdb-remote protocol has certain limitations, binary escaping
 //  convention is used.
 //  In case the trace instance with the <trace id> was not found, an
-//  error code is returned.
+//  error code along with a hex encoded ASCII message is returned.
 //----------------------------------------------------------------------
 
 send packet: jTraceConfigRead:{"traceid":<trace id>}
-read packet: {"conf1":<conf1>,"conf2":<conf2>,"params":{"paramName":paramValue}]}];/E<error code>
+read packet: {"conf1":<conf1>,"conf2":<conf2>,"params":{"paramName":paramValue}]}];/E<error code>;AAAAAAAAA
 
 //----------------------------------------------------------------------
 // "qRegisterInfo<hex-reg-id>"

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

@@ -86,6 +86,7 @@ GDBRemoteCommunicationClient::GDBRemoteCommunicationClient()
       m_supports_jLoadedDynamicLibrariesInfos(eLazyBoolCalculate),
       m_supports_jGetSharedCacheInfo(eLazyBoolCalculate),
       m_supports_QPassSignals(eLazyBoolCalculate),
+      m_supports_error_string_reply(eLazyBoolCalculate),
       m_supports_qProcessInfoPID(true), m_supports_qfProcessInfo(true),
       m_supports_qUserName(true), m_supports_qGroupName(true),
       m_supports_qThreadStopInfo(true), m_supports_z0(true),
@@ -596,6 +597,21 @@ bool GDBRemoteCommunicationClient::GetThreadExtendedInfoSupported() {
   return m_supports_jThreadExtendedInfo;
 }
 
+void GDBRemoteCommunicationClient::EnableErrorStringInPacket() {
+  if (m_supports_error_string_reply == eLazyBoolCalculate) {
+    StringExtractorGDBRemote response;
+    // We try to enable error strings in remote packets
+    // but if we fail, we just work in the older way.
+    m_supports_error_string_reply = eLazyBoolNo;
+    if (SendPacketAndWaitForResponse("QEnableErrorStrings", response, false) ==
+        PacketResult::Success) {
+      if (response.IsOKResponse()) {
+        m_supports_error_string_reply = eLazyBoolYes;
+      }
+    }
+  }
+}
+
 bool GDBRemoteCommunicationClient::GetLoadedDynamicLibrariesInfosSupported() {
   if (m_supports_jLoadedDynamicLibrariesInfos == eLazyBoolCalculate) {
     StringExtractorGDBRemote response;
@@ -3181,8 +3197,8 @@ GDBRemoteCommunicationClient::SendStartTracePacket(const TraceOptions &options,
                                    true) ==
       GDBRemoteCommunication::PacketResult::Success) {
     if (!response.IsNormalResponse()) {
-      error.SetError(response.GetError(), eErrorTypeGeneric);
-      LLDB_LOG(log, "Target does not support Tracing");
+      error = response.GetStatus();
+      LLDB_LOG(log, "Target does not support Tracing , error {0}", error);
     } else {
       ret_uid = response.GetHexMaxU64(false, LLDB_INVALID_UID);
     }
@@ -3219,7 +3235,7 @@ GDBRemoteCommunicationClient::SendStopTracePacket(lldb::user_id_t uid,
                                    true) ==
       GDBRemoteCommunication::PacketResult::Success) {
     if (!response.IsOKResponse()) {
-      error.SetError(response.GetError(), eErrorTypeGeneric);
+      error = response.GetStatus();
       LLDB_LOG(log, "stop tracing failed");
     }
   } else {
@@ -3234,6 +3250,7 @@ GDBRemoteCommunicationClient::SendStopTracePacket(lldb::user_id_t uid,
 Status GDBRemoteCommunicationClient::SendGetDataPacket(
     lldb::user_id_t uid, lldb::tid_t thread_id,
     llvm::MutableArrayRef<uint8_t> &buffer, size_t offset) {
+
   StreamGDBRemote escaped_packet;
   escaped_packet.PutCString("jTraceBufferRead:");
   return SendGetTraceDataPacket(escaped_packet, uid, thread_id, buffer, offset);
@@ -3242,6 +3259,7 @@ Status GDBRemoteCommunicationClient::SendGetDataPacket(
 Status GDBRemoteCommunicationClient::SendGetMetaDataPacket(
     lldb::user_id_t uid, lldb::tid_t thread_id,
     llvm::MutableArrayRef<uint8_t> &buffer, size_t offset) {
+
   StreamGDBRemote escaped_packet;
   escaped_packet.PutCString("jTraceMetaRead:");
   return SendGetTraceDataPacket(escaped_packet, uid, thread_id, buffer, offset);
@@ -3308,7 +3326,7 @@ GDBRemoteCommunicationClient::SendGetTraceConfigPacket(lldb::user_id_t uid,
                   custom_params_sp));
       }
     } else {
-      error.SetError(response.GetError(), eErrorTypeGeneric);
+      error = response.GetStatus();
     }
   } else {
     LLDB_LOG(log, "failed to send packet");
@@ -3344,7 +3362,7 @@ Status GDBRemoteCommunicationClient::SendGetTraceDataPacket(
       size_t filled_size = response.GetHexBytesAvail(buffer);
       buffer = llvm::MutableArrayRef<uint8_t>(buffer.data(), filled_size);
     } else {
-      error.SetError(response.GetError(), eErrorTypeGeneric);
+      error = response.GetStatus();
       buffer = buffer.slice(buffer.size());
     }
   } else {

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h

@@ -340,6 +340,8 @@ class GDBRemoteCommunicationClient : public GDBRemoteClientBase {
 
   bool GetQXferAuxvReadSupported();
 
+  void EnableErrorStringInPacket();
+
   bool GetQXferLibrariesReadSupported();
 
   bool GetQXferLibrariesSVR4ReadSupported();
@@ -549,6 +551,7 @@ class GDBRemoteCommunicationClient : public GDBRemoteClientBase {
   LazyBool m_supports_jLoadedDynamicLibrariesInfos;
   LazyBool m_supports_jGetSharedCacheInfo;
   LazyBool m_supports_QPassSignals;
+  LazyBool m_supports_error_string_reply;
 
   bool m_supports_qProcessInfoPID : 1, m_supports_qfProcessInfo : 1,
       m_supports_qUserName : 1, m_supports_qGroupName : 1,

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

@@ -20,14 +20,20 @@
 // Project includes
 #include "ProcessGDBRemoteLog.h"
 #include "Utility/StringExtractorGDBRemote.h"
+#include "lldb/Utility/StreamString.h"
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::process_gdb_remote;
 
 GDBRemoteCommunicationServer::GDBRemoteCommunicationServer(
     const char *comm_name, const char *listener_name)
-    : GDBRemoteCommunication(comm_name, listener_name), m_exit_now(false) {}
+    : GDBRemoteCommunication(comm_name, listener_name), m_exit_now(false) {
+  RegisterPacketHandler(
+      StringExtractorGDBRemote::eServerPacketType_QEnableErrorStrings,
+      [this](StringExtractorGDBRemote packet, Status &error, bool &interrupt,
+             bool &quit) { return this->Handle_QErrorStringEnable(packet); });
+}
 
 GDBRemoteCommunicationServer::~GDBRemoteCommunicationServer() {}
 
@@ -99,6 +105,24 @@ GDBRemoteCommunicationServer::SendErrorResponse(uint8_t err) {
   return SendPacketNoLock(llvm::StringRef(packet, packet_len));
 }
 
+GDBRemoteCommunication::PacketResult
+GDBRemoteCommunicationServer::SendErrorResponse(const Status &error) {
+  if (m_send_error_strings) {
+    lldb_private::StreamString packet;
+    packet.Printf("E%2.2x;", static_cast<uint8_t>(error.GetError()));
+    packet.PutCStringAsRawHex8(error.AsCString());
+    return SendPacketNoLock(packet.GetString());
+  } else
+    return SendErrorResponse(error.GetError());
+}
+
+GDBRemoteCommunication::PacketResult
+GDBRemoteCommunicationServer::Handle_QErrorStringEnable(
+    StringExtractorGDBRemote &packet) {
+  m_send_error_strings = true;
+  return SendOKResponse();
+}
+
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServer::SendIllFormedResponse(
     const StringExtractorGDBRemote &failed_packet, const char *message) {

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h

@@ -57,6 +57,13 @@ class GDBRemoteCommunicationServer : public GDBRemoteCommunication {
   bool m_exit_now; // use in asynchronous handling to indicate process should
                    // exit.
 
+  bool m_send_error_strings; // If the client enables this then
+                             // we will send error strings as well.
+
+  PacketResult Handle_QErrorStringEnable(StringExtractorGDBRemote &packet);
+
+  PacketResult SendErrorResponse(const Status &error);
+
   PacketResult SendUnimplementedResponse(const char *packet);
 
   PacketResult SendErrorResponse(uint8_t error);

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

@@ -1123,7 +1123,7 @@ GDBRemoteCommunicationServerLLGS::Handle_jTraceStart(
   uid = m_debugged_process_sp->StartTrace(options, error);
   LLDB_LOG(log, "uid is {0} , error is {1}", uid, error.GetError());
   if (error.Fail())
-    return SendErrorResponse(error.GetError());
+    return SendErrorResponse(error);
 
   StreamGDBRemote response;
   response.Printf("%" PRIx64, uid);
@@ -1160,7 +1160,7 @@ GDBRemoteCommunicationServerLLGS::Handle_jTraceStop(
   Status error = m_debugged_process_sp->StopTrace(uid, tid);
 
   if (error.Fail())
-    return SendErrorResponse(error.GetError());
+    return SendErrorResponse(error);
 
   return SendOKResponse();
 }
@@ -1203,7 +1203,7 @@ GDBRemoteCommunicationServerLLGS::Handle_jTraceConfigRead(
   Status error = m_debugged_process_sp->GetTraceConfig(uid, options);
 
   if (error.Fail())
-    return SendErrorResponse(error.GetError());
+    return SendErrorResponse(error);
 
   StreamGDBRemote escaped_response;
   StructuredData::Dictionary json_packet;
@@ -1279,7 +1279,7 @@ GDBRemoteCommunicationServerLLGS::Handle_jTraceRead(
     error = m_debugged_process_sp->GetMetaData(uid, tid, buf, offset);
 
   if (error.Fail())
-    return SendErrorResponse(error.GetError());
+    return SendErrorResponse(error);
 
   for (auto i : buf)
     response.PutHex8(i);

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

@@ -1031,6 +1031,7 @@ Status ProcessGDBRemote::ConnectToDebugserver(llvm::StringRef connect_url) {
   m_gdb_comm.GetHostInfo();
   m_gdb_comm.GetVContSupported('c');
   m_gdb_comm.GetVAttachOrWaitSupported();
+  m_gdb_comm.EnableErrorStringInPacket();
 
   // Ask the remote server for the default thread id
   if (GetTarget().GetNonStopModeEnabled())

lldb/source/Utility/StringExtractorGDBRemote.cpp

@@ -19,8 +19,18 @@ StringExtractorGDBRemote::GetResponseType() const {
 
   switch (m_packet[0]) {
   case 'E':
-    if (m_packet.size() == 3 && isxdigit(m_packet[1]) && isxdigit(m_packet[2]))
-      return eError;
+    if (isxdigit(m_packet[1]) && isxdigit(m_packet[2])) {
+      if (m_packet.size() == 3)
+        return eError;
+      llvm::StringRef packet_ref(m_packet);
+      if (packet_ref[3] == ';') {
+        auto err_string = packet_ref.substr(4);
+        for (auto e : err_string)
+          if (!isxdigit(e))
+            return eResponse;
+        return eError;
+      }
+    }
     break;
 
   case 'O':
@@ -86,6 +96,8 @@ StringExtractorGDBRemote::GetServerPacketType() const {
         return eServerPacketType_QEnvironment;
       if (PACKET_STARTS_WITH("QEnvironmentHexEncoded:"))
         return eServerPacketType_QEnvironmentHexEncoded;
+      if (PACKET_STARTS_WITH("QEnableErrorStrings"))
+        return eServerPacketType_QEnableErrorStrings;
       break;
 
     case 'P':
@@ -438,8 +450,8 @@ bool StringExtractorGDBRemote::IsNormalResponse() const {
 }
 
 bool StringExtractorGDBRemote::IsErrorResponse() const {
-  return GetResponseType() == eError && m_packet.size() == 3 &&
-         isxdigit(m_packet[1]) && isxdigit(m_packet[2]);
+  return GetResponseType() == eError && isxdigit(m_packet[1]) &&
+         isxdigit(m_packet[2]);
 }
 
 uint8_t StringExtractorGDBRemote::GetError() {
@@ -450,6 +462,23 @@ uint8_t StringExtractorGDBRemote::GetError() {
   return 0;
 }
 
+lldb_private::Status StringExtractorGDBRemote::GetStatus() {
+  lldb_private::Status error;
+  if (GetResponseType() == eError) {
+    SetFilePos(1);
+    uint8_t errc = GetHexU8(255);
+    error.SetError(errc, lldb::eErrorTypeGeneric);
+
+    std::string error_messg("Error ");
+    error_messg += std::to_string(errc);
+    if (GetChar() == ';')
+      GetHexByteString(error_messg);
+
+    error.SetErrorString(error_messg);
+  }
+  return error;
+}
+
 size_t StringExtractorGDBRemote::GetEscapedBinaryData(std::string &str) {
   // Just get the data bytes in the string as
   // GDBRemoteCommunication::CheckForPacket()

lldb/source/Utility/StringExtractorGDBRemote.h

@@ -10,6 +10,7 @@
 #ifndef utility_StringExtractorGDBRemote_h_
 #define utility_StringExtractorGDBRemote_h_
 
+#include "lldb/Utility/Status.h"
 #include "lldb/Utility/StringExtractor.h"
 #include "llvm/ADT/StringRef.h" // for StringRef
 
@@ -72,6 +73,7 @@ class StringExtractorGDBRemote : public StringExtractor {
     eServerPacketType_qGetWorkingDir,
     eServerPacketType_qFileLoadAddress,
     eServerPacketType_QEnvironment,
+    eServerPacketType_QEnableErrorStrings,
     eServerPacketType_QLaunchArch,
     eServerPacketType_QSetDisableASLR,
     eServerPacketType_QSetDetachOnError,
@@ -190,6 +192,8 @@ class StringExtractorGDBRemote : public StringExtractor {
   // digits. Otherwise the error encoded in XX is returned.
   uint8_t GetError();
 
+  lldb_private::Status GetStatus();
+
   size_t GetEscapedBinaryData(std::string &str);
 
 protected: