labath created this revision.
labath added reviewers: clayborg, zturner, jingham.
labath added a subscriber: lldb-commits.
This replaces the usage of raw integers with duration classes in the gdb-remote
packet management functions. The values are still converted back to integers
once
they go into the generic Communication class -- that I am leaving to a separate
change.
The changes are mostly straight-forward (*). The thing I'd most like feedback on
is the representation of timeouts.
Currently, we use UINT32_MAX to denote infinite timeout. This is not well suited
for duration classes, as they tend to do arithmetic on the values, and the
identity of the MAX value can easily get lost (e.g.
microseconds(seconds(UINT32_MAX)).count() != UINT32_MAX). We cannot use zero to
represent infinity (as Listener classes do) because we already use it to do
non-blocking polling reads. For this reason, I believe it is better to have an
explicit value for infinity.
The way I achieved that is via llvm::Optional, and I think it reads quite
natural. Passing llvm::None as "timeout" means "no timeout", while passing zero
means "poll". The only tricky part is this breaks implicit conversions (seconds
are implicitly convertible to microseconds, but Optional<seconds> cannot be
easily converted into Optional<microseconds>). To make this less annoying I have
switched a couple of places from using seconds to microseconds to make the code
flow better.
I think other places could also benefit from the same pattern (e.g., if Listener
functions accepted an llvm::Optional timeout, then we wouldn't need to have
separate Peek*** and Get*** versions of the functions).
(*) The other tricky part was GDBRemoteCommunication::PopPacketFromQueue, which
was needlessly complicated. I've simplified it, but that one is only used in
non-stop mode, and so is untested.
https://reviews.llvm.org/D26971
Files:
source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
tools/lldb-server/lldb-platform.cpp
Index: tools/lldb-server/lldb-platform.cpp
===================================================================
--- tools/lldb-server/lldb-platform.cpp
+++ tools/lldb-server/lldb-platform.cpp
@@ -364,7 +364,7 @@
bool interrupt = false;
bool done = false;
while (!interrupt && !done) {
- if (platform.GetPacketAndSendResponse(UINT32_MAX, error, interrupt,
+ if (platform.GetPacketAndSendResponse(llvm::None, error, interrupt,
done) !=
GDBRemoteCommunication::PacketResult::Success)
break;
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===================================================================
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -926,8 +926,8 @@
bool done = false;
Error error;
while (true) {
- const PacketResult result =
- GetPacketAndSendResponse(0, error, interrupt, done);
+ const PacketResult result = GetPacketAndSendResponse(
+ std::chrono::microseconds(0), error, interrupt, done);
if (result == PacketResult::ErrorReplyTimeout)
break; // No more packets in the queue
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
===================================================================
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
@@ -43,8 +43,9 @@
RegisterPacketHandler(StringExtractorGDBRemote::ServerPacketType packet_type,
PacketHandler handler);
- PacketResult GetPacketAndSendResponse(uint32_t timeout_usec, Error &error,
- bool &interrupt, bool &quit);
+ PacketResult
+ GetPacketAndSendResponse(llvm::Optional<std::chrono::microseconds> timeout,
+ Error &error, bool &interrupt, bool &quit);
// After connecting, do a little handshake with the client to make sure
// we are at least communicating
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
===================================================================
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
@@ -38,14 +38,12 @@
}
GDBRemoteCommunication::PacketResult
-GDBRemoteCommunicationServer::GetPacketAndSendResponse(uint32_t timeout_usec,
- Error &error,
- bool &interrupt,
- bool &quit) {
+GDBRemoteCommunicationServer::GetPacketAndSendResponse(
+ llvm::Optional<std::chrono::microseconds> timeout, Error &error,
+ bool &interrupt, bool &quit) {
StringExtractorGDBRemote packet;
- PacketResult packet_result =
- WaitForPacketWithTimeoutMicroSecondsNoLock(packet, timeout_usec, false);
+ PacketResult packet_result = WaitForPacketNoLock(packet, timeout, false);
if (packet_result == PacketResult::Success) {
const StringExtractorGDBRemote::ServerPacketType packet_type =
packet.GetServerPacketType();
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===================================================================
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -122,9 +122,9 @@
// GDB server and flush them all
StringExtractorGDBRemote response;
PacketResult packet_result = PacketResult::Success;
- const uint32_t timeout_usec = 10 * 1000; // Wait for 10 ms for a response
+ const std::chrono::microseconds timeout(10000); // 10 ms
while (packet_result == PacketResult::Success)
- packet_result = ReadPacket(response, timeout_usec, false);
+ packet_result = ReadPacket(response, timeout, false);
// The return value from QueryNoAckModeSupported() is true if the packet
// was sent and _any_ response (including UNIMPLEMENTED) was received),
@@ -202,8 +202,8 @@
// longer than normal to receive a reply. Wait at least 6 seconds for a
// reply to this packet.
- ScopedTimeout timeout(
- *this, std::max(GetPacketTimeout(), std::chrono::seconds(6)));
+ ScopedTimeout timeout(*this, std::max(GetPacketTimeout(),
+ std::chrono::microseconds(6000000)));
StringExtractorGDBRemote response;
if (SendPacketAndWaitForResponse("QStartNoAckMode", response, false) ==
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
===================================================================
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
@@ -83,12 +83,12 @@
class ScopedTimeout {
public:
ScopedTimeout(GDBRemoteCommunication &gdb_comm,
- std::chrono::seconds timeout);
+ std::chrono::microseconds timeout);
~ScopedTimeout();
private:
GDBRemoteCommunication &m_gdb_comm;
- std::chrono::seconds m_saved_timeout;
+ std::chrono::microseconds m_saved_timeout;
};
GDBRemoteCommunication(const char *comm_name, const char *listener_name);
@@ -115,13 +115,16 @@
// packets and waiting for responses. For servers, this is used when waiting
// for ACKs.
//------------------------------------------------------------------
- std::chrono::seconds SetPacketTimeout(std::chrono::seconds packet_timeout) {
+ std::chrono::microseconds
+ SetPacketTimeout(std::chrono::microseconds packet_timeout) {
const auto old_packet_timeout = m_packet_timeout;
m_packet_timeout = packet_timeout;
return old_packet_timeout;
}
- std::chrono::seconds GetPacketTimeout() const { return m_packet_timeout; }
+ std::chrono::microseconds GetPacketTimeout() const {
+ return m_packet_timeout;
+ }
//------------------------------------------------------------------
// Start a debugserver instance on the current host using the
@@ -212,7 +215,7 @@
mutable bool m_dumped_to_log;
};
- std::chrono::seconds m_packet_timeout;
+ std::chrono::microseconds m_packet_timeout;
uint32_t m_echo_number;
LazyBool m_supports_qEcho;
History m_history;
@@ -226,16 +229,18 @@
PacketResult SendPacketNoLock(llvm::StringRef payload);
PacketResult ReadPacket(StringExtractorGDBRemote &response,
- uint32_t timeout_usec, bool sync_on_timeout);
+ llvm::Optional<std::chrono::microseconds> timeout,
+ bool sync_on_timeout);
// Pop a packet from the queue in a thread safe manner
- PacketResult PopPacketFromQueue(StringExtractorGDBRemote &response,
- uint32_t timeout_usec);
+ PacketResult
+ PopPacketFromQueue(StringExtractorGDBRemote &response,
+ llvm::Optional<std::chrono::microseconds> timeout);
PacketResult
- WaitForPacketWithTimeoutMicroSecondsNoLock(StringExtractorGDBRemote &response,
- uint32_t timeout_usec,
- bool sync_on_timeout);
+ WaitForPacketNoLock(StringExtractorGDBRemote &response,
+ llvm::Optional<std::chrono::microseconds> timeout,
+ bool sync_on_timeout);
bool CompressionIsEnabled() {
return m_compression_type != CompressionType::None;
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
===================================================================
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -133,9 +133,9 @@
const char *listener_name)
: Communication(comm_name),
#ifdef LLDB_CONFIGURATION_DEBUG
- m_packet_timeout(1000),
+ m_packet_timeout(1000000000),
#else
- m_packet_timeout(1),
+ m_packet_timeout(1000000),
#endif
m_echo_number(0), m_supports_qEcho(eLazyBoolCalculate), m_history(512),
m_send_acks(true), m_compression_type(CompressionType::None),
@@ -263,11 +263,7 @@
GDBRemoteCommunication::PacketResult GDBRemoteCommunication::GetAck() {
StringExtractorGDBRemote packet;
- PacketResult result = ReadPacket(
- packet,
- std::chrono::duration_cast<std::chrono::microseconds>(GetPacketTimeout())
- .count(),
- false);
+ PacketResult result = ReadPacket(packet, GetPacketTimeout(), false);
if (result == PacketResult::Success) {
if (packet.GetResponseType() ==
StringExtractorGDBRemote::ResponseType::eAck)
@@ -278,67 +274,49 @@
return result;
}
-GDBRemoteCommunication::PacketResult
-GDBRemoteCommunication::ReadPacket(StringExtractorGDBRemote &response,
- uint32_t timeout_usec,
- bool sync_on_timeout) {
+GDBRemoteCommunication::PacketResult GDBRemoteCommunication::ReadPacket(
+ StringExtractorGDBRemote &response,
+ llvm::Optional<std::chrono::microseconds> timeout, bool sync_on_timeout) {
if (m_read_thread_enabled)
- return PopPacketFromQueue(response, timeout_usec);
+ return PopPacketFromQueue(response, timeout);
else
- return WaitForPacketWithTimeoutMicroSecondsNoLock(response, timeout_usec,
- sync_on_timeout);
+ return WaitForPacketNoLock(response, timeout, sync_on_timeout);
}
// This function is called when a packet is requested.
// A whole packet is popped from the packet queue and returned to the caller.
// Packets are placed into this queue from the communication read thread.
// See GDBRemoteCommunication::AppendBytesToCache.
-GDBRemoteCommunication::PacketResult
-GDBRemoteCommunication::PopPacketFromQueue(StringExtractorGDBRemote &response,
- uint32_t timeout_usec) {
- auto until = std::chrono::system_clock::now() +
- std::chrono::microseconds(timeout_usec);
-
- while (true) {
- // scope for the mutex
- {
- // lock down the packet queue
- std::unique_lock<std::mutex> lock(m_packet_queue_mutex);
-
- // Wait on condition variable.
- if (m_packet_queue.size() == 0) {
- std::cv_status result =
- m_condition_queue_not_empty.wait_until(lock, until);
- if (result == std::cv_status::timeout)
- break;
- }
-
- if (m_packet_queue.size() > 0) {
- // get the front element of the queue
- response = m_packet_queue.front();
-
- // remove the front element
- m_packet_queue.pop();
-
- // we got a packet
- return PacketResult::Success;
- }
- }
-
- // Disconnected
+GDBRemoteCommunication::PacketResult GDBRemoteCommunication::PopPacketFromQueue(
+ StringExtractorGDBRemote &response,
+ llvm::Optional<std::chrono::microseconds> timeout) {
+ auto pred = [&] { return !m_packet_queue.empty() && IsConnected(); };
+ // lock down the packet queue
+ std::unique_lock<std::mutex> lock(m_packet_queue_mutex);
+
+ if (!timeout)
+ m_condition_queue_not_empty.wait(lock, pred);
+ else {
+ if (!m_condition_queue_not_empty.wait_for(lock, *timeout, pred))
+ return PacketResult::ErrorReplyTimeout;
if (!IsConnected())
return PacketResult::ErrorDisconnected;
-
- // Loop while not timed out
}
- return PacketResult::ErrorReplyTimeout;
+ // get the front element of the queue
+ response = m_packet_queue.front();
+
+ // remove the front element
+ m_packet_queue.pop();
+
+ // we got a packet
+ return PacketResult::Success;
}
GDBRemoteCommunication::PacketResult
-GDBRemoteCommunication::WaitForPacketWithTimeoutMicroSecondsNoLock(
- StringExtractorGDBRemote &packet, uint32_t timeout_usec,
- bool sync_on_timeout) {
+GDBRemoteCommunication::WaitForPacketNoLock(
+ StringExtractorGDBRemote &packet,
+ llvm::Optional<std::chrono::microseconds> timeout, bool sync_on_timeout) {
uint8_t buffer[8192];
Error error;
@@ -354,12 +332,13 @@
while (IsConnected() && !timed_out) {
lldb::ConnectionStatus status = eConnectionStatusNoConnection;
size_t bytes_read =
- Read(buffer, sizeof(buffer), timeout_usec, status, &error);
+ Read(buffer, sizeof(buffer), timeout ? timeout->count() : UINT32_MAX,
+ status, &error);
if (log)
- log->Printf("%s: Read (buffer, (sizeof(buffer), timeout_usec = 0x%x, "
+ log->Printf("%s: Read (buffer, (sizeof(buffer), timeout = %ld us, "
"status = %s, error = %s) => bytes_read = %" PRIu64,
- LLVM_PRETTY_FUNCTION, timeout_usec,
+ LLVM_PRETTY_FUNCTION, long(timeout ? timeout->count() : -1),
Communication::ConnectionStatusAsCString(status),
error.AsCString(), (uint64_t)bytes_read);
@@ -422,8 +401,8 @@
uint32_t successful_responses = 0;
for (uint32_t i = 0; i < max_retries; ++i) {
StringExtractorGDBRemote echo_response;
- echo_packet_result = WaitForPacketWithTimeoutMicroSecondsNoLock(
- echo_response, timeout_usec, false);
+ echo_packet_result =
+ WaitForPacketNoLock(echo_response, timeout, false);
if (echo_packet_result == PacketResult::Success) {
++successful_responses;
if (response_regex.Execute(echo_response.GetStringRef())) {
@@ -1324,7 +1303,7 @@
void GDBRemoteCommunication::DumpHistory(Stream &strm) { m_history.Dump(strm); }
GDBRemoteCommunication::ScopedTimeout::ScopedTimeout(
- GDBRemoteCommunication &gdb_comm, std::chrono::seconds timeout)
+ GDBRemoteCommunication &gdb_comm, std::chrono::microseconds timeout)
: m_gdb_comm(gdb_comm) {
m_saved_timeout = m_gdb_comm.SetPacketTimeout(timeout);
}
Index: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
===================================================================
--- source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
+++ source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
@@ -21,7 +21,7 @@
using namespace lldb_private;
using namespace lldb_private::process_gdb_remote;
-static const std::chrono::seconds kInterruptTimeout(5);
+static const std::chrono::microseconds kInterruptTimeout(5000000); // 5s
/////////////////////////
// GDBRemoteClientBase //
@@ -51,11 +51,7 @@
OnRunPacketSent(true);
for (;;) {
- PacketResult read_result = ReadPacket(
- response,
- std::chrono::duration_cast<std::chrono::microseconds>(kInterruptTimeout)
- .count(),
- false);
+ PacketResult read_result = ReadPacket(response, kInterruptTimeout, false);
switch (read_result) {
case PacketResult::ErrorReplyTimeout: {
std::lock_guard<std::mutex> lock(m_mutex);
@@ -188,11 +184,7 @@
const size_t max_response_retries = 3;
for (size_t i = 0; i < max_response_retries; ++i) {
- packet_result = ReadPacket(
- response, std::chrono::duration_cast<std::chrono::microseconds>(
- GetPacketTimeout())
- .count(),
- true);
+ packet_result = ReadPacket(response, GetPacketTimeout(), true);
// Make sure we received a response
if (packet_result != PacketResult::Success)
return packet_result;
@@ -232,7 +224,7 @@
OnRunPacketSent(true);
// wait for the response to the vCont
- if (ReadPacket(response, UINT32_MAX, false) == PacketResult::Success) {
+ if (ReadPacket(response, llvm::None, false) == PacketResult::Success) {
if (response.IsOKResponse())
return true;
}
@@ -254,8 +246,8 @@
// additional packet to make sure the packet sequence does not get
// skewed.
StringExtractorGDBRemote extra_stop_reply_packet;
- uint32_t timeout_usec = 100000; // 100ms
- ReadPacket(extra_stop_reply_packet, timeout_usec, false);
+ std::chrono::microseconds timeout(100000); // 100ms
+ ReadPacket(extra_stop_reply_packet, timeout, false);
// Interrupting is typically done using SIGSTOP or SIGINT, so if
// the process stops with some other signal, we definitely want to
@@ -268,11 +260,9 @@
// We probably only stopped to perform some async processing, so continue
// after that is done.
// TODO: This is not 100% correct, as the process may have been stopped with
- // SIGINT or SIGSTOP
- // that was not caused by us (e.g. raise(SIGINT)). This will normally cause a
- // stop, but if it's
- // done concurrently with a async interrupt, that stop will get eaten
- // (llvm.org/pr20231).
+ // SIGINT or SIGSTOP that was not caused by us (e.g. raise(SIGINT)). This will
+ // normally cause a stop, but if it's done concurrently with a async
+ // interrupt, that stop will get eaten (llvm.org/pr20231).
return false;
}
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
