Skip to content

Revert "Add an "interrupt timeout" to Process, and pipe that through … #2972

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 18, 2021
Merged

Revert "Add an "interrupt timeout" to Process, and pipe that through … #2972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion lldb/include/lldb/Target/Process.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ class ProcessProperties : public Properties {
bool GetWarningsUnsupportedLanguage() const;
bool GetStopOnExec() const;
std::chrono::seconds GetUtilityExpressionTimeout() const;
std::chrono::seconds GetInterruptTimeout() const;
bool GetOSPluginReportsAllThreads() const;
void SetOSPluginReportsAllThreads(bool does_report);
bool GetSteppingRunsAllThreads() const;
Expand Down
4 changes: 2 additions & 2 deletions lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -736,8 +736,8 @@ const UnixSignalsSP &PlatformRemoteGDBServer::GetRemoteUnixSignals() {
m_remote_signals_sp = UnixSignals::Create(GetRemoteSystemArchitecture());

StringExtractorGDBRemote response;
auto result =
m_gdb_client.SendPacketAndWaitForResponse("jSignalsInfo", response);
auto result = m_gdb_client.SendPacketAndWaitForResponse("jSignalsInfo",
response, false);

if (result != decltype(result)::Success ||
response.GetResponseType() != response.eResponse)
Expand Down
81 changes: 27 additions & 54 deletions lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,7 @@ using namespace lldb_private;
using namespace lldb_private::process_gdb_remote;
using namespace std::chrono;

// When we've sent a continue packet and are waiting for the target to stop,
// we wake up the wait with this interval to make sure the stub hasn't gone
// away while we were waiting.
static const seconds kWakeupInterval(5);
static const seconds kInterruptTimeout(5);

/////////////////////////
// GDBRemoteClientBase //
Expand All @@ -38,8 +35,7 @@ GDBRemoteClientBase::GDBRemoteClientBase(const char *comm_name,

StateType GDBRemoteClientBase::SendContinuePacketAndWaitForResponse(
ContinueDelegate &delegate, const UnixSignals &signals,
llvm::StringRef payload, std::chrono::seconds interrupt_timeout,
StringExtractorGDBRemote &response) {
llvm::StringRef payload, StringExtractorGDBRemote &response) {
Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS));
response.Clear();

Expand All @@ -52,34 +48,16 @@ StateType GDBRemoteClientBase::SendContinuePacketAndWaitForResponse(
if (!cont_lock)
return eStateInvalid;
OnRunPacketSent(true);
// The main ReadPacket loop wakes up at computed_timeout intervals, just to
// check that the connection hasn't dropped. When we wake up we also check
// whether there is an interrupt request that has reached its endpoint.
// If we want a shorter interrupt timeout that kWakeupInterval, we need to
// choose the shorter interval for the wake up as well.
std::chrono::seconds computed_timeout = std::min(interrupt_timeout,
kWakeupInterval);

for (;;) {
PacketResult read_result = ReadPacket(response, computed_timeout, false);
PacketResult read_result = ReadPacket(response, kInterruptTimeout, false);
switch (read_result) {
case PacketResult::ErrorReplyTimeout: {
std::lock_guard<std::mutex> lock(m_mutex);
if (m_async_count == 0) {
if (m_async_count == 0)
continue;
}
auto cur_time = steady_clock::now();
if (cur_time >= m_interrupt_endpoint)
if (steady_clock::now() >= m_interrupt_time + kInterruptTimeout)
return eStateInvalid;
else {
// We woke up and found an interrupt is in flight, but we haven't
// exceeded the interrupt wait time. So reset the wait time to the
// time left till the interrupt timeout. But don't wait longer
// than our wakeup timeout.
auto new_wait = m_interrupt_endpoint - cur_time;
computed_timeout = std::min(kWakeupInterval,
std::chrono::duration_cast<std::chrono::seconds>(new_wait));
continue;
}
break;
}
case PacketResult::Success:
Expand Down Expand Up @@ -155,9 +133,8 @@ StateType GDBRemoteClientBase::SendContinuePacketAndWaitForResponse(
}
}

bool GDBRemoteClientBase::SendAsyncSignal(
int signo, std::chrono::seconds interrupt_timeout) {
Lock lock(*this, interrupt_timeout);
bool GDBRemoteClientBase::SendAsyncSignal(int signo) {
Lock lock(*this, true);
if (!lock || !lock.DidInterrupt())
return false;

Expand All @@ -167,26 +144,25 @@ bool GDBRemoteClientBase::SendAsyncSignal(
return true;
}

bool GDBRemoteClientBase::Interrupt(std::chrono::seconds interrupt_timeout) {
Lock lock(*this, interrupt_timeout);
bool GDBRemoteClientBase::Interrupt() {
Lock lock(*this, true);
if (!lock.DidInterrupt())
return false;
m_should_stop = true;
return true;
}

GDBRemoteCommunication::PacketResult
GDBRemoteClientBase::SendPacketAndWaitForResponse(
llvm::StringRef payload, StringExtractorGDBRemote &response,
std::chrono::seconds interrupt_timeout) {
Lock lock(*this, interrupt_timeout);
bool send_async) {
Lock lock(*this, send_async);
if (!lock) {
if (Log *log =
ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS))
LLDB_LOGF(log,
"GDBRemoteClientBase::%s failed to get mutex, not sending "
"packet '%.*s'",
__FUNCTION__, int(payload.size()), payload.data());
"packet '%.*s' (send_async=%d)",
__FUNCTION__, int(payload.size()), payload.data(), send_async);
return PacketResult::ErrorSendFailed;
}

Expand All @@ -196,16 +172,16 @@ GDBRemoteClientBase::SendPacketAndWaitForResponse(
GDBRemoteCommunication::PacketResult
GDBRemoteClientBase::SendPacketAndReceiveResponseWithOutputSupport(
llvm::StringRef payload, StringExtractorGDBRemote &response,
std::chrono::seconds interrupt_timeout,
bool send_async,
llvm::function_ref<void(llvm::StringRef)> output_callback) {
Lock lock(*this, interrupt_timeout);
Lock lock(*this, send_async);
if (!lock) {
if (Log *log =
ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS))
LLDB_LOGF(log,
"GDBRemoteClientBase::%s failed to get mutex, not sending "
"packet '%.*s'",
__FUNCTION__, int(payload.size()), payload.data());
"packet '%.*s' (send_async=%d)",
__FUNCTION__, int(payload.size()), payload.data(), send_async);
return PacketResult::ErrorSendFailed;
}

Expand Down Expand Up @@ -246,14 +222,13 @@ GDBRemoteClientBase::SendPacketAndWaitForResponseNoLock(
return packet_result;
}

bool GDBRemoteClientBase::SendvContPacket(
llvm::StringRef payload, std::chrono::seconds interrupt_timeout,
StringExtractorGDBRemote &response) {
bool GDBRemoteClientBase::SendvContPacket(llvm::StringRef payload,
StringExtractorGDBRemote &response) {
Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS));
LLDB_LOGF(log, "GDBRemoteCommunicationClient::%s ()", __FUNCTION__);

// we want to lock down packet sending while we continue
Lock lock(*this, interrupt_timeout);
Lock lock(*this, true);

LLDB_LOGF(log,
"GDBRemoteCommunicationClient::%s () sending vCont packet: %.*s",
Expand Down Expand Up @@ -361,20 +336,18 @@ GDBRemoteClientBase::ContinueLock::lock() {
// GDBRemoteClientBase::Lock //
///////////////////////////////

GDBRemoteClientBase::Lock::Lock(GDBRemoteClientBase &comm,
std::chrono::seconds interrupt_timeout)
GDBRemoteClientBase::Lock::Lock(GDBRemoteClientBase &comm, bool interrupt)
: m_async_lock(comm.m_async_mutex, std::defer_lock), m_comm(comm),
m_interrupt_timeout(interrupt_timeout), m_acquired(false),
m_did_interrupt(false) {
SyncWithContinueThread();
m_acquired(false), m_did_interrupt(false) {
SyncWithContinueThread(interrupt);
if (m_acquired)
m_async_lock.lock();
}

void GDBRemoteClientBase::Lock::SyncWithContinueThread() {
void GDBRemoteClientBase::Lock::SyncWithContinueThread(bool interrupt) {
Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS));
std::unique_lock<std::mutex> lock(m_comm.m_mutex);
if (m_comm.m_is_running && m_interrupt_timeout == std::chrono::seconds(0))
if (m_comm.m_is_running && !interrupt)
return; // We were asked to avoid interrupting the sender. Lock is not
// acquired.

Expand All @@ -392,9 +365,9 @@ void GDBRemoteClientBase::Lock::SyncWithContinueThread() {
"interrupt packet");
return;
}
m_comm.m_interrupt_endpoint = steady_clock::now() + m_interrupt_timeout;
if (log)
log->PutCString("GDBRemoteClientBase::Lock::Lock sent packet: \\x03");
m_comm.m_interrupt_time = steady_clock::now();
}
m_comm.m_cv.wait(lock, [this] { return !m_comm.m_is_running; });
m_did_interrupt = true;
Expand Down
40 changes: 11 additions & 29 deletions lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,46 +33,29 @@ class GDBRemoteClientBase : public GDBRemoteCommunication {

GDBRemoteClientBase(const char *comm_name, const char *listener_name);

bool SendAsyncSignal(int signo, std::chrono::seconds interrupt_timeout);
bool SendAsyncSignal(int signo);

bool Interrupt(std::chrono::seconds interrupt_timeout);
bool Interrupt();

lldb::StateType SendContinuePacketAndWaitForResponse(
ContinueDelegate &delegate, const UnixSignals &signals,
llvm::StringRef payload, std::chrono::seconds interrupt_timeout,
StringExtractorGDBRemote &response);

// If interrupt_timeout == 0 seconds, don't interrupt the target.
// Only send the packet if the target is stopped.
// If you want to use this mode, use the fact that the timeout is defaulted
// so it's clear from the call-site that you are using no-interrupt.
// If it is non-zero, interrupt the target if it is running, and
// send the packet.
// It the target doesn't respond within the given timeout, it returns
// ErrorReplyTimeout.
PacketResult SendPacketAndWaitForResponse(
llvm::StringRef payload, StringExtractorGDBRemote &response,
std::chrono::seconds interrupt_timeout = std::chrono::seconds(0));
llvm::StringRef payload, StringExtractorGDBRemote &response);

PacketResult SendPacketAndWaitForResponse(llvm::StringRef payload,
StringExtractorGDBRemote &response,
bool send_async);

PacketResult SendPacketAndReceiveResponseWithOutputSupport(
llvm::StringRef payload, StringExtractorGDBRemote &response,
std::chrono::seconds interrupt_timeout,
bool send_async,
llvm::function_ref<void(llvm::StringRef)> output_callback);

bool SendvContPacket(llvm::StringRef payload,
std::chrono::seconds interrupt_timeout,
StringExtractorGDBRemote &response);

class Lock {
public:
// If interrupt_timeout == 0 seconds, only take the lock if the target is
// not running. If using this option, use the fact that the
// interrupt_timeout is defaulted so it will be obvious at the call site
// that you are choosing this mode. If it is non-zero, interrupt the target
// if it is running, waiting for the given timeout for the interrupt to
// succeed.
Lock(GDBRemoteClientBase &comm,
std::chrono::seconds interrupt_timeout = std::chrono::seconds(0));
Lock(GDBRemoteClientBase &comm, bool interrupt);
~Lock();

explicit operator bool() { return m_acquired; }
Expand All @@ -84,11 +67,10 @@ class GDBRemoteClientBase : public GDBRemoteCommunication {
private:
std::unique_lock<std::recursive_mutex> m_async_lock;
GDBRemoteClientBase &m_comm;
std::chrono::seconds m_interrupt_timeout;
bool m_acquired;
bool m_did_interrupt;

void SyncWithContinueThread();
void SyncWithContinueThread(bool interrupt);
};

protected:
Expand Down Expand Up @@ -127,7 +109,7 @@ class GDBRemoteClientBase : public GDBRemoteCommunication {

/// When was the interrupt packet sent. Used to make sure we time out if the
/// stub does not respond to interrupt requests.
std::chrono::time_point<std::chrono::steady_clock> m_interrupt_endpoint;
std::chrono::time_point<std::chrono::steady_clock> m_interrupt_time;

/// Number of threads interested in sending.
uint32_t m_async_count;
Expand Down
Loading