labath created this revision.
labath added reviewers: jingham, clayborg.
Herald added a subscriber: mgorny.
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
https://reviews.llvm.org/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.
https://reviews.llvm.org/D46580
Files:
include/lldb/Core/Event.h
include/lldb/Host/Predicate.h
include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
include/lldb/Target/Process.h
source/Commands/CommandObjectProcess.cpp
source/Commands/CommandObjectThread.cpp
source/Host/common/Host.cpp
source/Host/posix/ConnectionFileDescriptorPosix.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
source/Target/Process.cpp
unittests/Host/CMakeLists.txt
unittests/Host/PredicateTest.cpp
Index: unittests/Host/PredicateTest.cpp
===================================================================
--- /dev/null
+++ 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)));
+}
Index: unittests/Host/CMakeLists.txt
===================================================================
--- unittests/Host/CMakeLists.txt
+++ unittests/Host/CMakeLists.txt
@@ -3,6 +3,7 @@
HostInfoTest.cpp
HostTest.cpp
MainLoopTest.cpp
+ PredicateTest.cpp
SocketAddressTest.cpp
SocketTest.cpp
SymbolsTest.cpp
Index: source/Target/Process.cpp
===================================================================
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -947,21 +947,25 @@
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,
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
===================================================================
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -1133,7 +1133,7 @@
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_);
Index: source/Host/posix/ConnectionFileDescriptorPosix.cpp
===================================================================
--- source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -746,14 +746,10 @@
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 {
Index: source/Host/common/Host.cpp
===================================================================
--- source/Host/common/Host.cpp
+++ source/Host/common/Host.cpp
@@ -536,8 +536,11 @@
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
Index: source/Commands/CommandObjectThread.cpp
===================================================================
--- source/Commands/CommandObjectThread.cpp
+++ source/Commands/CommandObjectThread.cpp
@@ -786,7 +786,7 @@
// 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
Index: source/Commands/CommandObjectProcess.cpp
===================================================================
--- source/Commands/CommandObjectProcess.cpp
+++ source/Commands/CommandObjectProcess.cpp
@@ -237,7 +237,7 @@
// 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 @@
// 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());
Index: include/lldb/Target/Process.h
===================================================================
--- include/lldb/Target/Process.h
+++ include/lldb/Target/Process.h
@@ -2443,11 +2443,11 @@
/// 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,
Index: include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
===================================================================
--- include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
+++ include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
@@ -72,7 +72,7 @@
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);
Index: include/lldb/Host/Predicate.h
===================================================================
--- include/lldb/Host/Predicate.h
+++ 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 @@
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 @@
/// @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 @@
/// @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:
Index: include/lldb/Core/Event.h
===================================================================
--- include/lldb/Core/Event.h
+++ include/lldb/Core/Event.h
@@ -121,9 +121,8 @@
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-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
