Introduce chrono to more gdb-remote functions
authorPavel Labath <labath@google.com>
Thu, 24 Nov 2016 10:54:49 +0000 (10:54 +0000)
committerPavel Labath <labath@google.com>
Thu, 24 Nov 2016 10:54:49 +0000 (10:54 +0000)
Summary:
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 only tricky part was
representation of infinite 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 chose 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>). For this reason I added a special
class Timeout, inheriting from Optional, and enabling the necessary conversions
one would normally expect.

(*) 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.

Reviewers: clayborg, zturner, jingham

Subscribers: lldb-commits

Differential Revision: https://reviews.llvm.org/D26971

llvm-svn: 287864

lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
lldb/tools/lldb-server/lldb-platform.cpp

index af8db8e..1e20a09 100644 (file)
@@ -20,8 +20,9 @@
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::process_gdb_remote;
+using namespace std::chrono;
 
-static const std::chrono::seconds kInterruptTimeout(5);
+static const seconds kInterruptTimeout(5);
 
 /////////////////////////
 // GDBRemoteClientBase //
@@ -51,18 +52,13 @@ StateType GDBRemoteClientBase::SendContinuePacketAndWaitForResponse(
   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);
       if (m_async_count == 0)
         continue;
-      if (std::chrono::steady_clock::now() >=
-          m_interrupt_time + kInterruptTimeout)
+      if (steady_clock::now() >= m_interrupt_time + kInterruptTimeout)
         return eStateInvalid;
     }
     case PacketResult::Success:
