[debugger] Removed state enum from thread.
The enum tracked the suspension/exception state of a thread. This
information is redundant with the suspension and exception handles the
thread class tracks.
This also decouples exception and suspension (both can happen at the
same time).
This adds new functionality.
TEST=fx run-test debug_agent_tests
Change-Id: I16fcc0dc1c4e2a1586811eb4a0f7c3176ef3d750
diff --git a/src/developer/debug/debug_agent/debugged_process.cc b/src/developer/debug/debug_agent/debugged_process.cc
index 3fea04c..313f44f 100644
--- a/src/developer/debug/debug_agent/debugged_process.cc
+++ b/src/developer/debug/debug_agent/debugged_process.cc
@@ -514,9 +514,8 @@
std::vector<uint64_t>* suspended_koids) {
// We issue the suspension order for all the threads.
for (auto& [thread_koid, thread] : threads_) {
- auto past_state = thread->Suspend(synchronous);
- FXL_DCHECK(past_state);
- if (*past_state == DebuggedThread::State::kRunning) {
+ bool was_suspended = thread->Suspend(synchronous);
+ if (was_suspended) {
if (suspended_koids)
suspended_koids->push_back(thread_koid);
}
diff --git a/src/developer/debug/debug_agent/debugged_thread.cc b/src/developer/debug/debug_agent/debugged_thread.cc
index cec9e34..386eb7c 100644
--- a/src/developer/debug/debug_agent/debugged_thread.cc
+++ b/src/developer/debug/debug_agent/debugged_thread.cc
@@ -84,10 +84,9 @@
// do nothing
break;
case ThreadCreationOption::kSuspendedKeepSuspended:
- state_ = State::kException;
break;
case ThreadCreationOption::kSuspendedShouldRun:
- ResumeFromException();
+ ResumeException();
break;
}
}
@@ -96,7 +95,6 @@
void DebuggedThread::OnException(zx::exception exception_token,
zx_exception_info_t exception_info) {
- state_ = State::kException;
exception_token_ = std::move(exception_token);
debug_ipc::NotifyException exception;
@@ -147,7 +145,7 @@
// was from a normal completion of the breakpoint step, or whether
// something else went wrong while stepping.
bool completes_bp_step =
- current_breakpoint_->BreakpointStepHasException(koid_, exception->type);
+ current_breakpoint_->EndStepOver(koid_, exception->type);
current_breakpoint_ = nullptr;
if (completes_bp_step &&
run_mode_ == debug_ipc::ResumeRequest::How::kContinue) {
@@ -256,7 +254,7 @@
ResumeForRunMode();
}
-void DebuggedThread::ResumeFromException() {
+void DebuggedThread::ResumeException() {
// We need to mark that this token is correctly handled before closing it.
if (exception_token_.is_valid()) {
DEBUG_LOG(Thread) << "Resuming from exception.";
@@ -268,28 +266,30 @@
exception_token_.reset();
}
-std::optional<DebuggedThread::State> DebuggedThread::Suspend(bool synchronous) {
+void DebuggedThread::ResumeSuspension() {
+ suspend_token_.reset();
+}
+
+bool DebuggedThread::Suspend(bool synchronous) {
// Subsequent suspend calls should return immediatelly. Note that this does
// not mean that the thread is in that state, but rather that that operation
// was sent to the kernel.
- if (state_ == State::kException)
- return State::kException;
-
- if (state_ == State::kSuspended)
- return State::kSuspended;
+ if (suspended() || in_exception())
+ return false;
DEBUG_LOG(Thread) << ThreadPreamble(this) << "Suspending thread.";
zx_status_t status;
status = thread_.suspend(&suspend_token_);
- if (status != ZX_OK)
- return std::nullopt;
-
- state_ = State::kSuspended;
+ if (status != ZX_OK) {
+ FXL_LOG(WARNING) << ThreadPreamble(this)
+ << "Could not suspend: " << zx_status_get_string(status);
+ return false;
+ }
if (synchronous)
return WaitForSuspension(DefaultSuspendDeadline());
- return State::kRunning;
+ return true;
}
zx::time DebuggedThread::DefaultSuspendDeadline() {
@@ -300,8 +300,7 @@
return zx::deadline_after(zx::sec(1));
}
-std::optional<DebuggedThread::State> DebuggedThread::WaitForSuspension(
- zx::time deadline) {
+bool DebuggedThread::WaitForSuspension(zx::time deadline) {
// This function is complex because a thread in an exception state can't be
// suspended (ZX-3772). Delivery of exceptions are queued on the
// exception port so our cached state may be stale, and exceptions can also
@@ -326,16 +325,16 @@
// Always check the thread state from the kernel because of queue desribed
// above.
if (IsBlockedOnException(thread_))
- return State::kException;
+ return true;
zx_signals_t observed;
status = thread_.wait_one(ZX_THREAD_SUSPENDED,
zx::deadline_after(poll_time), &observed);
if (status == ZX_OK && (observed & ZX_THREAD_SUSPENDED))
- return State::kSuspended;
+ return true;
} while (status == ZX_ERR_TIMED_OUT && zx::clock::get_monotonic() < deadline);
- return std::nullopt;
+ return false;
}
void DebuggedThread::FillThreadRecord(
@@ -666,7 +665,7 @@
void DebuggedThread::ResumeForRunMode() {
// If we jumped, once we resume we reset the status.
- if (state_ == State::kException) {
+ if (in_exception()) {
// Note: we could have a valid suspend token here in addition to the
// exception if the suspension races with the delivery of the exception.
if (current_breakpoint_) {
@@ -679,10 +678,9 @@
// All non-continue resumptions require single stepping.
SetSingleStep(run_mode_ != debug_ipc::ResumeRequest::How::kContinue);
}
- state_ = State::kRunning;
- ResumeFromException();
- } else if (state_ == State::kSuspended) {
+ ResumeException();
+ } else if (suspended()) {
// A breakpoint should only be current when it was hit which will be
// caused by an exception.
FXL_DCHECK(!current_breakpoint_);
@@ -692,9 +690,7 @@
// The suspend token is holding the thread suspended, releasing it will
// resume (if nobody else has the thread suspended).
- state_ = State::kRunning;
- FXL_DCHECK(suspend_token_.is_valid());
- suspend_token_.reset();
+ ResumeSuspension();
}
}
@@ -705,20 +701,6 @@
thread_.write_state(ZX_THREAD_STATE_SINGLE_STEP, &value, sizeof(value));
}
-const char* DebuggedThread::StateToString(State state) {
- switch (state) {
- case State::kRunning:
- return "Running";
- case State::kException:
- return "Exception";
- case State::kSuspended:
- return "Suspended";
- }
-
- FXL_NOTREACHED();
- return "<unknown>";
-}
-
const char* DebuggedThread::ClientStateToString(ClientState client_state) {
switch (client_state) {
case ClientState::kRunning:
diff --git a/src/developer/debug/debug_agent/debugged_thread.h b/src/developer/debug/debug_agent/debugged_thread.h
index a7f95f9..e55c4e5 100644
--- a/src/developer/debug/debug_agent/debugged_thread.h
+++ b/src/developer/debug/debug_agent/debugged_thread.h
@@ -34,22 +34,6 @@
class DebuggedThread {
public:
- // The State indicates what the debugger thinks the state of the thread is.
- // This doesn't take into account other things on the system that may have
- // suspended a thread. If somebody does this, the thread will be suspended but
- // our state will be kRunning (meaning resuming it is not something the
- // debugger can do).
- enum class State {
- kRunning,
-
- // Exception from the program.
- kException,
-
- // The exception is suspended through a zx_task_suspend call.
- kSuspended,
- };
- const char* StateToString(State);
-
// Represents the state the client thinks this thread is in. Certain
// operations can suspend all the threads of a process and the debugger needs
// to know which threads should remain suspended after that operation is done.
@@ -82,7 +66,11 @@
// Resume the thread from an exception.
// If |exception_token_| is not valid, this will no-op.
- void ResumeFromException();
+ void ResumeException();
+
+ // Resume the thread from a suspension.
+ // if |suspend_token_| is not valid, this will no-op.
+ void ResumeSuspension();
// Pauses execution of the thread. Pausing happens asynchronously so the
// thread will not necessarily have stopped when this returns. Set the
@@ -92,20 +80,20 @@
// |new_state| represents what the new state of the client should be. If no
// change is wanted, you can use the overload that doesn't receives that.
//
- // The return type determines the state the thread was in *before* the suspend
- // operation. That means that if |kRunning| is returned, means that the thread
- // was running and now is suspended.
+ // Returns true if the thread was running at the moment of this call being
+ // made. Returns false if it was on a suspension condition (suspended or on an
+ // exception).
//
// A nullopt means an error ocurred while suspending.
- std::optional<State> Suspend(bool synchronous = false);
+ bool Suspend(bool synchronous = false);
// The typical suspend deadline users should use when suspending.
static zx::time DefaultSuspendDeadline();
// Waits on a suspension token.
- // A nullopt means an error ocurred while suspending.
- std::optional<State> WaitForSuspension(
- zx::time deadline = DefaultSuspendDeadline());
+ // Returns true if we could find a valid suspension condition (either
+ // suspended or on an exception). False if timeout or error.
+ bool WaitForSuspension(zx::time deadline = DefaultSuspendDeadline());
// Fills the thread status record. If full_stack is set, a full backtrace
// will be generated, otherwise a minimal one will be generated.
@@ -129,11 +117,10 @@
// Notification that a ProcessBreakpoint is about to be deleted.
void WillDeleteProcessBreakpoint(ProcessBreakpoint* bp);
- State state() const { return state_; }
-
ClientState client_state() const { return client_state_; }
void set_client_state(ClientState cs) { client_state_ = cs; }
+ bool running() const { return !suspended() && !in_exception(); }
bool suspended() const { return suspend_token_.is_valid(); }
bool in_exception() const { return exception_token_.is_valid(); }
@@ -215,10 +202,8 @@
uint64_t step_in_range_begin_ = 0;
uint64_t step_in_range_end_ = 0;
- // This is the reason for the thread suspend. This controls how the thread
- // will be resumed. State::kOther implies the suspend_token_ is
- // valid.
- State state_ = State::kRunning;
+ // This is the state the client is considering this thread to be. This is used
+ // for internal suspension the agent can do.
ClientState client_state_ = ClientState::kRunning;
zx::suspend_token suspend_token_;
diff --git a/src/developer/debug/debug_agent/hardware_breakpoint.cc b/src/developer/debug/debug_agent/hardware_breakpoint.cc
index 090dd9b..05eed2e 100644
--- a/src/developer/debug/debug_agent/hardware_breakpoint.cc
+++ b/src/developer/debug/debug_agent/hardware_breakpoint.cc
@@ -91,11 +91,7 @@
// Thread needs to be suspended or on an exception (ZX-3772).
// We make a synchronous (blocking) call.
- auto past_state = thread->Suspend(true);
- if (!past_state) {
- Warn(WarningType::kInstall, thread_koid, address, ZX_ERR_BAD_STATE);
- return ZX_ERR_BAD_STATE;
- }
+ bool was_running = thread->Suspend(true);
// Do the actual installation.
auto& arch = arch::ArchProvider::Get();
@@ -107,7 +103,7 @@
}
// If the thread was running, we need to resume it.
- if (*past_state == DebuggedThread::State::kRunning) {
+ if (was_running) {
debug_ipc::ResumeRequest resume;
resume.how = debug_ipc::ResumeRequest::How::kContinue;
thread->Resume(resume);
@@ -139,11 +135,7 @@
// Thread needs to be suspended or on an exception (ZX-3772).
// We make a synchronous (blocking) call.
- auto past_state = thread->Suspend(true);
- if (!past_state) {
- Warn(WarningType::kInstall, thread_koid, address, ZX_ERR_BAD_STATE);
- return ZX_ERR_BAD_STATE;
- }
+ bool was_running = thread->Suspend(true);
auto& arch = arch::ArchProvider::Get();
zx_status_t status = arch.UninstallHWBreakpoint(&thread->thread(), address);
@@ -153,7 +145,7 @@
}
// If the thread was running, we need to resume it.
- if (*past_state == DebuggedThread::State::kRunning) {
+ if (was_running) {
debug_ipc::ResumeRequest resume;
resume.how = debug_ipc::ResumeRequest::How::kContinue;
thread->Resume(resume);
diff --git a/src/developer/debug/debug_agent/process_breakpoint.cc b/src/developer/debug/debug_agent/process_breakpoint.cc
index 0277274..23c296d 100644
--- a/src/developer/debug/debug_agent/process_breakpoint.cc
+++ b/src/developer/debug/debug_agent/process_breakpoint.cc
@@ -93,7 +93,7 @@
thread_step_over_[thread_koid] = StepStatus::kCurrent;
}
-bool ProcessBreakpoint::BreakpointStepHasException(
+bool ProcessBreakpoint::EndStepOver(
zx_koid_t thread_koid, debug_ipc::NotifyException::Type exception_type) {
auto found_thread = thread_step_over_.find(thread_koid);
if (found_thread == thread_step_over_.end()) {
diff --git a/src/developer/debug/debug_agent/process_breakpoint.h b/src/developer/debug/debug_agent/process_breakpoint.h
index 9a2c95b..a1696733 100644
--- a/src/developer/debug/debug_agent/process_breakpoint.h
+++ b/src/developer/debug/debug_agent/process_breakpoint.h
@@ -94,7 +94,7 @@
// (the one with the breakpoint) caused an exception itself (say, an access
// violation). In either case, the breakpoint will clean up after itself from
// a single-step.
- bool BreakpointStepHasException(
+ bool EndStepOver(
zx_koid_t thread_koid, debug_ipc::NotifyException::Type exception_type);
bool SoftwareBreakpointInstalled() const;
diff --git a/src/developer/debug/debug_agent/process_breakpoint_unittest.cc b/src/developer/debug/debug_agent/process_breakpoint_unittest.cc
index 6e1daba..8ccdf65 100644
--- a/src/developer/debug/debug_agent/process_breakpoint_unittest.cc
+++ b/src/developer/debug/debug_agent/process_breakpoint_unittest.cc
@@ -213,16 +213,16 @@
// In real life, the thread would now single-step over the breakpoint. It
// would trigger a hardware breakpoint at the next instruction.
- EXPECT_TRUE(bp.BreakpointStepHasException(
- kThread1Koid, debug_ipc::NotifyException::Type::kSingleStep));
+ EXPECT_TRUE(bp.EndStepOver(kThread1Koid,
+ debug_ipc::NotifyException::Type::kSingleStep));
// Since one thread is still stepping, the memory should still be original.
EXPECT_TRUE(process_delegate.mem().IsOriginal());
// As soon as the second breakpoint is resolved, the breakpoint instruction
// should be put back.
- EXPECT_TRUE(bp.BreakpointStepHasException(
- kThread2Koid, debug_ipc::NotifyException::Type::kSingleStep));
+ EXPECT_TRUE(bp.EndStepOver(kThread2Koid,
+ debug_ipc::NotifyException::Type::kSingleStep));
EXPECT_TRUE(process_delegate.mem().StartsWithBreak());
}
