[llvm-branch-commits] [lldb] [lldb][NFC] Refactor StopInfoWatchpoint::PerformAction (PR #163696)
via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Wed Oct 15 21:52:58 PDT 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-lldb
Author: None (dlav-sc)
<details>
<summary>Changes</summary>
Refactor watchpoint logic 2/2
This patch refactors the StopInfoWatchpoint::PerformAction function. It leverages the ShouldReport method introduced in the previous patch to significantly simplify the PerformAction logic.
---
Full diff: https://github.com/llvm/llvm-project/pull/163696.diff
3 Files Affected:
- (modified) lldb/include/lldb/Breakpoint/Watchpoint.h (-5)
- (modified) lldb/source/Breakpoint/Watchpoint.cpp (-56)
- (modified) lldb/source/Target/StopInfo.cpp (+52-136)
``````````diff
diff --git a/lldb/include/lldb/Breakpoint/Watchpoint.h b/lldb/include/lldb/Breakpoint/Watchpoint.h
index 9e7e986e60606..3ca7629750ebb 100644
--- a/lldb/include/lldb/Breakpoint/Watchpoint.h
+++ b/lldb/include/lldb/Breakpoint/Watchpoint.h
@@ -86,7 +86,6 @@ class Watchpoint : public std::enable_shared_from_this<Watchpoint>,
void SetDeclInfo(const std::string &str);
std::string GetWatchSpec() const;
void SetWatchSpec(const std::string &str);
- bool WatchedValueReportable(const ExecutionContext &exe_ctx);
// This function determines whether we should report a watchpoint value
// change. Specifically, it checks the watchpoint condition (if present),
@@ -102,7 +101,6 @@ class Watchpoint : public std::enable_shared_from_this<Watchpoint>,
// Snapshot management interface.
bool IsWatchVariable() const;
void SetWatchVariable(bool val);
- bool CaptureWatchedValue(const ExecutionContext &exe_ctx);
/// \struct WatchpointVariableContext
/// \brief Represents the context of a watchpoint variable.
@@ -205,7 +203,6 @@ class Watchpoint : public std::enable_shared_from_this<Watchpoint>,
private:
friend class Target;
friend class WatchpointList;
- friend class StopInfoWatchpoint; // This needs to call UndoHitCount()
lldb::ValueObjectSP CalculateWatchedValue() const;
@@ -223,8 +220,6 @@ class Watchpoint : public std::enable_shared_from_this<Watchpoint>,
m_new_value_sp.reset();
}
- void UndoHitCount() { m_hit_counter.Decrement(); }
-
Target &m_target;
bool m_enabled; // Is this watchpoint enabled
bool m_is_hardware; // Is this a hardware watchpoint
diff --git a/lldb/source/Breakpoint/Watchpoint.cpp b/lldb/source/Breakpoint/Watchpoint.cpp
index 07d8f64737dc1..5ee8b227428d3 100644
--- a/lldb/source/Breakpoint/Watchpoint.cpp
+++ b/lldb/source/Breakpoint/Watchpoint.cpp
@@ -260,66 +260,10 @@ bool Watchpoint::IsWatchVariable() const { return m_is_watch_variable; }
void Watchpoint::SetWatchVariable(bool val) { m_is_watch_variable = val; }
-bool Watchpoint::CaptureWatchedValue(const ExecutionContext &exe_ctx) {
- ConstString g_watch_name("$__lldb__watch_value");
- m_old_value_sp = m_new_value_sp;
- Address watch_address(GetLoadAddress());
- if (!m_type.IsValid()) {
- // Don't know how to report new & old values, since we couldn't make a
- // scalar type for this watchpoint. This works around an assert in
- // ValueObjectMemory::Create.
- // FIXME: This should not happen, but if it does in some case we care about,
- // we can go grab the value raw and print it as unsigned.
- return false;
- }
- m_new_value_sp = ValueObjectMemory::Create(
- exe_ctx.GetBestExecutionContextScope(), g_watch_name.GetStringRef(),
- watch_address, m_type);
- m_new_value_sp = m_new_value_sp->CreateConstantValue(g_watch_name);
- return (m_new_value_sp && m_new_value_sp->GetError().Success());
-}
-
-bool Watchpoint::WatchedValueReportable(const ExecutionContext &exe_ctx) {
- if (!WatchpointModify() || WatchpointRead())
- return true;
- if (!m_type.IsValid())
- return true;
-
- ConstString g_watch_name("$__lldb__watch_value");
- Address watch_address(GetLoadAddress());
- ValueObjectSP newest_valueobj_sp = ValueObjectMemory::Create(
- exe_ctx.GetBestExecutionContextScope(), g_watch_name.GetStringRef(),
- watch_address, m_type);
- newest_valueobj_sp = newest_valueobj_sp->CreateConstantValue(g_watch_name);
- Status error;
-
- DataExtractor new_data;
- DataExtractor old_data;
-
- newest_valueobj_sp->GetData(new_data, error);
- if (error.Fail())
- return true;
- m_new_value_sp->GetData(old_data, error);
- if (error.Fail())
- return true;
-
- if (new_data.GetByteSize() != old_data.GetByteSize() ||
- new_data.GetByteSize() == 0)
- return true;
-
- if (memcmp(new_data.GetDataStart(), old_data.GetDataStart(),
- old_data.GetByteSize()) == 0)
- return false; // Value has not changed, user requested modify watchpoint
-
- return true;
-}
-
// RETURNS - true if we should stop at this breakpoint, false if we
// should continue.
bool Watchpoint::ShouldStop(StoppointCallbackContext *context) {
- m_hit_counter.Increment();
-
return IsEnabled();
}
diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp
index 7fa1fc5d71f13..586421c9daa6d 100644
--- a/lldb/source/Target/StopInfo.cpp
+++ b/lldb/source/Target/StopInfo.cpp
@@ -28,6 +28,8 @@
#include "lldb/Utility/StreamString.h"
#include "lldb/ValueObject/ValueObject.h"
+#include "llvm/ADT/ScopeExit.h"
+
using namespace lldb;
using namespace lldb_private;
@@ -704,6 +706,16 @@ class StopInfoBreakpoint : public StopInfo {
// StopInfoWatchpoint
+static void ReportWatchpointHit(const ExecutionContext &exe_ctx,
+ lldb::WatchpointSP wp_sp) {
+ Debugger &debugger = exe_ctx.GetTargetRef().GetDebugger();
+ StreamSP output_sp = debugger.GetAsyncOutputStream();
+ if (wp_sp->DumpSnapshots(output_sp.get())) {
+ output_sp->EOL();
+ output_sp->Flush();
+ }
+}
+
class StopInfoWatchpoint : public StopInfo {
public:
// Make sure watchpoint is properly disabled and subsequently enabled while
@@ -959,151 +971,55 @@ class StopInfoWatchpoint : public StopInfo {
}
void PerformAction(Event *event_ptr) override {
- Log *log = GetLog(LLDBLog::Watchpoints);
- // We're going to calculate if we should stop or not in some way during the
- // course of this code. Also by default we're going to stop, so set that
- // here.
- m_should_stop = true;
-
-
ThreadSP thread_sp(m_thread_wp.lock());
- if (thread_sp) {
-
- WatchpointSP wp_sp(
- thread_sp->CalculateTarget()->GetWatchpointList().FindByID(
- GetValue()));
- if (wp_sp) {
- // This sentry object makes sure the current watchpoint is disabled
- // while performing watchpoint actions, and it is then enabled after we
- // are finished.
- ExecutionContext exe_ctx(thread_sp->GetStackFrameAtIndex(0));
- ProcessSP process_sp = exe_ctx.GetProcessSP();
-
- WatchpointSentry sentry(process_sp, wp_sp);
-
- if (m_silently_skip_wp) {
- m_should_stop = false;
- wp_sp->UndoHitCount();
- }
-
- if (wp_sp->GetHitCount() <= wp_sp->GetIgnoreCount()) {
- m_should_stop = false;
- m_should_stop_is_valid = true;
- }
-
- Debugger &debugger = exe_ctx.GetTargetRef().GetDebugger();
-
- if (m_should_stop && wp_sp->GetConditionText() != nullptr) {
- // We need to make sure the user sees any parse errors in their
- // condition, so we'll hook the constructor errors up to the
- // debugger's Async I/O.
- ExpressionResults result_code;
- EvaluateExpressionOptions expr_options;
- expr_options.SetUnwindOnError(true);
- expr_options.SetIgnoreBreakpoints(true);
- ValueObjectSP result_value_sp;
- result_code = UserExpression::Evaluate(
- exe_ctx, expr_options, wp_sp->GetConditionText(),
- llvm::StringRef(), result_value_sp);
-
- if (result_code == eExpressionCompleted) {
- if (result_value_sp) {
- Scalar scalar_value;
- if (result_value_sp->ResolveValue(scalar_value)) {
- if (scalar_value.ULongLong(1) == 0) {
- // The condition failed, which we consider "not having hit
- // the watchpoint" so undo the hit count here.
- wp_sp->UndoHitCount();
- m_should_stop = false;
- } else
- m_should_stop = true;
- LLDB_LOGF(log,
- "Condition successfully evaluated, result is %s.\n",
- m_should_stop ? "true" : "false");
- } else {
- m_should_stop = true;
- LLDB_LOGF(
- log,
- "Failed to get an integer result from the expression.");
- }
- }
- } else {
- const char *err_str = "<unknown error>";
- if (result_value_sp)
- err_str = result_value_sp->GetError().AsCString();
-
- LLDB_LOGF(log, "Error evaluating condition: \"%s\"\n", err_str);
-
- StreamString strm;
- strm << "stopped due to an error evaluating condition of "
- "watchpoint ";
- wp_sp->GetDescription(&strm, eDescriptionLevelBrief);
- strm << ": \"" << wp_sp->GetConditionText() << "\"\n";
- strm << err_str;
-
- Debugger::ReportError(strm.GetString().str(),
- exe_ctx.GetTargetRef().GetDebugger().GetID());
- }
- }
-
- // If the condition says to stop, we run the callback to further decide
- // whether to stop.
- if (m_should_stop) {
- // FIXME: For now the callbacks have to run in async mode - the
- // first time we restart we need
- // to get out of there. So set it here.
- // When we figure out how to nest watchpoint hits then this will
- // change.
-
- bool old_async = debugger.GetAsyncExecution();
- debugger.SetAsyncExecution(true);
-
- StoppointCallbackContext context(event_ptr, exe_ctx, false);
- bool stop_requested = wp_sp->InvokeCallback(&context);
+ if (!thread_sp)
+ return;
- debugger.SetAsyncExecution(old_async);
+ auto Deferred = llvm::make_scope_exit([this]() {
+ Log *log = GetLog(LLDBLog::Watchpoints);
+ LLDB_LOGF(log,
+ "Process::%s returning from action with m_should_stop: %d.",
+ __FUNCTION__, m_should_stop);
+ m_should_stop_is_valid = true;
+ });
- // Also make sure that the callback hasn't continued the target. If
- // it did, when we'll set m_should_stop to false and get out of here.
- if (HasTargetRunSinceMe())
- m_should_stop = false;
+ WatchpointSP wp_sp(
+ thread_sp->CalculateTarget()->GetWatchpointList().FindByID(GetValue()));
+ if (!wp_sp) {
+ Log *log_process(GetLog(LLDBLog::Process));
- if (m_should_stop && !stop_requested) {
- // We have been vetoed by the callback mechanism.
- m_should_stop = false;
- }
- }
+ LLDB_LOGF(log_process,
+ "Process::%s could not find watchpoint id: %" PRId64 "...",
+ __FUNCTION__, m_value);
+ m_should_stop = true;
+ return;
+ }
- // Don't stop if the watched region value is unmodified, and
- // this is a Modify-type watchpoint.
- if (m_should_stop && !wp_sp->WatchedValueReportable(exe_ctx)) {
- wp_sp->UndoHitCount();
- m_should_stop = false;
- }
+ // We're going to calculate if we should stop or not in some way during the
+ // course of this code. Also by default we're not going to stop, so set
+ // that here.
+ m_should_stop = false;
- // Finally, if we are going to stop, print out the new & old values:
- if (m_should_stop) {
- wp_sp->CaptureWatchedValue(exe_ctx);
+ ExecutionContext exe_ctx(thread_sp->GetStackFrameAtIndex(0));
- Debugger &debugger = exe_ctx.GetTargetRef().GetDebugger();
- StreamUP output_up = debugger.GetAsyncOutputStream();
- if (wp_sp->DumpSnapshots(output_up.get()))
- output_up->EOL();
- }
+ // This sentry object makes sure the current watchpoint is disabled
+ // while performing watchpoint actions, and it is then enabled after we
+ // are finished.
+ WatchpointSentry sentry(exe_ctx.GetProcessSP(), wp_sp);
- } else {
- Log *log_process(GetLog(LLDBLog::Process));
+ // Hardware watchpoint may want to be skipped, so check it here.
+ if (m_silently_skip_wp)
+ return;
- LLDB_LOGF(log_process,
- "Process::%s could not find watchpoint id: %" PRId64 "...",
- __FUNCTION__, m_value);
- }
- LLDB_LOGF(log,
- "Process::%s returning from action with m_should_stop: %d.",
- __FUNCTION__, m_should_stop);
+ // Check watchpoint condition, ignore count and other staff related to
+ // watchpoint here.
+ StoppointCallbackContext context(event_ptr, exe_ctx, false);
+ if (!wp_sp->ShouldReport(context))
+ return;
- m_should_stop_is_valid = true;
- }
+ // Finally, if we are going to stop, print out the new & old values:
+ m_should_stop = true;
+ ReportWatchpointHit(exe_ctx, wp_sp);
}
private:
@@ -1111,7 +1027,7 @@ class StopInfoWatchpoint : public StopInfo {
assert(m_using_step_over_plan);
m_step_over_plan_complete = true;
}
-
+
bool m_should_stop = false;
bool m_should_stop_is_valid = false;
// A false watchpoint hit has happened -
``````````
</details>
https://github.com/llvm/llvm-project/pull/163696
More information about the llvm-branch-commits
mailing list