Introduce chrono to the Communication class

This replaces the raw integer timeout parameters in the class with their
chrono-based equivalents.  To achieve this, I have moved the Timeout class to a
more generic place and added a quick unit test for it.

llvm-svn: 287920

lldb/include/lldb/Core/Communication.h

@@ -21,7 +21,7 @@
 #include "lldb/Core/Broadcaster.h"
 #include "lldb/Core/Error.h"
 #include "lldb/Host/HostThread.h"
-#include "lldb/lldb-private.h"
+#include "lldb/Utility/Timeout.h"
 #include "lldb/lldb-private.h"
 
 namespace lldb_private {
@@ -196,15 +196,15 @@ public:
   ///     The number of bytes to attempt to read, and also the max
   ///     number of bytes that can be placed into \a dst.
   ///
-  /// @param[in] timeout_usec
-  ///     A timeout value in micro-seconds.
+  /// @param[in] timeout
+  ///     A timeout value or llvm::None for no timeout.
   ///
   /// @return
   ///     The number of bytes actually read.
   ///
   /// @see size_t Connection::Read (void *, size_t);
   //------------------------------------------------------------------
-  size_t Read(void *dst, size_t dst_len, uint32_t timeout_usec,
+  size_t Read(void *dst, size_t dst_len, const Timeout<std::micro> &timeout,
               lldb::ConnectionStatus &status, Error *error_ptr);
 
   //------------------------------------------------------------------
@@ -347,7 +347,8 @@ protected:
   void *m_callback_baton;
   bool m_close_on_eof;
 
-  size_t ReadFromConnection(void *dst, size_t dst_len, uint32_t timeout_usec,
+  size_t ReadFromConnection(void *dst, size_t dst_len,
+                            const Timeout<std::micro> &timeout,
                             lldb::ConnectionStatus &status, Error *error_ptr);
 
   //------------------------------------------------------------------

lldb/include/lldb/Utility/Timeout.h

@@ -0,0 +1,56 @@
+//===-- Timeout.h -----------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef liblldb_Timeout_h_
+#define liblldb_Timeout_h_
+
+#include <llvm/ADT/Optional.h>
+
+#include <chrono>
+
+namespace lldb_private {
+
+// A general purpose class for representing timeouts for various APIs. It's
+// basically an llvm::Optional<std::chrono::duration<int64_t, Ratio>>, but we
+// customize it a bit to enable the standard chrono implicit conversions (e.g.
+// from Timeout<std::milli> to Timeout<std::micro>.
+//
+// The intended meaning of the values is:
+// - llvm::None - no timeout, the call should wait forever
+// - 0 - poll, only complete the call if it will not block
+// - >0 - wait for a given number of units for the result
+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 Rep2, typename Ratio2>
+  using EnableIf = std::enable_if<
+      std::is_convertible<std::chrono::duration<Rep2, 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<int64_t, Ratio2>::type>
+  Timeout(const Timeout<Ratio2> &other)
+      : Base(other ? Base(Dur<Ratio>(*other)) : llvm::None) {}
+
+  template <typename Rep2, typename Ratio2,
+            typename = typename EnableIf<Rep2, Ratio2>::type>
+  Timeout(const std::chrono::duration<Rep2, Ratio2> &other)
+      : Base(Dur<Ratio>(other)) {}
+};
+
+} // namespace lldb_private
+
+#endif // liblldb_Timeout_h_

lldb/source/API/SBCommunication.cpp

@@ -119,8 +119,11 @@ size_t SBCommunication::Read(void *dst, size_t dst_len, uint32_t timeout_usec,
                 static_cast<void *>(m_opaque), static_cast<void *>(dst),
                 static_cast<uint64_t>(dst_len), timeout_usec);
   size_t bytes_read = 0;
+  Timeout<std::micro> timeout = timeout_usec == UINT32_MAX
+                                    ? Timeout<std::micro>(llvm::None)
+                                    : std::chrono::microseconds(timeout_usec);
   if (m_opaque)
-    bytes_read = m_opaque->Read(dst, dst_len, timeout_usec, status, NULL);
+    bytes_read = m_opaque->Read(dst, dst_len, timeout, status, NULL);
   else
     status = eConnectionStatusNoConnection;
 

lldb/source/Core/Communication.cpp

@@ -112,18 +112,20 @@ bool Communication::HasConnection() const {
   return m_connection_sp.get() != nullptr;
 }
 
-size_t Communication::Read(void *dst, size_t dst_len, uint32_t timeout_usec,
+size_t Communication::Read(void *dst, size_t dst_len,
+                           const Timeout<std::micro> &timeout,
                            ConnectionStatus &status, Error *error_ptr) {
   lldb_private::LogIfAnyCategoriesSet(
       LIBLLDB_LOG_COMMUNICATION,
       "%p Communication::Read (dst = %p, dst_len = %" PRIu64
       ", timeout = %u usec) connection = %p",
-      this, dst, (uint64_t)dst_len, timeout_usec, m_connection_sp.get());
+      this, dst, (uint64_t)dst_len, timeout ? timeout->count() : -1,
+      m_connection_sp.get());
 
   if (m_read_thread_enabled) {
     // We have a dedicated read thread that is getting data for us
     size_t cached_bytes = GetCachedBytes(dst, dst_len);
-    if (cached_bytes > 0 || timeout_usec == 0) {
+    if (cached_bytes > 0 || (timeout && timeout->count() == 0)) {
       status = eConnectionStatusSuccess;
       return cached_bytes;
     }
@@ -139,10 +141,9 @@ size_t Communication::Read(void *dst, size_t dst_len, uint32_t timeout_usec,
     listener_sp->StartListeningForEvents(
         this, eBroadcastBitReadThreadGotBytes | eBroadcastBitReadThreadDidExit);
     EventSP event_sp;
-    std::chrono::microseconds timeout = std::chrono::microseconds(0);
-    if (timeout_usec != UINT32_MAX)
-      timeout = std::chrono::microseconds(timeout_usec);
-    while (listener_sp->WaitForEvent(timeout, event_sp)) {
+    std::chrono::microseconds listener_timeout =
+        timeout ? *timeout : std::chrono::microseconds(0);
+    while (listener_sp->WaitForEvent(listener_timeout, event_sp)) {
       const uint32_t event_type = event_sp->GetType();
       if (event_type & eBroadcastBitReadThreadGotBytes) {
         return GetCachedBytes(dst, dst_len);
@@ -159,15 +160,7 @@ size_t Communication::Read(void *dst, size_t dst_len, uint32_t timeout_usec,
 
   // We aren't using a read thread, just read the data synchronously in this
   // thread.
-  lldb::ConnectionSP connection_sp(m_connection_sp);
-  if (connection_sp) {
-    return connection_sp->Read(dst, dst_len, timeout_usec, status, error_ptr);
-  }
-
-  if (error_ptr)
-    error_ptr->SetErrorString("Invalid connection.");
-  status = eConnectionStatusNoConnection;
-  return 0;
+  return ReadFromConnection(dst, dst_len, timeout, status, error_ptr);
 }
 
 size_t Communication::Write(const void *src, size_t src_len,
@@ -279,14 +272,20 @@ void Communication::AppendBytesToCache(const uint8_t *bytes, size_t len,
 }
 
 size_t Communication::ReadFromConnection(void *dst, size_t dst_len,
-                                         uint32_t timeout_usec,
+                                         const Timeout<std::micro> &timeout,
                                          ConnectionStatus &status,
                                          Error *error_ptr) {
   lldb::ConnectionSP connection_sp(m_connection_sp);
-  return (
-      connection_sp
-          ? connection_sp->Read(dst, dst_len, timeout_usec, status, error_ptr)
-          : 0);
+  if (connection_sp) {
+    return connection_sp->Read(dst, dst_len,
+                               timeout ? timeout->count() : UINT32_MAX, status,
+                               error_ptr);
+  }
+
+  if (error_ptr)
+    error_ptr->SetErrorString("Invalid connection.");
+  status = eConnectionStatusNoConnection;
+  return 0;
 }
 
 bool Communication::ReadThreadIsRunning() { return m_read_thread_enabled; }
@@ -305,9 +304,8 @@ lldb::thread_result_t Communication::ReadThread(lldb::thread_arg_t p) {
   ConnectionStatus status = eConnectionStatusSuccess;
   bool done = false;
   while (!done && comm->m_read_thread_enabled) {
-    const int timeout_us = 5000000;
     size_t bytes_read = comm->ReadFromConnection(
-        buf, sizeof(buf), timeout_us, status, &error);
+        buf, sizeof(buf), std::chrono::seconds(5), status, &error);
     if (bytes_read > 0)
       comm->AppendBytesToCache(buf, bytes_read, true, status);
     else if ((bytes_read == 0) && status == eConnectionStatusEndOfFile) {

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp

@@ -332,9 +332,7 @@ GDBRemoteCommunication::WaitForPacketNoLock(StringExtractorGDBRemote &packet,
   bool disconnected = false;
   while (IsConnected() && !timed_out) {
     lldb::ConnectionStatus status = eConnectionStatusNoConnection;
-    size_t bytes_read =
-        Read(buffer, sizeof(buffer), timeout ? timeout->count() : UINT32_MAX,
-             status, &error);
+    size_t bytes_read = Read(buffer, sizeof(buffer), timeout, status, &error);
 
     if (log)
       log->Printf("%s: Read (buffer, (sizeof(buffer), timeout = %ld us, "

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h

@@ -53,32 +53,6 @@ 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 Rep2, typename Ratio2>
-  using EnableIf = std::enable_if<
-      std::is_convertible<std::chrono::duration<Rep2, 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<int64_t, Ratio2>::type>
-  Timeout(const Timeout<Ratio2> &other)
-      : Base(other ? Base(Dur<Ratio>(*other)) : llvm::None) {}
-
-  template <typename Rep2, typename Ratio2,
-            typename = typename EnableIf<Rep2, Ratio2>::type>
-  Timeout(const std::chrono::duration<Rep2, Ratio2> &other)
-      : Base(Dur<Ratio>(other)) {}
-};
-
 class GDBRemoteCommunication : public Communication {
 public:
   enum {

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

@@ -1021,8 +1021,8 @@ void GDBRemoteCommunicationServerLLGS::SendProcessOutput() {
   ConnectionStatus status;
   Error error;
   while (true) {
-    size_t bytes_read =
-        m_stdio_communication.Read(buffer, sizeof buffer, 0, status, &error);
+    size_t bytes_read = m_stdio_communication.Read(
+        buffer, sizeof buffer, std::chrono::microseconds(0), status, &error);
     switch (status) {
     case eConnectionStatusSuccess:
       SendONotification(buffer, bytes_read);

lldb/unittests/Utility/CMakeLists.txt

@@ -2,6 +2,7 @@ add_lldb_unittest(UtilityTests
   ModuleCacheTest.cpp
   StringExtractorTest.cpp
   TaskPoolTest.cpp
+  TimeoutTest.cpp
   UriParserTest.cpp
   )
 

lldb/unittests/Utility/TimeoutTest.cpp

@@ -0,0 +1,22 @@
+//===-- TimeoutTest.cpp -----------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Utility/Timeout.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace std::chrono;
+
+TEST(TimeoutTest, Construction) {
+  ASSERT_FALSE(Timeout<std::micro>(llvm::None));
+  ASSERT_TRUE(bool(Timeout<std::micro>(seconds(0))));
+  ASSERT_EQ(seconds(0), *Timeout<std::micro>(seconds(0)));
+  ASSERT_EQ(seconds(3), *Timeout<std::micro>(seconds(3)));
+  ASSERT_TRUE(bool(Timeout<std::micro>(Timeout<std::milli>(seconds(0)))));
+}