labath updated this revision to Diff 78895.
labath added a comment.
A new version, which uses a helper class which enables all the implicit
conversions that one would normally expect from the duration classes.
Things left TBD:
- name
- where to put it
- whether inheriting from Optional is fine (it is slightly dodgy, but it
reduces boilerplate).
- using namespace std::chrono, where it makes sense.
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(OptionalDur<std::micro> 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(
+ OptionalDur<std::micro> 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::milliseconds timeout(10);
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),
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
===================================================================
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
@@ -53,6 +53,30 @@
class ProcessGDBRemote;
+template <typename Ratio>
+class OptionalDur : public llvm::Optional<std::chrono::duration<int64_t, Ratio>> {
+private:
+ template<typename Ratio2>
+ using Dur = std::chrono::duration<int64_t, Ratio2>;
+ template <typename Ratio2>
+ using EnableIf = std::enable_if<
+ std::is_convertible<std::chrono::duration<int64_t, Ratio2>,
+ std::chrono::duration<int64_t, Ratio>>::value>;
+
+ using Base = llvm::Optional<Dur<Ratio>>;
+
+public:
+ OptionalDur(llvm::NoneType none) : Base(none) {}
+ OptionalDur(const OptionalDur &other) = default;
+
+ template <typename Ratio2, typename = typename EnableIf<Ratio2>::type>
+ OptionalDur(const OptionalDur<Ratio2> &other)
+ : Base(other ? Base(Dur<Ratio>(*other)) : llvm::None) {}
+
+ template <typename Ratio2, typename = typename EnableIf<Ratio2>::type>
+ OptionalDur(const Dur<Ratio2> &other) : Base(Dur<Ratio>(other)) {}
+};
+
class GDBRemoteCommunication : public Communication {
public:
enum {
@@ -115,13 +139,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::seconds
+ SetPacketTimeout(std::chrono::seconds 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::seconds GetPacketTimeout() const {
+ return m_packet_timeout;
+ }
//------------------------------------------------------------------
// Start a debugserver instance on the current host using the
@@ -226,16 +253,18 @@
PacketResult SendPacketNoLock(llvm::StringRef payload);
PacketResult ReadPacket(StringExtractorGDBRemote &response,
- uint32_t timeout_usec, bool sync_on_timeout);
+ OptionalDur<std::micro> 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,
+ OptionalDur<std::micro> timeout);
PacketResult
- WaitForPacketWithTimeoutMicroSecondsNoLock(StringExtractorGDBRemote &response,
- uint32_t timeout_usec,
- bool sync_on_timeout);
+ WaitForPacketNoLock(StringExtractorGDBRemote &response,
+ OptionalDur<std::micro> 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
@@ -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,
+ OptionalDur<std::micro> 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
+ OptionalDur<std::micro> 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,
+ OptionalDur<std::micro> 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())) {
Index: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
===================================================================
--- source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
+++ source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
@@ -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::milliseconds timeout(100);
+ 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
