[Lldb-commits] [lldb] r331880 - Modernize and clean-up the Predicate class

Pavel Labath via lldb-commits lldb-commits at lists.llvm.org
Wed May 9 07:29:30 PDT 2018


Author: labath
Date: Wed May  9 07:29:30 2018
New Revision: 331880

URL: http://llvm.org/viewvc/llvm-project?rev=331880&view=rev
Log:
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

Added:
    lldb/trunk/unittests/Host/PredicateTest.cpp
Modified:
    lldb/trunk/include/lldb/Core/Event.h
    lldb/trunk/include/lldb/Host/Predicate.h
    lldb/trunk/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
    lldb/trunk/include/lldb/Target/Process.h
    lldb/trunk/source/Commands/CommandObjectProcess.cpp
    lldb/trunk/source/Commands/CommandObjectThread.cpp
    lldb/trunk/source/Host/common/Host.cpp
    lldb/trunk/source/Host/posix/ConnectionFileDescriptorPosix.cpp
    lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
    lldb/trunk/source/Target/Process.cpp
    lldb/trunk/unittests/Host/CMakeLists.txt

Modified: lldb/trunk/include/lldb/Core/Event.h
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Core/Event.h?rev=331880&r1=331879&r2=331880&view=diff
==============================================================================
--- lldb/trunk/include/lldb/Core/Event.h (original)
+++ lldb/trunk/include/lldb/Core/Event.h Wed May  9 07:29:30 2018
@@ -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:

Modified: lldb/trunk/include/lldb/Host/Predicate.h
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Host/Predicate.h?rev=331880&r1=331879&r2=331880&view=diff
==============================================================================
--- lldb/trunk/include/lldb/Host/Predicate.h (original)
+++ lldb/trunk/include/lldb/Host/Predicate.h Wed May  9 07:29:30 2018
@@ -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
@@ -118,6 +119,40 @@ public:
   }
 
   //------------------------------------------------------------------
+  /// 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
@@ -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:

Modified: lldb/trunk/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h?rev=331880&r1=331879&r2=331880&view=diff
==============================================================================
--- lldb/trunk/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h (original)
+++ lldb/trunk/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h Wed May  9 07:29:30 2018
@@ -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);

Modified: lldb/trunk/include/lldb/Target/Process.h
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Target/Process.h?rev=331880&r1=331879&r2=331880&view=diff
==============================================================================
--- lldb/trunk/include/lldb/Target/Process.h (original)
+++ lldb/trunk/include/lldb/Target/Process.h Wed May  9 07:29:30 2018
@@ -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,

Modified: lldb/trunk/source/Commands/CommandObjectProcess.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Commands/CommandObjectProcess.cpp?rev=331880&r1=331879&r2=331880&view=diff
==============================================================================
--- lldb/trunk/source/Commands/CommandObjectProcess.cpp (original)
+++ lldb/trunk/source/Commands/CommandObjectProcess.cpp Wed May  9 07:29:30 2018
@@ -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());

Modified: lldb/trunk/source/Commands/CommandObjectThread.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Commands/CommandObjectThread.cpp?rev=331880&r1=331879&r2=331880&view=diff
==============================================================================
--- lldb/trunk/source/Commands/CommandObjectThread.cpp (original)
+++ lldb/trunk/source/Commands/CommandObjectThread.cpp Wed May  9 07:29:30 2018
@@ -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

Modified: lldb/trunk/source/Host/common/Host.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/common/Host.cpp?rev=331880&r1=331879&r2=331880&view=diff
==============================================================================
--- lldb/trunk/source/Host/common/Host.cpp (original)
+++ lldb/trunk/source/Host/common/Host.cpp Wed May  9 07:29:30 2018
@@ -536,8 +536,11 @@ Status Host::RunShellCommand(const Args
     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

Modified: lldb/trunk/source/Host/posix/ConnectionFileDescriptorPosix.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/posix/ConnectionFileDescriptorPosix.cpp?rev=331880&r1=331879&r2=331880&view=diff
==============================================================================
--- lldb/trunk/source/Host/posix/ConnectionFileDescriptorPosix.cpp (original)
+++ lldb/trunk/source/Host/posix/ConnectionFileDescriptorPosix.cpp Wed May  9 07:29:30 2018
@@ -746,14 +746,10 @@ ConnectionStatus ConnectionFileDescripto
   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 {

Modified: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp?rev=331880&r1=331879&r2=331880&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp (original)
+++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp Wed May  9 07:29:30 2018
@@ -1133,7 +1133,7 @@ Status GDBRemoteCommunication::StartDebu
         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_);

Modified: lldb/trunk/source/Target/Process.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Target/Process.cpp?rev=331880&r1=331879&r2=331880&view=diff
==============================================================================
--- lldb/trunk/source/Target/Process.cpp (original)
+++ lldb/trunk/source/Target/Process.cpp Wed May  9 07:29:30 2018
@@ -947,21 +947,25 @@ StateType Process::GetNextEvent(EventSP
   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,

Modified: lldb/trunk/unittests/Host/CMakeLists.txt
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Host/CMakeLists.txt?rev=331880&r1=331879&r2=331880&view=diff
==============================================================================
--- lldb/trunk/unittests/Host/CMakeLists.txt (original)
+++ lldb/trunk/unittests/Host/CMakeLists.txt Wed May  9 07:29:30 2018
@@ -3,6 +3,7 @@ set (FILES
   HostInfoTest.cpp
   HostTest.cpp
   MainLoopTest.cpp
+  PredicateTest.cpp
   SocketAddressTest.cpp
   SocketTest.cpp
   SymbolsTest.cpp

Added: lldb/trunk/unittests/Host/PredicateTest.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Host/PredicateTest.cpp?rev=331880&view=auto
==============================================================================
--- lldb/trunk/unittests/Host/PredicateTest.cpp (added)
+++ lldb/trunk/unittests/Host/PredicateTest.cpp Wed May  9 07:29:30 2018
@@ -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)));
+}




More information about the lldb-commits mailing list