Modernize and clean-up the Predicate class

[labath]

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.

[clayborg]

Looks great!

lldb/trunk/include/lldb/Core/Event.h

public:
 
   static const ConstString &GetFlavorString() {
     static ConstString g_flavor("Process::ProcessEventData");
     return g_flavor;
   }
 
   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:
   Predicate<bool> m_predicate;
 
   void DoOnRemoval(Event *event_ptr) override {
     m_predicate.SetValue(true, eBroadcastAlways);
   }

lldb/trunk/include/lldb/Host/Predicate.h

 #include <time.h>
 
 // C++ Includes
 #include <condition_variable>
 #include <mutex>
 
 // Other libraries and framework includes
 // Project includes
+#include "lldb/Utility/Timeout.h"
 #include "lldb/lldb-defines.h"
 
 //#define DB_PTHREAD_LOG_EVENTS
 
 //----------------------------------------------------------------------
 /// Enumerations for broadcasting.
 //----------------------------------------------------------------------
 namespace lldb_private {

 #endif
     const T old_value = m_value;
     m_value = value;
 
     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.
   ///
   /// Waits in a thread safe way for \a m_value to be equal to \a
   /// value. If \a m_value is already equal to \a value, 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 matches the requested value 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] 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;
   }
 
   //------------------------------------------------------------------
   /// Wait for \a m_value to not be equal to \a value.
   ///
   /// Waits in a thread safe way for \a m_value to not be equal to \a
   /// value. If \a m_value is already not equal to \a value, 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 is equal to the test value 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] 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:
   //----------------------------------------------------------------------
   // pthread condition and mutex variable to control access and allow blocking
   // between the main thread and the spotlight index thread.
   //----------------------------------------------------------------------
   T m_value; ///< The templatized value T that we are protecting access to

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

public:
 
   lldb::ConnectionStatus BytesAvailable(const Timeout<std::micro> &timeout,
                                         Status *error_ptr);
 
   bool InterruptRead() override;
 
   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);
 
 protected:
   void OpenCommandPipe();
 
   void CloseCommandPipe();

lldb/trunk/include/lldb/Target/Process.h

public:
   uint32_t GetIOHandlerID() const { return m_iohandler_sync.GetValue(); }
 
   //--------------------------------------------------------------------------------------
   /// Waits for the process state to be running within a given msec timeout.
   ///
   /// 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::ListenerSP
           hijack_listener); // Pass an empty ListenerSP to use builtin listener
 
   //--------------------------------------------------------------------------------------
   /// Centralize the code that handles and prints descriptions for process

lldb/trunk/source/Commands/CommandObjectProcess.cpp

bool DoExecute(Args &launch_args, CommandReturnObject &result) override {
 
     if (error.Success()) {
       ProcessSP process_sp(target->GetProcessSP());
       if (process_sp) {
         // There is a race condition where this thread will return up the call
         // 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())
           result.AppendMessage(data);
         const char *archname =
             exe_module_sp->GetArchitecture().GetArchitectureName();
         result.AppendMessageWithFormat(
             "Process %" PRIu64 " launched: '%s' (%s)\n", process_sp->GetID(),
if (state == eStateStopped) {
       else
         error = process->Resume();
 
       if (error.Success()) {
         // There is a race condition where this thread will return up the call
         // 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());
         if (synchronous_execution) {
           // If any state changed events had anything to say, add that to the
           // result
           result.AppendMessage(stream.GetString());
 

lldb/trunk/source/Commands/CommandObjectThread.cpp

if (new_plan_sp) {
         result.SetStatus(eReturnStatusFailed);
         return false;
       }
 
       // There is a race condition where this thread will return up the call
       // 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
         // result
         if (stream.GetSize() > 0)
           result.AppendMessage(stream.GetString());
 
         process->GetThreadList().SetSelectedThreadByID(thread->GetID());

lldb/trunk/source/Host/common/Host.cpp

Status Host::RunShellCommand(const Args &args, const FileSpec &working_dir,
 
   error = LaunchProcess(launch_info);
   const lldb::pid_t pid = launch_info.GetProcessID();
 
   if (error.Success() && pid == LLDB_INVALID_PROCESS_ID)
     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
       Kill(pid, SIGKILL);
       // Wait for the monitor callback to get the message
       shell_info_sp->process_reaped.WaitForValueEqualTo(
           true, std::chrono::seconds(1));
     } else {

lldb/trunk/source/Host/posix/ConnectionFileDescriptorPosix.cpp

ConnectionStatus ConnectionFileDescriptor::ConnectUDP(llvm::StringRef s,
   m_read_sp = m_write_sp;
   if (error.Fail()) {
     return eConnectionStatusError;
   }
   m_uri.assign(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 {
   return m_child_processes_inherit;
 }
 
 void ConnectionFileDescriptor::SetChildProcessesInherit(
     bool child_processes_inherit) {

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

#endif
                         "thread: %s",
                         __FUNCTION__, error.AsCString());
           return error;
         }
 
         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_);
           // Send the host and port down that debugserver and specify an option
           // so that it connects back to the port we are listening to in this
           // process
           debugserver_args.AppendArgument(llvm::StringRef("--reverse-connect"));
           debugserver_args.AppendArgument(llvm::StringRef(port_cstr));

lldb/trunk/source/Target/Process.cpp

StateType Process::GetNextEvent(EventSP &event_sp) {
   if (m_listener_sp->GetEventForBroadcaster(this, event_sp,
                                             std::chrono::seconds(0)) &&
       event_sp)
     state = Process::ProcessEventData::GetStateFromEvent(event_sp.get());
 
   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,
                                         EventSP *event_sp_ptr, bool wait_always,
                                         ListenerSP hijack_listener_sp,
                                         Stream *stream, bool use_run_lock) {
   // We can't just wait for a "stopped" event, because the stopped event may
   // have restarted the target. We have to actually check each event, and in

lldb/trunk/unittests/Host/CMakeLists.txt

 set (FILES
   FileSystemTest.cpp
   HostInfoTest.cpp
   HostTest.cpp
   MainLoopTest.cpp
+  PredicateTest.cpp
   SocketAddressTest.cpp
   SocketTest.cpp
   SymbolsTest.cpp
   TaskPoolTest.cpp
 )
 
 if (CMAKE_SYSTEM_NAME MATCHES "Linux|Android")
   list(APPEND FILES

lldb/trunk/unittests/Host/PredicateTest.cpp

+//===-- 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)));
+}