@@ -188,11 +184,7 @@ GDBRemoteClientBase::SendPacketAndWaitForResponseNoLock(
 
   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 @@ bool GDBRemoteClientBase::SendvContPacket(llvm::StringRef payload,
   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,7 @@ bool GDBRemoteClientBase::ShouldStop(const UnixSignals &signals,
   // 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);
+  ReadPacket(extra_stop_reply_packet, milliseconds(100), false);
 
   // Interrupting is typically done using SIGSTOP or SIGINT, so if
   // the process stops with some other signal, we definitely want to
@@ -268,11 +259,9 @@ bool GDBRemoteClientBase::ShouldStop(const UnixSignals &signals,
   // 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;
 }
 
@@ -368,7 +357,7 @@ void GDBRemoteClientBase::Lock::SyncWithContinueThread(bool interrupt) {
       }
       if (log)
         log->PutCString("GDBRemoteClientBase::Lock::Lock sent packet: \\x03");
-      m_comm.m_interrupt_time = std::chrono::steady_clock::now();
+      m_comm.m_interrupt_time = steady_clock::now();
     }
     m_comm.m_cv.wait(lock, [this] { return m_comm.m_is_running == false; });
     m_did_interrupt = true;
index 19fc78d..c263454 100644 (file)
@@ -263,11 +263,7 @@ GDBRemoteCommunication::SendPacketNoLock(llvm::StringRef payload) {
 
 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)
@@ -280,13 +276,12 @@ GDBRemoteCommunication::PacketResult GDBRemoteCommunication::GetAck() {
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunication::ReadPacket(StringExtractorGDBRemote &response,
-                                   uint32_t timeout_usec,
+                                   Timeout<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.
@@ -295,50 +290,34 @@ GDBRemoteCommunication::ReadPacket(StringExtractorGDBRemote &response,
 // 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
+                                           Timeout<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,
+                                            Timeout<std::micro> timeout,
+                                            bool sync_on_timeout) {
   uint8_t buffer[8192];
   Error error;
 
@@ -354,12 +333,13 @@ GDBRemoteCommunication::WaitForPacketWithTimeoutMicroSecondsNoLock(
   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 +402,8 @@ GDBRemoteCommunication::WaitForPacketWithTimeoutMicroSecondsNoLock(
             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 4bcc01a..619113d 100644 (file)
@@ -53,6 +53,29 @@ enum class CompressionType {
 
 class ProcessGDBRemote;
 
+template <typename Ratio>
+class Timeout : 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:
+  Timeout(llvm::NoneType none) : Base(none) {}
+  Timeout(const Timeout &other) = default;
+
+  template <typename Ratio2, typename = typename EnableIf<Ratio2>::type>
+  Timeout(const Timeout<Ratio2> &other)
+      : Base(other ? Base(Dur<Ratio>(*other)) : llvm::None) {}
+
+  template <typename Ratio2, typename = typename EnableIf<Ratio2>::type>
+  Timeout(const Dur<Ratio2> &other) : Base(Dur<Ratio>(other)) {}
+};
+
 class GDBRemoteCommunication : public Communication {
 public:
   enum {
@@ -226,16 +249,15 @@ protected:
   PacketResult SendPacketNoLock(llvm::StringRef payload);
 
   PacketResult ReadPacket(StringExtractorGDBRemote &response,
-                          uint32_t timeout_usec, bool sync_on_timeout);
+                          Timeout<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);
+                                  Timeout<std::micro> timeout);
 
-  PacketResult
-  WaitForPacketWithTimeoutMicroSecondsNoLock(StringExtractorGDBRemote &response,
-                                             uint32_t timeout_usec,
-                                             bool sync_on_timeout);
+  PacketResult WaitForPacketNoLock(StringExtractorGDBRemote &response,
+                                   Timeout<std::micro> timeout,
+                                   bool sync_on_timeout);
 
   bool CompressionIsEnabled() {
     return m_compression_type != CompressionType::None;
index 1e5067a..f9bbaef 100644 (file)
@@ -49,6 +49,7 @@
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::process_gdb_remote;
+using namespace std::chrono;
 
 //----------------------------------------------------------------------
 // GDBRemoteCommunicationClient constructor
@@ -122,9 +123,8 @@ bool GDBRemoteCommunicationClient::HandshakeWithServer(Error *error_ptr) {
     // 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
     while (packet_result == PacketResult::Success)
-      packet_result = ReadPacket(response, timeout_usec, false);
+      packet_result = ReadPacket(response, milliseconds(10), false);
 
     // The return value from QueryNoAckModeSupported() is true if the packet
     // was sent and _any_ response (including UNIMPLEMENTED) was received),
@@ -202,8 +202,7 @@ bool GDBRemoteCommunicationClient::QueryNoAckModeSupported() {
     // 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(), seconds(6)));
 
     StringExtractorGDBRemote response;
     if (SendPacketAndWaitForResponse("QStartNoAckMode", response, false) ==
@@ -315,7 +314,7 @@ void GDBRemoteCommunicationClient::ResetDiscoverableSettings(bool did_exec) {
     m_hostname.clear();
     m_gdb_server_name.clear();
     m_gdb_server_version = UINT32_MAX;
-    m_default_packet_timeout = std::chrono::seconds(0);
+    m_default_packet_timeout = seconds(0);
     m_max_packet_size = 0;
     m_qSupported_response.clear();
     m_supported_async_json_packets_is_valid = false;
@@ -1200,7 +1199,7 @@ bool GDBRemoteCommunicationClient::GetHostInfo(bool force) {
           } else if (name.equals("default_packet_timeout")) {
             uint32_t timeout_seconds;
             if (!value.getAsInteger(0, timeout_seconds)) {
-              m_default_packet_timeout = std::chrono::seconds(timeout_seconds);
+              m_default_packet_timeout = seconds(timeout_seconds);
               SetPacketTimeout(m_default_packet_timeout);
               ++num_keys_decoded;
             }
@@ -1329,8 +1328,7 @@ GDBRemoteCommunicationClient::GetHostArchitecture() {
   return m_host_arch;
 }
 
-std::chrono::seconds
-GDBRemoteCommunicationClient::GetHostDefaultPacketTimeout() {
+seconds GDBRemoteCommunicationClient::GetHostDefaultPacketTimeout() {
   if (m_qHostInfo_is_valid == eLazyBoolCalculate)
     GetHostInfo();
   return m_default_packet_timeout;
@@ -2022,7 +2020,7 @@ uint32_t GDBRemoteCommunicationClient::FindProcesses(
     StringExtractorGDBRemote response;
     // Increase timeout as the first qfProcessInfo packet takes a long time
     // on Android. The value of 1min was arrived at empirically.
-    ScopedTimeout timeout(*this, std::chrono::seconds(60));
+    ScopedTimeout timeout(*this, minutes(1));
     if (SendPacketAndWaitForResponse(packet.GetString(), response, false) ==
         PacketResult::Success) {
       do {
@@ -2132,9 +2130,9 @@ static void MakeSpeedTestPacket(StreamString &packet, uint32_t send_size,
   }
 }
 
-std::chrono::duration<float> calculate_standard_deviation(
-    const std::vector<std::chrono::duration<float>> &v) {
-  using Dur = std::chrono::duration<float>;
+duration<float>
+calculate_standard_deviation(const std::vector<duration<float>> &v) {
+  using Dur = duration<float>;
   Dur sum = std::accumulate(std::begin(v), std::end(v), Dur());
   Dur mean = sum / v.size();
   float accum = 0;
@@ -2151,8 +2149,6 @@ void GDBRemoteCommunicationClient::TestPacketSpeed(const uint32_t num_packets,
                                                    uint32_t max_recv,
                                                    uint64_t recv_amount,
                                                    bool json, Stream &strm) {
-  using namespace std::chrono;
-
   uint32_t i;
   if (SendSpeedTestPacket(0, 0)) {
     StreamString packet;
@@ -2324,7 +2320,7 @@ bool GDBRemoteCommunicationClient::LaunchGDBServer(
     }
   }
   // give the process a few seconds to startup
-  ScopedTimeout timeout(*this, std::chrono::seconds(10));
+  ScopedTimeout timeout(*this, seconds(10));
 
   if (SendPacketAndWaitForResponse(stream.GetString(), response, false) ==
       PacketResult::Success) {
index 8ccab36..934824e 100644 (file)
@@ -38,14 +38,11 @@ void GDBRemoteCommunicationServer::RegisterPacketHandler(
 }
 
 GDBRemoteCommunication::PacketResult
-GDBRemoteCommunicationServer::GetPacketAndSendResponse(uint32_t timeout_usec,
-                                                       Error &error,
-                                                       bool &interrupt,
-                                                       bool &quit) {
+GDBRemoteCommunicationServer::GetPacketAndSendResponse(
+    Timeout<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 55a615a..0c583e6 100644 (file)
@@ -43,8 +43,9 @@ public:
   RegisterPacketHandler(StringExtractorGDBRemote::ServerPacketType packet_type,
                         PacketHandler handler);
 
-  PacketResult GetPacketAndSendResponse(uint32_t timeout_usec, Error &error,
-                                        bool &interrupt, bool &quit);
+  PacketResult GetPacketAndSendResponse(Timeout<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 0897611..fe439cd 100644 (file)
@@ -926,8 +926,8 @@ void GDBRemoteCommunicationServerLLGS::DataAvailableCallback() {
   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 21d422c..1772921 100644 (file)
@@ -364,7 +364,7 @@ int main_platform(int argc, char *argv[]) {
         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;