https://github.com/dlav-sc created https://github.com/llvm/llvm-project/pull/163696
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. >From b35eaf0aa8aa9b4de19ca88b101ba950e0b91b0e Mon Sep 17 00:00:00 2001 From: Daniil Avdeev <[email protected]> Date: Mon, 10 Feb 2025 03:16:50 +0000 Subject: [PATCH] [lldb][NFC] Refactor StopInfoWatchpoint::PerformAction 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. --- lldb/include/lldb/Breakpoint/Watchpoint.h | 5 - lldb/source/Breakpoint/Watchpoint.cpp | 56 ------- lldb/source/Target/StopInfo.cpp | 188 ++++++---------------- 3 files changed, 52 insertions(+), 197 deletions(-) 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 - _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
