[Lldb-commits] [PATCH] D34945: Adding Support for Error Strings in Remote Packets

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

See inlined comments.



================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3176
 
+  EnableErrorStringInPacket();
   StreamGDBRemote escaped_packet;
----------------
I would move this into ProcessGDBRemote::ConnectToDebugserver() where many one time things happen. We should enable this once at the beginning and then deal with the error messages always possibly being present. The code in ProcessGDBRemote::ConnectToDebugserver() where you would insert:

```
  m_gdb_comm.GetEchoSupported();
  m_gdb_comm.GetThreadSuffixSupported();
  m_gdb_comm.GetListThreadsInStopReplySupported();
  m_gdb_comm.GetHostInfo();
  m_gdb_comm.GetVContSupported('c');
  m_gdb_comm.GetVAttachOrWaitSupported();
  m_gdb_comm.EnableErrorStringInPacket(); /// <<< HERE
```


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3221
 
+  EnableErrorStringInPacket();
   StructuredData::Dictionary json_packet;
----------------
remove


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3255
     llvm::MutableArrayRef<uint8_t> &buffer, size_t offset) {
+  EnableErrorStringInPacket();
   StreamGDBRemote escaped_packet;
----------------
remove


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3264
     llvm::MutableArrayRef<uint8_t> &buffer, size_t offset) {
+  EnableErrorStringInPacket();
   StreamGDBRemote escaped_packet;
----------------
remove


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3277
 
+  EnableErrorStringInPacket();
   StreamString json_string;
----------------
remove


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3348
 
+  EnableErrorStringInPacket();
   StructuredData::Dictionary json_packet;
----------------
remove


================
Comment at: source/Utility/StringExtractorGDBRemote.cpp:22
   case 'E':
-    if (m_packet.size() == 3 && isxdigit(m_packet[1]) && isxdigit(m_packet[2]))
+    if (isxdigit(m_packet[1]) && isxdigit(m_packet[2]))
       return eError;
----------------
We should probably be a bit more specific with the check to check for the ';' or NULL terminator at index 3:

```
if (isxdigit(m_packet[1]) && isxdigit(m_packet[2])) {
  if (m_packet.size() == 3)
    return eError; 
  if (m_packet[3] == ';') {
    if (verify all remaining bytes are valid HEX ASCII bytes)
      return eError; 
  }
}
```

We don't want some random string packet response like "Each time I do" to make this the response claim to be an error response.


================
Comment at: source/Utility/StringExtractorGDBRemote.cpp:467
+    if (str_index != std::string::npos)
+      error_messg = m_packet.substr(++str_index);
+
----------------
hex encode


https://reviews.llvm.org/D34945