Modernize and clean-up the Predicate class

Summary:
The comments on this class were out of date with the implementation, and
the implementation itself was inconsistent with our usage of the Timeout
class (I started converting everything to use this class back in D27136,
but I missed this one). I avoid duplicating the waiting logic by
introducing a templated WaitFor function, and make other functions
delegate to that. This function can be also used as a replacement for
the unused WaitForBitToBeSet functions I removed, if it turns out to be
necessary.

As this changes the meaning of a "zero" timeout, I tracked down all the
callers of these functions and updated them accordingly. Propagating the
changes to all the callers of RunShellCommand was a bit too much for
this patch, so I stopped there and will continue that in a follow-up
patch.

I also add some basic unittests for the functions I modified.

Reviewers: jingham, clayborg

Subscribers: mgorny, lldb-commits

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

llvm-svn: 331880

lldb/include/lldb/Core/Event.h

@@ -121,9 +121,8 @@ public:
 
   const ConstString &GetFlavor() const override { return GetFlavorString(); }
 
-  bool WaitForEventReceived(
-      const std::chrono::microseconds &abstime = std::chrono::microseconds(0)) {
-    return m_predicate.WaitForValueEqualTo(true, abstime);
+  bool WaitForEventReceived(const Timeout<std::micro> &timeout = llvm::None) {
+    return m_predicate.WaitForValueEqualTo(true, timeout);
   }
 
 private:

lldb/include/lldb/Host/Predicate.h

@@ -20,6 +20,7 @@
 
 // Other libraries and framework includes
 // Project includes
+#include "lldb/Utility/Timeout.h"
 #include "lldb/lldb-defines.h"
 
 //#define DB_PTHREAD_LOG_EVENTS
@@ -117,6 +118,40 @@ public:
     Broadcast(old_value, broadcast_type);
   }
 
+  //------------------------------------------------------------------
+  /// Wait for Cond(m_value) to be true.
+  ///
+  /// Waits in a thread safe way for Cond(m_value) to be true. If Cond(m_value)
+  /// is already true, this function will return without waiting.
+  ///
+  /// It is possible for the value to be changed between the time the value is
+  /// set and the time the waiting thread wakes up. If the value no longer
+  /// satisfies the condition when the waiting thread wakes up, it will go back
+  /// into a wait state. It may be necessary for the calling code to use
+  /// additional thread synchronization methods to detect transitory states.
+  ///
+  /// @param[in] Cond
+  ///     The condition we want \a m_value satisfy.
+  ///
+  /// @param[in] timeout
+  ///     How long to wait for the condition to hold.
+  ///
+  /// @return
+  ///     @li m_value if Cond(m_value) is true.
+  ///     @li None otherwise (timeout occurred).
+  //------------------------------------------------------------------
+  template <typename C>
+  llvm::Optional<T> WaitFor(C Cond, const Timeout<std::micro> &timeout) {
+    std::unique_lock<std::mutex> lock(m_mutex);
+    auto RealCond = [&] { return Cond(m_value); };
+    if (!timeout) {
+      m_condition.wait(lock, RealCond);
+      return m_value;
+    }
+    if (m_condition.wait_for(lock, *timeout, RealCond))
+      return m_value;
+    return llvm::None;
+  }
   //------------------------------------------------------------------
   /// Wait for \a m_value to be equal to \a value.
   ///
@@ -134,38 +169,17 @@ public:
   /// @param[in] value
   ///     The value we want \a m_value to be equal to.
   ///
-  /// @param[in] abstime
-  ///     If non-nullptr, the absolute time at which we should stop
-  ///     waiting, else wait an infinite amount of time.
+  /// @param[in] timeout
+  ///     How long to wait for the condition to hold.
   ///
   /// @return
   ///     @li \b true if the \a m_value is equal to \a value
   ///     @li \b false otherwise (timeout occurred)
   //------------------------------------------------------------------
