Handle O reply packets during qRcmd

[owenpshaw]

Summary

Gdb servers like openocd may send many $O reply packets for the client to output during a qRcmd command sequence. Currently, lldb interprets the first O packet as an unexpected response. Besides generating no output, this causes lldb to get out of sync with future commands because it continues reading O packets from the first command as response to subsequent commands.

This patch handles any O packets during an qRcmd, treating the first non-O packet as the true response.

Looking for guidance on how best to write test cases, haven't found anything currently testing qRcmd but may have just missed it.

Preliminary discussion at http://lists.llvm.org/pipermail/lldb-dev/2018-January/013078.html

[labath]

The ProcessGdbRemote part is not really testable in it's current form, but for the rest, you should be able to write a gdbremoteclient unittest (see unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp).

source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h

53 ↗

(On Diff #128672)

this could be an llvm::function_ref (more lightweight than std::function).

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

5157 ↗

(On Diff #128672)

I think the API would be cleaner if the output callback was an argument to the standard SendPacket function, where the (default) value of null would mean "don't do anything special with the O packets", although that might just be a personal preference.

However, even if we choose to have separate functions as the API, I believe they should delegate to a single unified implementation internally.

[owenpshaw]

Updated patch with new function name suggested by @clayborg. Added unit test and changed to llvm::function_ref as suggested by @labath.

Based on Greg's comments in the other thread, I've kept the new function separate, rather than use a flag in SendPacketAndWaitForResponse.

Is it worth creating a function to share the locking code that's common to SendPacketAndWaitForResponse and SendPacketAndReceiveResponseWithOutputSupport? Alternatively, should the lock-related error logging be unique to distinguish between the two functions?

[clayborg]

In D41745#970362, @owenpshaw wrote:

Updated patch with new function name suggested by @clayborg. Added unit test and changed to llvm::function_ref as suggested by @labath.

Looks great.

Based on Greg's comments in the other thread, I've kept the new function separate, rather than use a flag in SendPacketAndWaitForResponse.

Yeah, it doesn't seem like any other commands will us it, so this is best just so people don't think it is normal.

Is it worth creating a function to share the locking code that's common to SendPacketAndWaitForResponse and SendPacketAndReceiveResponseWithOutputSupport?

I don't think so since it is so simple.

Alternatively, should the lock-related error logging be unique to distinguish between the two functions?

I am not sure it matters since this only affects the rCmd, and the packet reception stuff is still the same.

[owenpshaw]

Thanks! I don't have commit access, so someone needs to commit this for me, right?

[labath]

Committed as r322190. I've changed one more std::function to function_ref and re-clang-formatted the patch.

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h

public:
   lldb::StateType SendContinuePacketAndWaitForResponse(
       ContinueDelegate &delegate, const UnixSignals &signals,
       llvm::StringRef payload, StringExtractorGDBRemote &response);
 
   PacketResult SendPacketAndWaitForResponse(llvm::StringRef payload,
                                             StringExtractorGDBRemote &response,
                                             bool send_async);
 
+  PacketResult SendPacketAndReceiveResponseWithOutputSupport(
+      llvm::StringRef payload, StringExtractorGDBRemote &response,
+      bool send_async,
+      llvm::function_ref<void(llvm::StringRef)> output_callback);
+
   bool SendvContPacket(llvm::StringRef payload,
                        StringExtractorGDBRemote &response);
 
   class Lock {
   public:
     Lock(GDBRemoteClientBase &comm, bool interrupt);
     ~Lock();
 

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp

if (Log *log =
                   send_async);
     return PacketResult::ErrorSendFailed;
   }
 
   return SendPacketAndWaitForResponseNoLock(payload, response);
 }
 
 GDBRemoteCommunication::PacketResult
+GDBRemoteClientBase::SendPacketAndReceiveResponseWithOutputSupport(
+    llvm::StringRef payload, StringExtractorGDBRemote &response,
+    bool send_async,
+    llvm::function_ref<void(llvm::StringRef)> output_callback) {
+  Lock lock(*this, send_async);
+  if (!lock) {
+    if (Log *log =
+            ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS))
+      log->Printf("GDBRemoteClientBase::%s failed to get mutex, not sending "
+                  "packet '%.*s' (send_async=%d)",
+                  __FUNCTION__, int(payload.size()), payload.data(),
+                  send_async);
+    return PacketResult::ErrorSendFailed;
+  }
+
+  PacketResult packet_result = SendPacketNoLock(payload);
+  if (packet_result != PacketResult::Success)
+    return packet_result;
+
+  return ReadPacketWithOutputSupport(response, GetPacketTimeout(), true,
+                                     output_callback);
+}
+
+GDBRemoteCommunication::PacketResult
 GDBRemoteClientBase::SendPacketAndWaitForResponseNoLock(
     llvm::StringRef payload, StringExtractorGDBRemote &response) {
   PacketResult packet_result = SendPacketNoLock(payload);
   if (packet_result != PacketResult::Success)
     return packet_result;
 
   const size_t max_response_retries = 3;
   for (size_t i = 0; i < max_response_retries; ++i) {

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h

protected:
 
   CompressionType m_compression_type;
 
   PacketResult SendPacketNoLock(llvm::StringRef payload);
 
   PacketResult ReadPacket(StringExtractorGDBRemote &response,
                           Timeout<std::micro> timeout, bool sync_on_timeout);
 
+  PacketResult ReadPacketWithOutputSupport(
+      StringExtractorGDBRemote &response, Timeout<std::micro> timeout,
+      bool sync_on_timeout,
+      llvm::function_ref<void(llvm::StringRef)> output_callback);
+
   // Pop a packet from the queue in a thread safe manner
   PacketResult PopPacketFromQueue(StringExtractorGDBRemote &response,
                                   Timeout<std::micro> timeout);
 
   PacketResult WaitForPacketNoLock(StringExtractorGDBRemote &response,
                                    Timeout<std::micro> timeout,
                                    bool sync_on_timeout);
 

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp

if (packet.GetResponseType() ==
       return PacketResult::Success;
     else
       return PacketResult::ErrorSendAck;
   }
   return result;
 }
 
 GDBRemoteCommunication::PacketResult
+GDBRemoteCommunication::ReadPacketWithOutputSupport(
+    StringExtractorGDBRemote &response, Timeout<std::micro> timeout,
+    bool sync_on_timeout,
+    llvm::function_ref<void(llvm::StringRef)> output_callback) {
+  auto result = ReadPacket(response, timeout, sync_on_timeout);
+  while (result == PacketResult::Success && response.IsNormalResponse() &&
+         response.PeekChar() == 'O') {
+    response.GetChar();
+    std::string output;
+    if (response.GetHexByteString(output))
+      output_callback(output);
+    result = ReadPacket(response, timeout, sync_on_timeout);
+  }
+  return result;
+}
+
+GDBRemoteCommunication::PacketResult
 GDBRemoteCommunication::ReadPacket(StringExtractorGDBRemote &response,
                                    Timeout<std::micro> timeout,
                                    bool sync_on_timeout) {
   if (m_read_thread_enabled)
     return PopPacketFromQueue(response, timeout);
   else
     return WaitForPacketNoLock(response, timeout, sync_on_timeout);
 }

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

ProcessGDBRemote *process =
         (ProcessGDBRemote *)m_interpreter.GetExecutionContext().GetProcessPtr();
     if (process) {
       StreamString packet;
       packet.PutCString("qRcmd,");
       packet.PutBytesAsRawHex8(command, strlen(command));
 
       bool send_async = true;
       StringExtractorGDBRemote response;
-      process->GetGDBRemote().SendPacketAndWaitForResponse(
-          packet.GetString(), response, send_async);
-      result.SetStatus(eReturnStatusSuccessFinishResult);
       Stream &output_strm = result.GetOutputStream();
+      process->GetGDBRemote().SendPacketAndReceiveResponseWithOutputSupport(
+          packet.GetString(), response, send_async,
+          [&output_strm](llvm::StringRef output) { output_strm << output; });
+      result.SetStatus(eReturnStatusSuccessFinishResult);
       output_strm.Printf("  packet: %s\n", packet.GetData());
       const std::string &response_str = response.GetStringRef();
 
       if (response_str.empty())
         output_strm.PutCString("response: \nerror: UNIMPLEMENTED\n");
       else
         output_strm.Printf("response: %s\n", response.GetStringRef().c_str());
     }

lldb/trunk/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp

TEST_F(GDBRemoteClientBaseTest, InterruptNoResponse) {
   // We get interrupted, but we don't send a stop packet.
   ASSERT_EQ(PacketResult::Success, server.GetPacket(response));
   ASSERT_EQ("\x03", response.GetStringRef());
 
   // The functions should still terminate (after a timeout).
   ASSERT_TRUE(async_result.get());
   ASSERT_EQ(eStateInvalid, continue_state.get());
 }
+
+TEST_F(GDBRemoteClientBaseTest, SendPacketAndReceiveResponseWithOutputSupport) {
+  StringExtractorGDBRemote response;
+  StreamString command_output;
+
+  ASSERT_EQ(PacketResult::Success, server.SendPacket("O"));
+  ASSERT_EQ(PacketResult::Success, server.SendPacket("O48656c6c6f2c"));
+  ASSERT_EQ(PacketResult::Success, server.SendPacket("O20"));
+  ASSERT_EQ(PacketResult::Success, server.SendPacket("O"));
+  ASSERT_EQ(PacketResult::Success, server.SendPacket("O776f726c64"));
+  ASSERT_EQ(PacketResult::Success, server.SendPacket("OK"));
+
+  PacketResult result = client.SendPacketAndReceiveResponseWithOutputSupport(
+      "qRcmd,test", response, true,
+      [&command_output](llvm::StringRef output) { command_output << output; });
+
+  ASSERT_EQ(PacketResult::Success, result);
+  ASSERT_EQ("OK", response.GetStringRef());
+  ASSERT_EQ("Hello, world", command_output.GetString().str());
+}