[Lldb-commits] [lldb] r364494 - [Reproducers] Fix flakiness and off-by-one during replay.

[Pavel Labath via lldb-commits]

Do we have a test which records us interrupting a process, and then 
makes sure this replays correctly ?



On 27/06/2019 04:03, Jonas Devlieghere via lldb-commits wrote:
> Author: jdevlieghere
> Date: Wed Jun 26 19:03:34 2019
> New Revision: 364494
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=364494&view=rev
> Log:
> [Reproducers] Fix flakiness and off-by-one during replay.
> 
> This fixes two replay issues that caused the tests to behave
> erratically:
> 
> 1. It fixes an off-by-one error, where all replies where shifted by 1
>     because of a `+` packet that should've been ignored.
> 
> 2. It fixes another off-by-one-error, where an asynchronous ^C was
>     offsetting all subsequent packets. The reason is that we
>     'synchronize' requests and replies. In reality, a stop reply is only
>     sent when the process halt. During replay however, we instantly
>     report the stop, as the reply to packets like continue (vCont).
> 
> Both packets should be ignored, and indeed, checking the gdb-remote log,
> no unexpected packets are received anymore.
> 
> Additionally, be more pedantic when it comes to unexpected packets and
> return an failure form the replay server. This way we should be able to
> catch these things faster in the future.
> 
> 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=364494&r1=364493&r2=364494&view=diff
> ==============================================================================
> --- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationReplayServer.cpp (original)
> +++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationReplayServer.cpp Wed Jun 26 19:03:34 2019
> @@ -30,6 +30,7 @@ using namespace lldb;
>   using namespace lldb_private;
>   using namespace lldb_private::process_gdb_remote;
>   
> +/// Check if the given expected packet matches the actual packet.
>   static bool unexpected(llvm::StringRef expected, llvm::StringRef actual) {
>     // The 'expected' string contains the raw data, including the leading $ and
>     // trailing checksum. The 'actual' string contains only the packet's content.
> @@ -48,6 +49,25 @@ static bool unexpected(llvm::StringRef e
>     return true;
>   }
>   
> +/// Check if we should reply to the given packet.
> +static bool skip(llvm::StringRef data) {
> +  assert(!data.empty() && "Empty packet?");
> +
> +  // We've already acknowledge the '+' packet so we're done here.
> +  if (data == "+")
> +    return true;
> +
> +  /// Don't 't reply to ^C. We need this because of stop reply packets, which
> +  /// are only returned when the target halts. Reproducers synchronize these
> +  /// 'asynchronous' replies, by recording them as a regular replies to the
> +  /// previous packet (e.g. vCont). As a result, we should ignore real
> +  /// asynchronous requests.
> +  if (data.data()[0] == 0x03)
> +    return true;
> +
> +  return false;
> +}
> +
>   GDBRemoteCommunicationReplayServer::GDBRemoteCommunicationReplayServer()
>       : GDBRemoteCommunication("gdb-replay", "gdb-replay.rx_packet"),
>         m_async_broadcaster(nullptr, "lldb.gdb-replay.async-broadcaster"),
> @@ -89,9 +109,8 @@ GDBRemoteCommunicationReplayServer::GetP
>   
>     m_async_broadcaster.BroadcastEvent(eBroadcastBitAsyncContinue);
>   
> -  // 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() == "+")
> +  // Check if we should reply to this packet.
> +  if (skip(packet.GetStringRef()))
>       return PacketResult::Success;
>   
>     // This completes the handshake. Since m_send_acks was true, we can unset it
> @@ -102,9 +121,8 @@ GDBRemoteCommunicationReplayServer::GetP
>     // 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) {
> +  if (packet.GetStringRef().find("QEnvironment") == 0)
>       return SendRawPacketNoLock("$OK#9a");
> -  }
>   
>     Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS));
>     while (!m_packet_history.empty()) {
> @@ -112,7 +130,7 @@ 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
> +    // We've handled the handshake implicitly before. Skip the packet and move
>       // on.
>       if (entry.packet.data == "+")
>         continue;
> @@ -124,6 +142,7 @@ GDBRemoteCommunicationReplayServer::GetP
>                    entry.packet.data);
>           LLDB_LOG(log, "GDBRemoteCommunicationReplayServer actual packet: '{0}'",
>                    packet.GetStringRef());
> +        return PacketResult::ErrorSendFailed;
>         }
>   
>         // Ignore QEnvironment packets as they're handled earlier.
> 
> 
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>