-  bool WaitForValueEqualTo(T value, const std::chrono::microseconds &timeout =
-                                        std::chrono::microseconds(0)) {
-    // pthread_cond_timedwait() or pthread_cond_wait() will atomically unlock
-    // the mutex and wait for the condition to be set. When either function
-    // returns, they will re-lock the mutex. We use an auto lock/unlock class
-    // (std::lock_guard) to allow us to return at any point in this function
-    // and not have to worry about unlocking the mutex.
-    std::unique_lock<std::mutex> lock(m_mutex);
-
-#ifdef DB_PTHREAD_LOG_EVENTS
-    printf("%s (value = 0x%8.8x, timeout = %llu), m_value = 0x%8.8x\n",
-           __FUNCTION__, value, timeout.count(), m_value);
-#endif
-    while (m_value != value) {
-      if (timeout == std::chrono::microseconds(0)) {
-        m_condition.wait(lock);
-      } else {
-        std::cv_status result = m_condition.wait_for(lock, timeout);
-        if (result == std::cv_status::timeout)
-          break;
-      }
-    }
-
-    return m_value == value;
+  bool WaitForValueEqualTo(T value,
+                           const Timeout<std::micro> &timeout = llvm::None) {
+    return WaitFor([&value](T current) { return value == current; }, timeout) !=
+           llvm::None;
   }
 
   //------------------------------------------------------------------
@@ -185,45 +199,17 @@ public:
   /// @param[in] value
   ///     The value we want \a m_value to not be equal to.
   ///
-  /// @param[out] new_value
-  ///     The new value if \b true is returned.
-  ///
-  /// @param[in] abstime
-  ///     If non-nullptr, the absolute time at which we should stop
-  ///     waiting, else wait an infinite amount of time.
+  /// @param[in] timeout
+  ///     How long to wait for the condition to hold.
   ///
   /// @return
-  ///     @li \b true if the \a m_value is equal to \a value
-  ///     @li \b false otherwise
+  ///     @li m_value if m_value != value
+  ///     @li None otherwise (timeout occurred).
   //------------------------------------------------------------------
-  bool WaitForValueNotEqualTo(
-      T value, T &new_value,
-      const std::chrono::microseconds &timeout = std::chrono::microseconds(0)) {
-    // pthread_cond_timedwait() or pthread_cond_wait() will atomically unlock
-    // the mutex and wait for the condition to be set. When either function
-    // returns, they will re-lock the mutex. We use an auto lock/unlock class
-    // (std::lock_guard) to allow us to return at any point in this function
-    // and not have to worry about unlocking the mutex.
-    std::unique_lock<std::mutex> lock(m_mutex);
-#ifdef DB_PTHREAD_LOG_EVENTS
-    printf("%s (value = 0x%8.8x, timeout = %llu), m_value = 0x%8.8x\n",
-           __FUNCTION__, value, timeout.count(), m_value);
-#endif
-    while (m_value == value) {
-      if (timeout == std::chrono::microseconds(0)) {
-        m_condition.wait(lock);
-      } else {
-        std::cv_status result = m_condition.wait_for(lock, timeout);
-        if (result == std::cv_status::timeout)
-          break;
-      }
-    }
-
-    if (m_value != value) {
-      new_value = m_value;
-      return true;
-    }
-    return false;
+  llvm::Optional<T>
+  WaitForValueNotEqualTo(T value,
+                         const Timeout<std::micro> &timeout = llvm::None) {
+    return WaitFor([&value](T current) { return value != current; }, timeout);
   }
 
 protected:

lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h

@@ -72,7 +72,7 @@ public:
 
   lldb::IOObjectSP GetReadObject() override { return m_read_sp; }
 
-  uint16_t GetListeningPort(uint32_t timeout_sec);
+  uint16_t GetListeningPort(const Timeout<std::micro> &timeout);
 
   bool GetChildProcessesInherit() const;
   void SetChildProcessesInherit(bool child_processes_inherit);

lldb/include/lldb/Target/Process.h

@@ -2443,11 +2443,11 @@ public:
   /// The main purpose of this is to implement an interlock waiting for
   /// HandlePrivateEvent to push an IOHandler.
   ///
-  /// @param[in] timeout_msec
+  /// @param[in] timeout
   ///     The maximum time length to wait for the process to transition to the
-  ///     eStateRunning state, specified in milliseconds.
+  ///     eStateRunning state.
   //--------------------------------------------------------------------------------------
-  void SyncIOHandler(uint32_t iohandler_id, uint64_t timeout_msec);
+  void SyncIOHandler(uint32_t iohandler_id, const Timeout<std::micro> &timeout);
 
   lldb::StateType GetStateChangedEvents(
       lldb::EventSP &event_sp, const Timeout<std::micro> &timeout,

lldb/source/Commands/CommandObjectProcess.cpp

@@ -237,7 +237,7 @@ protected:
         // stack to the main command handler and show an (lldb) prompt before
         // HandlePrivateEvent (from PrivateStateThread) has a chance to call
         // PushProcessIOHandler().
-        process_sp->SyncIOHandler(0, 2000);
+        process_sp->SyncIOHandler(0, std::chrono::seconds(2));
 
         llvm::StringRef data = stream.GetString();
         if (!data.empty())
@@ -691,7 +691,7 @@ protected:
         // stack to the main command handler and show an (lldb) prompt before
         // HandlePrivateEvent (from PrivateStateThread) has a chance to call
         // PushProcessIOHandler().
-        process->SyncIOHandler(iohandler_id, 2000);
+        process->SyncIOHandler(iohandler_id, std::chrono::seconds(2));
 
         result.AppendMessageWithFormat("Process %" PRIu64 " resuming\n",
                                        process->GetID());

lldb/source/Commands/CommandObjectThread.cpp

@@ -786,7 +786,7 @@ protected:
       // stack to the main command handler and show an (lldb) prompt before
       // HandlePrivateEvent (from PrivateStateThread) has a chance to call
       // PushProcessIOHandler().
-      process->SyncIOHandler(iohandler_id, 2000);
+      process->SyncIOHandler(iohandler_id, std::chrono::seconds(2));
 
       if (synchronous_execution) {
         // If any state changed events had anything to say, add that to the

lldb/source/Host/common/Host.cpp

@@ -536,8 +536,11 @@ Status Host::RunShellCommand(const Args &args, const FileSpec &working_dir,
     error.SetErrorString("failed to get process ID");
 
   if (error.Success()) {
-    if (!shell_info_sp->process_reaped.WaitForValueEqualTo(
-            true, std::chrono::seconds(timeout_sec))) {
+    // TODO: Remove this and make the function take Timeout<> argument.
+    Timeout<std::micro> timeout(llvm::None);
+    if (timeout_sec != 0)
+      timeout = std::chrono::seconds(timeout_sec);
+    if (!shell_info_sp->process_reaped.WaitForValueEqualTo(true, timeout)) {
       error.SetErrorString("timed out waiting for shell command to complete");
 
       // Kill the process since it didn't complete within the timeout specified

lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

@@ -746,14 +746,10 @@ ConnectionStatus ConnectionFileDescriptor::ConnectUDP(llvm::StringRef s,
   return eConnectionStatusSuccess;
 }
 
-uint16_t ConnectionFileDescriptor::GetListeningPort(uint32_t timeout_sec) {
-  uint16_t bound_port = 0;
-  if (timeout_sec == UINT32_MAX)
-    m_port_predicate.WaitForValueNotEqualTo(0, bound_port);
-  else
-    m_port_predicate.WaitForValueNotEqualTo(0, bound_port,
-                                            std::chrono::seconds(timeout_sec));
-  return bound_port;
+uint16_t
+ConnectionFileDescriptor::GetListeningPort(const Timeout<std::micro> &timeout) {
+  auto Result = m_port_predicate.WaitForValueNotEqualTo(0, timeout);
+  return Result ? *Result : 0;
 }
 
 bool ConnectionFileDescriptor::GetChildProcessesInherit() const {

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

@@ -1133,7 +1133,7 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
         ConnectionFileDescriptor *connection =
             (ConnectionFileDescriptor *)GetConnection();
         // Wait for 10 seconds to resolve the bound port
-        uint16_t port_ = connection->GetListeningPort(10);
+        uint16_t port_ = connection->GetListeningPort(std::chrono::seconds(10));
         if (port_ > 0) {
           char port_cstr[32];
           snprintf(port_cstr, sizeof(port_cstr), "127.0.0.1:%i", port_);

lldb/source/Target/Process.cpp

@@ -947,21 +947,25 @@ StateType Process::GetNextEvent(EventSP &event_sp) {
   return state;
 }
 
-void Process::SyncIOHandler(uint32_t iohandler_id, uint64_t timeout_msec) {
+void Process::SyncIOHandler(uint32_t iohandler_id,
+                            const Timeout<std::micro> &timeout) {
   // don't sync (potentially context switch) in case where there is no process
   // IO
   if (!m_process_input_reader)
     return;
 
-  uint32_t new_iohandler_id = 0;
-  m_iohandler_sync.WaitForValueNotEqualTo(
-      iohandler_id, new_iohandler_id, std::chrono::milliseconds(timeout_msec));
+  auto Result = m_iohandler_sync.WaitForValueNotEqualTo(iohandler_id, timeout);
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS));
-  if (log)
-    log->Printf("Process::%s waited for m_iohandler_sync to change from %u, "
-                "new value is %u",
-                __FUNCTION__, iohandler_id, new_iohandler_id);
+  if (Result) {
+    LLDB_LOG(
+        log,
+        "waited from m_iohandler_sync to change from {0}. New value is {1}.",
+        iohandler_id, *Result);
+  } else {
+    LLDB_LOG(log, "timed out waiting for m_iohandler_sync to change from {0}.",
+             iohandler_id);
+  }
 }
 
 StateType Process::WaitForProcessToStop(const Timeout<std::micro> &timeout,

lldb/unittests/Host/CMakeLists.txt

@@ -3,6 +3,7 @@ set (FILES
   HostInfoTest.cpp
   HostTest.cpp
   MainLoopTest.cpp
+  PredicateTest.cpp
   SocketAddressTest.cpp
   SocketTest.cpp
   SymbolsTest.cpp

lldb/unittests/Host/PredicateTest.cpp

@@ -0,0 +1,34 @@
+//===-- PredicateTest.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/Host/Predicate.h"
+#include "gtest/gtest.h"
+#include <thread>
+
+using namespace lldb_private;
+
+TEST(Predicate, WaitForValueEqualTo) {
+  Predicate<int> P(0);
+  EXPECT_TRUE(P.WaitForValueEqualTo(0));
+  EXPECT_FALSE(P.WaitForValueEqualTo(1, std::chrono::milliseconds(10)));
+
+  std::thread Setter([&P] {
+    std::this_thread::sleep_for(std::chrono::milliseconds(100));
+    P.SetValue(1, eBroadcastAlways);
+  });
+  EXPECT_TRUE(P.WaitForValueEqualTo(1));
+  Setter.join();
+}
+
+TEST(Predicate, WaitForValueNotEqualTo) {
+  Predicate<int> P(0);
+  EXPECT_EQ(0, P.WaitForValueNotEqualTo(1));
+  EXPECT_EQ(llvm::None,
+            P.WaitForValueNotEqualTo(0, std::chrono::milliseconds(10)));
+}