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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3203
+        error.SetError(response.GetError(), eErrorTypeGeneric);
+      LLDB_LOG(log, "Target does not support Tracing , error {0}", error.AsCString());
     } else {
----------------
AsCString unnecessary


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3332
     } else {
-      error.SetError(response.GetError(), eErrorTypeGeneric);
+      if (GetErrorStringInPacketSupported())
+        error = response.GetStatus();
----------------
I don't think every packet function should need to write these four lines of code. This should be abstracted away. Ideally I'd like to see this as a method on the response class (i.e., simply `error = response.GetStatus()`), but if it's hard to plumb that information into the class cleanly, I can imagine it being a method on the communication class as well (`error = this->GetStatus(response)`)

update: It looks like the GetStatus function already supports that: Why not just write this as `error = response.GetStatus()` ???


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp:104
+GDBRemoteCommunicationServer::SendErrorResponse(Status &error) {
+  char errc[16];
+  int errc_len = ::snprintf(errc, sizeof(errc), "E%2.2x", error.GetError());
----------------
Condensed version without dodgy C functions:
`return SendPacketNoLock(llvm::formatv("E{0:x-2};{1}", error.GetError(), error).str());`


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp:110
+  packet += ";";
+  packet += std::string(error.AsCString());
+  return SendPacketNoLock(packet);
----------------
Aren't you supposed to send these only if the client enabled the error response?


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h:64
 
+  PacketResult SendErrorResponse(Status &error);
+
----------------
Why the reference?


================
Comment at: source/Utility/StringExtractorGDBRemote.cpp:463
+
+    std::string error_messg ("Error 47");
+    size_t str_index = m_packet.find(';', m_index);
----------------
replace 47 with the actual error code


https://reviews.llvm.org/D34945