Skip to content

Commit 3879fe0

Browse files
labathlabath
committedMay 9, 2018
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
1 parent e253f2f commit 3879fe0

File tree

12 files changed

+115
-92
lines changed

12 files changed

+115
-92
lines changed
 

‎lldb/include/lldb/Core/Event.h

+2-3
Original file line numberDiff line numberDiff line change
@@ -121,9 +121,8 @@ class EventDataReceipt : public EventData {
121121

122122
const ConstString &GetFlavor() const override { return GetFlavorString(); }
123123

124-
bool WaitForEventReceived(
125-
const std::chrono::microseconds &abstime = std::chrono::microseconds(0)) {
126-
return m_predicate.WaitForValueEqualTo(true, abstime);
124+
bool WaitForEventReceived(const Timeout<std::micro> &timeout = llvm::None) {
125+
return m_predicate.WaitForValueEqualTo(true, timeout);
127126
}
128127

129128
private:

‎lldb/include/lldb/Host/Predicate.h

+49-63
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
// Other libraries and framework includes
2222
// Project includes
23+
#include "lldb/Utility/Timeout.h"
2324
#include "lldb/lldb-defines.h"
2425

2526
//#define DB_PTHREAD_LOG_EVENTS
@@ -117,6 +118,40 @@ template <class T> class Predicate {
117118
Broadcast(old_value, broadcast_type);
118119
}
119120

121+
//------------------------------------------------------------------
122+
/// Wait for Cond(m_value) to be true.
123+
///
124+
/// Waits in a thread safe way for Cond(m_value) to be true. If Cond(m_value)
125+
/// is already true, this function will return without waiting.
126+
///
127+
/// It is possible for the value to be changed between the time the value is
128+
/// set and the time the waiting thread wakes up. If the value no longer
129+
/// satisfies the condition when the waiting thread wakes up, it will go back
130+
/// into a wait state. It may be necessary for the calling code to use
131+
/// additional thread synchronization methods to detect transitory states.
132+
///
133+
/// @param[in] Cond
134+
/// The condition we want \a m_value satisfy.
135+
///
136+
/// @param[in] timeout
137+
/// How long to wait for the condition to hold.
138+
///
139+
/// @return
140+
/// @li m_value if Cond(m_value) is true.
141+
/// @li None otherwise (timeout occurred).
142+
//------------------------------------------------------------------
143+
template <typename C>
144+
llvm::Optional<T> WaitFor(C Cond, const Timeout<std::micro> &timeout) {
145+
std::unique_lock<std::mutex> lock(m_mutex);
146+
auto RealCond = [&] { return Cond(m_value); };
147+
if (!timeout) {
148+
m_condition.wait(lock, RealCond);
149+
return m_value;
150+
}
151+
if (m_condition.wait_for(lock, *timeout, RealCond))
152+
return m_value;
153+
return llvm::None;
154+
}
120155
//------------------------------------------------------------------
121156
/// Wait for \a m_value to be equal to \a value.
122157
///
@@ -134,38 +169,17 @@ template <class T> class Predicate {
134169
/// @param[in] value
135170
/// The value we want \a m_value to be equal to.
136171
///
137-
/// @param[in] abstime
138-
/// If non-nullptr, the absolute time at which we should stop
139-
/// waiting, else wait an infinite amount of time.
172+
/// @param[in] timeout
173+
/// How long to wait for the condition to hold.
140174
///
141175
/// @return
142176
/// @li \b true if the \a m_value is equal to \a value
143177
/// @li \b false otherwise (timeout occurred)
144178
//------------------------------------------------------------------
145-
bool WaitForValueEqualTo(T value, const std::chrono::microseconds &timeout =
146-
std::chrono::microseconds(0)) {
147-
// pthread_cond_timedwait() or pthread_cond_wait() will atomically unlock
148-
// the mutex and wait for the condition to be set. When either function
149-
// returns, they will re-lock the mutex. We use an auto lock/unlock class
150-
// (std::lock_guard) to allow us to return at any point in this function
151-
// and not have to worry about unlocking the mutex.
152-
std::unique_lock<std::mutex> lock(m_mutex);
153-
154-
#ifdef DB_PTHREAD_LOG_EVENTS
155-
printf("%s (value = 0x%8.8x, timeout = %llu), m_value = 0x%8.8x\n",
156-
__FUNCTION__, value, timeout.count(), m_value);
157-
#endif
158-
while (m_value != value) {
159-
if (timeout == std::chrono::microseconds(0)) {
160-
m_condition.wait(lock);
161-
} else {
162-
std::cv_status result = m_condition.wait_for(lock, timeout);
163-
if (result == std::cv_status::timeout)
164-
break;
165-
}
166-
}
167-
168-
return m_value == value;
179+
bool WaitForValueEqualTo(T value,
180+
const Timeout<std::micro> &timeout = llvm::None) {
181+
return WaitFor([&value](T current) { return value == current; }, timeout) !=
182+
llvm::None;
169183
}
170184

171185
//------------------------------------------------------------------
@@ -185,45 +199,17 @@ template <class T> class Predicate {
185199
/// @param[in] value
186200
/// The value we want \a m_value to not be equal to.
187201
///
188-
/// @param[out] new_value
189-
/// The new value if \b true is returned.
190-
///
191-
/// @param[in] abstime
192-
/// If non-nullptr, the absolute time at which we should stop
193-
/// waiting, else wait an infinite amount of time.
202+
/// @param[in] timeout
203+
/// How long to wait for the condition to hold.
194204
///
195205
/// @return
196-
/// @li \b true if the \a m_value is equal to \a value
197-
/// @li \b false otherwise
206+
/// @li m_value if m_value != value
207+
/// @li None otherwise (timeout occurred).
198208
//------------------------------------------------------------------
199-
bool WaitForValueNotEqualTo(
200-
T value, T &new_value,
201-
const std::chrono::microseconds &timeout = std::chrono::microseconds(0)) {
202-
// pthread_cond_timedwait() or pthread_cond_wait() will atomically unlock
203-
// the mutex and wait for the condition to be set. When either function
204-
// returns, they will re-lock the mutex. We use an auto lock/unlock class
205-
// (std::lock_guard) to allow us to return at any point in this function
206-
// and not have to worry about unlocking the mutex.
207-
std::unique_lock<std::mutex> lock(m_mutex);
208-
#ifdef DB_PTHREAD_LOG_EVENTS
209-
printf("%s (value = 0x%8.8x, timeout = %llu), m_value = 0x%8.8x\n",
210-
__FUNCTION__, value, timeout.count(), m_value);
211-
#endif
212-
while (m_value == value) {
213-
if (timeout == std::chrono::microseconds(0)) {
214-
m_condition.wait(lock);
215-
} else {
216-
std::cv_status result = m_condition.wait_for(lock, timeout);
217-
if (result == std::cv_status::timeout)
218-
break;
219-
}
220-
}
221-
222-
if (m_value != value) {
223-
new_value = m_value;
224-
return true;
225-
}
226-
return false;
209+
llvm::Optional<T>
210+
WaitForValueNotEqualTo(T value,
211+
const Timeout<std::micro> &timeout = llvm::None) {
212+
return WaitFor([&value](T current) { return value != current; }, timeout);
227213
}
228214

229215
protected:

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ class ConnectionFileDescriptor : public Connection {
7272

7373
lldb::IOObjectSP GetReadObject() override { return m_read_sp; }
7474

75-
uint16_t GetListeningPort(uint32_t timeout_sec);
75+
uint16_t GetListeningPort(const Timeout<std::micro> &timeout);
7676

7777
bool GetChildProcessesInherit() const;
7878
void SetChildProcessesInherit(bool child_processes_inherit);

‎lldb/include/lldb/Target/Process.h

+3-3
Original file line numberDiff line numberDiff line change
@@ -2443,11 +2443,11 @@ class Process : public std::enable_shared_from_this<Process>,
24432443
/// The main purpose of this is to implement an interlock waiting for
24442444
/// HandlePrivateEvent to push an IOHandler.
24452445
///
2446-
/// @param[in] timeout_msec
2446+
/// @param[in] timeout
24472447
/// The maximum time length to wait for the process to transition to the
2448-
/// eStateRunning state, specified in milliseconds.
2448+
/// eStateRunning state.
24492449
//--------------------------------------------------------------------------------------
2450-
void SyncIOHandler(uint32_t iohandler_id, uint64_t timeout_msec);
2450+
void SyncIOHandler(uint32_t iohandler_id, const Timeout<std::micro> &timeout);
24512451

24522452
lldb::StateType GetStateChangedEvents(
24532453
lldb::EventSP &event_sp, const Timeout<std::micro> &timeout,

‎lldb/source/Commands/CommandObjectProcess.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,7 @@ class CommandObjectProcessLaunch : public CommandObjectProcessLaunchOrAttach {
237237
// stack to the main command handler and show an (lldb) prompt before
238238
// HandlePrivateEvent (from PrivateStateThread) has a chance to call
239239
// PushProcessIOHandler().
240-
process_sp->SyncIOHandler(0, 2000);
240+
process_sp->SyncIOHandler(0, std::chrono::seconds(2));
241241

242242
llvm::StringRef data = stream.GetString();
243243
if (!data.empty())
@@ -691,7 +691,7 @@ class CommandObjectProcessContinue : public CommandObjectParsed {
691691
// stack to the main command handler and show an (lldb) prompt before
692692
// HandlePrivateEvent (from PrivateStateThread) has a chance to call
693693
// PushProcessIOHandler().
694-
process->SyncIOHandler(iohandler_id, 2000);
694+
process->SyncIOHandler(iohandler_id, std::chrono::seconds(2));
695695

696696
result.AppendMessageWithFormat("Process %" PRIu64 " resuming\n",
697697
process->GetID());

‎lldb/source/Commands/CommandObjectThread.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -786,7 +786,7 @@ class CommandObjectThreadStepWithTypeAndScope : public CommandObjectParsed {
786786
// stack to the main command handler and show an (lldb) prompt before
787787
// HandlePrivateEvent (from PrivateStateThread) has a chance to call
788788
// PushProcessIOHandler().
789-
process->SyncIOHandler(iohandler_id, 2000);
789+
process->SyncIOHandler(iohandler_id, std::chrono::seconds(2));
790790

791791
if (synchronous_execution) {
792792
// If any state changed events had anything to say, add that to the

‎lldb/source/Host/common/Host.cpp

+5-2
Original file line numberDiff line numberDiff line change
@@ -536,8 +536,11 @@ Status Host::RunShellCommand(const Args &args, const FileSpec &working_dir,
536536
error.SetErrorString("failed to get process ID");
537537

538538
if (error.Success()) {
539-
if (!shell_info_sp->process_reaped.WaitForValueEqualTo(
540-
true, std::chrono::seconds(timeout_sec))) {
539+
// TODO: Remove this and make the function take Timeout<> argument.
540+
Timeout<std::micro> timeout(llvm::None);
541+
if (timeout_sec != 0)
542+
timeout = std::chrono::seconds(timeout_sec);
543+
if (!shell_info_sp->process_reaped.WaitForValueEqualTo(true, timeout)) {
541544
error.SetErrorString("timed out waiting for shell command to complete");
542545

543546
// Kill the process since it didn't complete within the timeout specified

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

+4-8
Original file line numberDiff line numberDiff line change
@@ -746,14 +746,10 @@ ConnectionStatus ConnectionFileDescriptor::ConnectUDP(llvm::StringRef s,
746746
return eConnectionStatusSuccess;
747747
}
748748

749-
uint16_t ConnectionFileDescriptor::GetListeningPort(uint32_t timeout_sec) {
750-
uint16_t bound_port = 0;
751-
if (timeout_sec == UINT32_MAX)
752-
m_port_predicate.WaitForValueNotEqualTo(0, bound_port);
753-
else
754-
m_port_predicate.WaitForValueNotEqualTo(0, bound_port,
755-
std::chrono::seconds(timeout_sec));
756-
return bound_port;
749+
uint16_t
750+
ConnectionFileDescriptor::GetListeningPort(const Timeout<std::micro> &timeout) {
751+
auto Result = m_port_predicate.WaitForValueNotEqualTo(0, timeout);
752+
return Result ? *Result : 0;
757753
}
758754

759755
bool ConnectionFileDescriptor::GetChildProcessesInherit() const {

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -1133,7 +1133,7 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
11331133
ConnectionFileDescriptor *connection =
11341134
(ConnectionFileDescriptor *)GetConnection();
11351135
// Wait for 10 seconds to resolve the bound port
1136-
uint16_t port_ = connection->GetListeningPort(10);
1136+
uint16_t port_ = connection->GetListeningPort(std::chrono::seconds(10));
11371137
if (port_ > 0) {
11381138
char port_cstr[32];
11391139
snprintf(port_cstr, sizeof(port_cstr), "127.0.0.1:%i", port_);

‎lldb/source/Target/Process.cpp

+12-8
Original file line numberDiff line numberDiff line change
@@ -947,21 +947,25 @@ StateType Process::GetNextEvent(EventSP &event_sp) {
947947
return state;
948948
}
949949

950-
void Process::SyncIOHandler(uint32_t iohandler_id, uint64_t timeout_msec) {
950+
void Process::SyncIOHandler(uint32_t iohandler_id,
951+
const Timeout<std::micro> &timeout) {
951952
// don't sync (potentially context switch) in case where there is no process
952953
// IO
953954
if (!m_process_input_reader)
954955
return;
955956

956-
uint32_t new_iohandler_id = 0;
957-
m_iohandler_sync.WaitForValueNotEqualTo(
958-
iohandler_id, new_iohandler_id, std::chrono::milliseconds(timeout_msec));
957+
auto Result = m_iohandler_sync.WaitForValueNotEqualTo(iohandler_id, timeout);
959958

960959
Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS));
961-
if (log)
962-
log->Printf("Process::%s waited for m_iohandler_sync to change from %u, "
963-
"new value is %u",
964-
__FUNCTION__, iohandler_id, new_iohandler_id);
960+
if (Result) {
961+
LLDB_LOG(
962+
log,
963+
"waited from m_iohandler_sync to change from {0}. New value is {1}.",
964+
iohandler_id, *Result);
965+
} else {
966+
LLDB_LOG(log, "timed out waiting for m_iohandler_sync to change from {0}.",
967+
iohandler_id);
968+
}
965969
}
966970

967971
StateType Process::WaitForProcessToStop(const Timeout<std::micro> &timeout,

‎lldb/unittests/Host/CMakeLists.txt

+1
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ set (FILES
33
HostInfoTest.cpp
44
HostTest.cpp
55
MainLoopTest.cpp
6+
PredicateTest.cpp
67
SocketAddressTest.cpp
78
SocketTest.cpp
89
SymbolsTest.cpp

‎lldb/unittests/Host/PredicateTest.cpp

+34
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
//===-- PredicateTest.cpp ---------------------------------------*- C++ -*-===//
2+
//
3+
// The LLVM Compiler Infrastructure
4+
//
5+
// This file is distributed under the University of Illinois Open Source
6+
// License. See LICENSE.TXT for details.
7+
//
8+
//===----------------------------------------------------------------------===//
9+
10+
#include "lldb/Host/Predicate.h"
11+
#include "gtest/gtest.h"
12+
#include <thread>
13+
14+
using namespace lldb_private;
15+
16+
TEST(Predicate, WaitForValueEqualTo) {
17+
Predicate<int> P(0);
18+
EXPECT_TRUE(P.WaitForValueEqualTo(0));
19+
EXPECT_FALSE(P.WaitForValueEqualTo(1, std::chrono::milliseconds(10)));
20+
21+
std::thread Setter([&P] {
22+
std::this_thread::sleep_for(std::chrono::milliseconds(100));
23+
P.SetValue(1, eBroadcastAlways);
24+
});
25+
EXPECT_TRUE(P.WaitForValueEqualTo(1));
26+
Setter.join();
27+
}
28+
29+
TEST(Predicate, WaitForValueNotEqualTo) {
30+
Predicate<int> P(0);
31+
EXPECT_EQ(0, P.WaitForValueNotEqualTo(1));
32+
EXPECT_EQ(llvm::None,
33+
P.WaitForValueNotEqualTo(0, std::chrono::milliseconds(10)));
34+
}

0 commit comments

Comments
 (0)
Please sign in to comment.