Author: gclayton
Date: Thu Mar 31 19:41:29 2016
New Revision: 265086
URL: http://llvm.org/viewvc/llvm-project?rev=265086&view=rev
Log:
Fixed an issue that could cause debugserver to return two stop reply packets
($T packets) for one \x03 interrupt. The problem was that when a \x03 byte is
sent to debugserver while the process is running, and up calling:
rnb_err_t
RNBRemote::HandlePacket_stop_process (const char *p)
{
if (!DNBProcessInterrupt(m_ctx.ProcessID()))
HandlePacket_last_signal (NULL);
return rnb_success;
}
In the call to DNBProcessInterrupt we did:
nub_bool_t
DNBProcessInterrupt(nub_process_t pid)
{
MachProcessSP procSP;
if (GetProcessSP (pid, procSP))
return procSP->Interrupt();
return false;
}
This would always return false. It would cause HandlePacket_stop_process to
always call "HandlePacket_last_signal (NULL);" which would send an extra stop
reply packet _if_ the process is stopped. On a machine with enough cores, it
would call DNBProcessInterrupt(...) and then HandlePacket_last_signal(NULL) so
quickly that it will never send out an extra stop reply packet. But if the
machine is slow enough or doesn't have enough cores, it could cause the call to
HandlePacket_last_signal() to actually succeed and send an extra stop reply
packet. This would cause problems up in
GDBRemoteCommunicationClient::SendContinuePacketAndWaitForResponse() where it
would get the first stop reply packet and then possibly return or execute an
async packet. If it returned, then the next packet that was sent will get the
second stop reply as its response. If it executes an async packet, the async
packet will get the wrong response.
To fix this I did the following:
1 - in debugserver, I fixed "bool MachProcess::Interrupt()" to return true if
it sends the signal so we avoid sending the stop reply twice on slower machines
2 - Added a log line to RNBRemote::HandlePacket_stop_process() to say if we
ever send an extra stop reply so we will see this in the darwin console output
if this does happen
3 - Added response validators to StringExtractorGDBRemote so that we can verify
some responses to some packets.
4 - Added validators to packets that often follow stop reply packets like the
"m" packet for memory reads, JSON packets since "jThreadsInfo" is often sent
immediately following a stop reply.
5 - Modified GDBRemoteCommunicationClient::SendPacketAndWaitForResponseNoLock()
to validate responses. Any "StringExtractorGDBRemote &response" that contains a
valid response verifier will verify the response and keep looking for correct
responses up to 3 times. This will help us get back on track if we do get extra
stop replies. If a StringExtractorGDBRemote does not have a response validator,
it will accept any packet in response.
6 - In GDBRemoteCommunicationClient::SendPacketAndWaitForResponse we copy the
response validator from the "response" argument over into m_async_response so
that if we send the packet by interrupting the running process, we can validate
the response we actually get in
GDBRemoteCommunicationClient::SendContinuePacketAndWaitForResponse()
7 - Modified
GDBRemoteCommunicationClient::SendContinuePacketAndWaitForResponse() to always
check for an extra stop reply packet for 100ms when the process is interrupted.
We were already doing this because we might interrupt a process with a \x03
packet, yet the process was in the process of stopping due to another reason.
This race condition could cause an extra stop reply packet because the GDB
remote protocol says if a \x03 packet is sent while the process is stopped, we
should send a stop reply packet back. Now we always check for an extra stop
reply packet when we manually interrupt a process.
The issue was showing up when our IDE would attempt to set a breakpoint while
the process is running and this would happen:
--> \x03
<-- $T<stop reply 1>
--> z0,AAAAA,BB (set breakpoint)
<-- $T<stop reply 1> (incorrect extra stop reply packet)
--> c
<-- OK (response from z0 packet)
Now all packet traffic was off by one response. Since we now have a validator
on the response for "z" packets, we do this:
--> \x03
<-- $T<stop reply 1>
--> z0,AAAAA,BB (set breakpoint)
<-- $T<stop reply 1> (Ignore this because this can't be the response to z0
packets)
<-- OK -- (we are back on track as this is a valid response to z0)
...
As time goes on we should add more packet validators.
<rdar://problem/22859505>
Modified:
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/trunk/source/Utility/StringExtractorGDBRemote.cpp
lldb/trunk/source/Utility/StringExtractorGDBRemote.h
lldb/trunk/tools/debugserver/source/MacOSX/MachProcess.mm
lldb/trunk/tools/debugserver/source/RNBRemote.cpp
Modified:
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp?rev=265086&r1=265085&r2=265086&view=diff
==============================================================================
---
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
(original)
+++
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
Thu Mar 31 19:41:29 2016
@@ -623,6 +623,7 @@ GDBRemoteCommunicationClient::GetThreads
if (m_supports_jThreadsInfo)
{
StringExtractorGDBRemote response;
+ response.SetResponseValidatorToJSON();
if (SendPacketAndWaitForResponse("jThreadsInfo", response, false) ==
PacketResult::Success)
{
if (response.IsUnsupportedResponse())
@@ -765,9 +766,29 @@ GDBRemoteCommunicationClient::SendPacket
size_t
payload_length,
StringExtractorGDBRemote &response)
{
- PacketResult packet_result = SendPacketNoLock (payload, payload_length);
+ PacketResult packet_result = SendPacketNoLock(payload, payload_length);
if (packet_result == PacketResult::Success)
- packet_result = ReadPacket (response, GetPacketTimeoutInMicroSeconds
(), true);
+ {
+ const size_t max_response_retries = 3;
+ for (size_t i=0; i<max_response_retries; ++i)
+ {
+ packet_result = ReadPacket(response,
GetPacketTimeoutInMicroSeconds (), true);
+ // Make sure we received a response
+ if (packet_result != PacketResult::Success)
+ return packet_result;
+ // Make sure our response is valid for the payload that was sent
+ if (response.ValidateResponse())
+ return packet_result;
+ // Response says it wasn't valid
+ Log *log =
ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PACKETS);
+ if (log)
+ log->Printf("error: packet with payload \"%*s\" got invalid
response \"%s\": %s",
+ (int)payload_length,
+ payload,
+ response.GetStringRef().c_str(),
+ (i == (max_response_retries - 1)) ? "using invalid
response and giving up" : "ignoring response and waiting for another");
+ }
+ }
return packet_result;
}
@@ -801,6 +822,7 @@ GDBRemoteCommunicationClient::SendPacket
{
Mutex::Locker async_locker (m_async_mutex);
m_async_packet.assign(payload, payload_length);
+ m_async_response.CopyResponseValidator(response);
m_async_packet_predicate.SetValue (true, eBroadcastNever);
if (log)
@@ -867,6 +889,9 @@ GDBRemoteCommunicationClient::SendPacket
if (log)
log->Printf ("async: failed to interrupt");
}
+
+ m_async_response.SetResponseValidator(nullptr, nullptr);
+
}
else
{
@@ -1136,8 +1161,11 @@ GDBRemoteCommunicationClient::SendContin
// which will just re-send a copy of the last stop
reply
// packet. If we don't do this, then the reply for
our
// async packet will be the repeat stop reply
packet and cause
- // a lot of trouble for us!
- if (signo != sigint_signo && signo !=
sigstop_signo)
+ // a lot of trouble for us! We also have some
debugserver
+ // binaries that would send two stop replies
anytime the process
+ // was interrupted, so we need to also check for
an extra
+ // stop reply packet if we interrupted the process
+ if (m_interrupt_sent || (signo != sigint_signo &&
signo != sigstop_signo))
{
continue_after_async = false;
@@ -3572,6 +3600,8 @@ GDBRemoteCommunicationClient::SendGDBSto
// Check we haven't overwritten the end of the packet buffer
assert (packet_len + 1 < (int)sizeof(packet));
StringExtractorGDBRemote response;
+ // Make sure the response is either "OK", "EXX" where XX are two hex
digits, or "" (unsupported)
+ response.SetResponseValidatorToOKErrorNotSupported();
// Try to send the breakpoint packet, and check that it was correctly sent
if (SendPacketAndWaitForResponse(packet, packet_len, response, true) ==
PacketResult::Success)
{
Modified: lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp?rev=265086&r1=265085&r2=265086&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (original)
+++ lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp Thu Mar
31 19:41:29 2016
@@ -4170,6 +4170,7 @@ ProcessGDBRemote::GetExtendedInfoForThre
packet << (char) (0x7d ^ 0x20);
StringExtractorGDBRemote response;
+ response.SetResponseValidatorToJSON();
if (m_gdb_comm.SendPacketAndWaitForResponse(packet.GetData(),
packet.GetSize(), response, false) ==
GDBRemoteCommunication::PacketResult::Success)
{
StringExtractorGDBRemote::ResponseType response_type =
response.GetResponseType();
@@ -4211,6 +4212,7 @@ ProcessGDBRemote::GetLoadedDynamicLibrar
packet << (char) (0x7d ^ 0x20);
StringExtractorGDBRemote response;
+ response.SetResponseValidatorToJSON();
if (m_gdb_comm.SendPacketAndWaitForResponse(packet.GetData(),
packet.GetSize(), response, false) ==
GDBRemoteCommunication::PacketResult::Success)
{
StringExtractorGDBRemote::ResponseType response_type =
response.GetResponseType();
Modified: lldb/trunk/source/Utility/StringExtractorGDBRemote.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Utility/StringExtractorGDBRemote.cpp?rev=265086&r1=265085&r2=265086&view=diff
==============================================================================
--- lldb/trunk/source/Utility/StringExtractorGDBRemote.cpp (original)
+++ lldb/trunk/source/Utility/StringExtractorGDBRemote.cpp Thu Mar 31 19:41:29
2016
@@ -14,8 +14,7 @@
// Other libraries and framework includes
// Project includes
#include "Utility/StringExtractorGDBRemote.h"
-
-
+#include "lldb/Utility/LLDBAssert.h"
StringExtractorGDBRemote::ResponseType
StringExtractorGDBRemote::GetResponseType () const
@@ -391,3 +390,136 @@ StringExtractorGDBRemote::GetEscapedBina
return str.size();
}
+static bool
+OKErrorNotSupportedResponseValidator(void *, const StringExtractorGDBRemote
&response)
+{
+ switch (response.GetResponseType())
+ {
+ case StringExtractorGDBRemote::eOK:
+ case StringExtractorGDBRemote::eError:
+ case StringExtractorGDBRemote::eUnsupported:
+ return true;
+
+ case StringExtractorGDBRemote::eAck:
+ case StringExtractorGDBRemote::eNack:
+ case StringExtractorGDBRemote::eResponse:
+ break;
+ }
+ lldbassert(!"Packet validatation failed, check why this is happening");
+ return false;
+}
+
+static bool
+JSONResponseValidator(void *, const StringExtractorGDBRemote &response)
+{
+ switch (response.GetResponseType())
+ {
+ case StringExtractorGDBRemote::eUnsupported:
+ case StringExtractorGDBRemote::eError:
+ return true; // Accept unsupported or EXX as valid responses
+
+ case StringExtractorGDBRemote::eOK:
+ case StringExtractorGDBRemote::eAck:
+ case StringExtractorGDBRemote::eNack:
+ break;
+
+ case StringExtractorGDBRemote::eResponse:
+ // JSON that is returned in from JSON query packets is currently
always
+ // either a dictionary which starts with a '{', or an array which
+ // starts with a '['. This is a quick validator to just make sure
the
+ // response could be valid JSON without having to validate all of
the
+ // JSON content.
+ switch (response.GetStringRef()[0])
+ {
+ case '{': return true;
+ case '[': return true;
+ default:
+ break;
+ }
+ break;
+ }
+ lldbassert(!"Packet validatation failed, check why this is happening");
+ return false;
+}
+
+static bool
+ASCIIHexBytesResponseValidator(void *, const StringExtractorGDBRemote
&response)
+{
+ switch (response.GetResponseType())
+ {
+ case StringExtractorGDBRemote::eUnsupported:
+ case StringExtractorGDBRemote::eError:
+ return true; // Accept unsupported or EXX as valid responses
+
+ case StringExtractorGDBRemote::eOK:
+ case StringExtractorGDBRemote::eAck:
+ case StringExtractorGDBRemote::eNack:
+ break;
+
+ case StringExtractorGDBRemote::eResponse:
+ {
+ uint32_t valid_count = 0;
+ for (const char ch : response.GetStringRef())
+ {
+ if (!isxdigit(ch))
+ {
+ lldbassert(!"Packet validatation failed, check why
this is happening");
+ return false;
+ }
+ if (++valid_count >= 16)
+ break; // Don't validate all the characters in case
the packet is very large
+ }
+ return true;
+ }
+ break;
+ }
+ lldbassert(!"Packet validatation failed, check why this is happening");
+ return false;
+}
+
+void
+StringExtractorGDBRemote::CopyResponseValidator(const
StringExtractorGDBRemote& rhs)
+{
+ m_validator = rhs.m_validator;
+ m_validator_baton = rhs.m_validator_baton;
+}
+
+void
+StringExtractorGDBRemote::SetResponseValidator(ResponseValidatorCallback
callback, void *baton)
+{
+ m_validator = callback;
+ m_validator_baton = baton;
+}
+
+void
+StringExtractorGDBRemote::SetResponseValidatorToOKErrorNotSupported()
+{
+ m_validator = OKErrorNotSupportedResponseValidator;
+ m_validator_baton = nullptr;
+}
+
+void
+StringExtractorGDBRemote::SetResponseValidatorToASCIIHexBytes()
+{
+ m_validator = ASCIIHexBytesResponseValidator;
+ m_validator_baton = nullptr;
+}
+
+void
+StringExtractorGDBRemote::SetResponseValidatorToJSON()
+{
+ m_validator = JSONResponseValidator;
+ m_validator_baton = nullptr;
+}
+
+bool
+StringExtractorGDBRemote::ValidateResponse() const
+{
+ // If we have a validator callback, try to validate the callback
+ if (m_validator)
+ return m_validator(m_validator_baton, *this);
+ else
+ return true; // No validator, so response is valid
+}
+
+
Modified: lldb/trunk/source/Utility/StringExtractorGDBRemote.h
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Utility/StringExtractorGDBRemote.h?rev=265086&r1=265085&r2=265086&view=diff
==============================================================================
--- lldb/trunk/source/Utility/StringExtractorGDBRemote.h (original)
+++ lldb/trunk/source/Utility/StringExtractorGDBRemote.h Thu Mar 31 19:41:29
2016
@@ -20,18 +20,23 @@
class StringExtractorGDBRemote : public StringExtractor
{
public:
+ typedef bool (*ResponseValidatorCallback)(void * baton, const
StringExtractorGDBRemote &response);
StringExtractorGDBRemote() :
- StringExtractor ()
+ StringExtractor(),
+ m_validator(nullptr)
{
}
StringExtractorGDBRemote(const char *cstr) :
- StringExtractor (cstr)
+ StringExtractor(cstr),
+ m_validator(nullptr)
{
}
+
StringExtractorGDBRemote(const StringExtractorGDBRemote& rhs) :
- StringExtractor (rhs)
+ StringExtractor(rhs),
+ m_validator(rhs.m_validator)
{
}
@@ -39,6 +44,24 @@ public:
{
}
+ bool
+ ValidateResponse() const;
+
+ void
+ CopyResponseValidator(const StringExtractorGDBRemote& rhs);
+
+ void
+ SetResponseValidator(ResponseValidatorCallback callback, void *baton);
+
+ void
+ SetResponseValidatorToOKErrorNotSupported();
+
+ void
+ SetResponseValidatorToASCIIHexBytes();
+
+ void
+ SetResponseValidatorToJSON();
+
enum ServerPacketType
{
eServerPacketType_nack = 0,
@@ -192,6 +215,9 @@ public:
size_t
GetEscapedBinaryData (std::string &str);
+protected:
+ ResponseValidatorCallback m_validator;
+ void *m_validator_baton;
};
#endif // utility_StringExtractorGDBRemote_h_
Modified: lldb/trunk/tools/debugserver/source/MacOSX/MachProcess.mm
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/debugserver/source/MacOSX/MachProcess.mm?rev=265086&r1=265085&r2=265086&view=diff
==============================================================================
--- lldb/trunk/tools/debugserver/source/MacOSX/MachProcess.mm (original)
+++ lldb/trunk/tools/debugserver/source/MacOSX/MachProcess.mm Thu Mar 31
19:41:29 2016
@@ -1048,6 +1048,7 @@ MachProcess::Interrupt()
if (Signal (m_sent_interrupt_signo))
{
DNBLogThreadedIf(LOG_PROCESS, "MachProcess::Interrupt() - sent
%i signal to interrupt process", m_sent_interrupt_signo);
+ return true;
}
else
{
Modified: lldb/trunk/tools/debugserver/source/RNBRemote.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/debugserver/source/RNBRemote.cpp?rev=265086&r1=265085&r2=265086&view=diff
==============================================================================
--- lldb/trunk/tools/debugserver/source/RNBRemote.cpp (original)
+++ lldb/trunk/tools/debugserver/source/RNBRemote.cpp Thu Mar 31 19:41:29 2016
@@ -4433,6 +4433,7 @@ RNBRemote::HandlePacket_stop_process (co
{
// If we failed to interrupt the process, then send a stop
// reply packet as the process was probably already stopped
+ DNBLogThreaded ("RNBRemote::HandlePacket_stop_process() sending extra
stop reply because DNBProcessInterrupt returned false");
HandlePacket_last_signal (NULL);
}
return rnb_success;
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
