[Lldb-commits] [lldb] r356825 - [Reproducers] Fix GDB remote flakiness during replay

[Jonas Devlieghere via lldb-commits]

Author: jdevlieghere
Date: Fri Mar 22 16:33:17 2019
New Revision: 356825

URL: http://llvm.org/viewvc/llvm-project?rev=356825&view=rev
Log:
[Reproducers] Fix GDB remote flakiness during replay

This fixes the flakiness of the GDB remote reproducer during replay. It
was caused by a combination sending one ACK to many from the replay
server and the code that "flushes" any queued GDB remote packets in
GDBRemoteCommunicationClient::HandshakeWithServer.

The spurious ACK was the result of combining both implicit and explicit
handling of ACKs in the replay server. The handshake consists of an ACK
followed by an QStartNoAckMode. As long as we haven't seen any
QStartNoAckMode, we were sending implicit acknowledgments. So the first
ACK got acknowledged twice, once implicitly, and once as part of the
replay.

The reason we didn't notice this was the code in HandshakeWithServer
that "waits for any responses that might have been queued up in the
remote GDB server and flush them all". A 10ms timeout is used to move on
when no packets are left. If the second ACK didn't make it within those
10ms, all packets were offset by one.

Modified:
    lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationReplayServer.cpp

Modified: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationReplayServer.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationReplayServer.cpp?rev=356825&r1=356824&r2=356825&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationReplayServer.cpp (original)
+++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationReplayServer.cpp Fri Mar 22 16:33:17 2019
@@ -35,8 +35,6 @@ static bool unexpected(llvm::StringRef e
   // trailing checksum. The 'actual' string contains only the packet's content.
   if (expected.contains(actual))
     return false;
-  if (expected == "+" || actual == "+")
-    return false;
   // Contains a PID which might be different.
   if (expected.contains("vAttach"))
     return false;
@@ -51,11 +49,10 @@ static bool unexpected(llvm::StringRef e
 }
 
 GDBRemoteCommunicationReplayServer::GDBRemoteCommunicationReplayServer()
-    : GDBRemoteCommunication("gdb-remote.server",
-                             "gdb-remote.server.rx_packet"),
-      m_async_broadcaster(nullptr, "lldb.gdb-remote.server.async-broadcaster"),
+    : GDBRemoteCommunication("gdb-replay", "gdb-replay.rx_packet"),
+      m_async_broadcaster(nullptr, "lldb.gdb-replay.async-broadcaster"),
       m_async_listener_sp(
-          Listener::MakeListener("lldb.gdb-remote.server.async-listener")),
+          Listener::MakeListener("lldb.gdb-replay.async-listener")),
       m_async_thread_state_mutex(), m_skip_acks(false) {
   m_async_broadcaster.SetEventName(eBroadcastBitAsyncContinue,
                                    "async thread continue");
@@ -92,26 +89,21 @@ GDBRemoteCommunicationReplayServer::GetP
 
   m_async_broadcaster.BroadcastEvent(eBroadcastBitAsyncContinue);
 
-  if (m_skip_acks) {
-    const StringExtractorGDBRemote::ServerPacketType packet_type =
-        packet.GetServerPacketType();
-    switch (packet_type) {
-    case StringExtractorGDBRemote::eServerPacketType_nack:
-    case StringExtractorGDBRemote::eServerPacketType_ack:
-      return PacketResult::Success;
-    default:
-      break;
-    }
-  } else if (packet.GetStringRef() == "QStartNoAckMode") {
-    m_skip_acks = true;
+  // If m_send_acks is true, we're before the handshake phase. We've already
+  // acknowledge the '+' packet so we're done here.
+  if (m_send_acks && packet.GetStringRef() == "+")
+    return PacketResult::Success;
+
+  // This completes the handshake. Since m_send_acks was true, we can unset it
+  // already.
+  if (packet.GetStringRef() == "QStartNoAckMode")
     m_send_acks = false;
-  }
 
   // A QEnvironment packet is sent for every environment variable. If the
   // number of environment variables is different during replay, the replies
   // become out of sync.
   if (packet.GetStringRef().find("QEnvironment") == 0) {
-    return SendRawPacketNoLock("$OK#9a", true);
+    return SendRawPacketNoLock("$OK#9a");
   }
 
   Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS));
@@ -120,13 +112,17 @@ GDBRemoteCommunicationReplayServer::GetP
     GDBRemoteCommunicationHistory::Entry entry = m_packet_history.back();
     m_packet_history.pop_back();
 
+    // We're handled the handshake implicitly before. Skip the packet and move
+    // on.
+    if (entry.packet.data == "+")
+      continue;
+
     if (entry.type == GDBRemoteCommunicationHistory::ePacketTypeSend) {
       if (unexpected(entry.packet.data, packet.GetStringRef())) {
         LLDB_LOG(log,
                  "GDBRemoteCommunicationReplayServer expected packet: '{0}'",
                  entry.packet.data);
-        LLDB_LOG(log,
-                 "GDBRemoteCommunicationReplayServer actual packet: '{0}'",
+        LLDB_LOG(log, "GDBRemoteCommunicationReplayServer actual packet: '{0}'",
                  packet.GetStringRef());
       }
 
@@ -148,7 +144,10 @@ GDBRemoteCommunicationReplayServer::GetP
       continue;
     }
 
-    return SendRawPacketNoLock(entry.packet.data, true);
+    LLDB_LOG(log,
+             "GDBRemoteCommunicationReplayServer replied to '{0}' with '{1}'",
+             packet.GetStringRef(), entry.packet.data);
+    return SendRawPacketNoLock(entry.packet.data);
   }
 
   quit = true;
@@ -185,7 +184,7 @@ bool GDBRemoteCommunicationReplayServer:
     // Create a thread that watches our internal state and controls which
     // events make it to clients (into the DCProcess event queue).
     m_async_thread = ThreadLauncher::LaunchThread(
-        "<lldb.gdb-remote.server.async>",
+        "<lldb.gdb-replay.async>",
         GDBRemoteCommunicationReplayServer::AsyncThread, this, nullptr);
   }