[lldb][NFC] Refactor StopInfoWatchpoint::PerformAction

[dlav-sc]

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.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: None (dlav-sc)

Changes

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 --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 -