[lldb] [mostly NFC] Large WP foundation: WatchpointResources (#68845)

This patch is rearranging code a bit to add WatchpointResources to
Process. A WatchpointResource is meant to represent a hardware
watchpoint register in the inferior process. It has an address, a size,
a type, and a list of Watchpoints that are using this
WatchpointResource.

This current patch doesn't add any of the features of
WatchpointResources that make them interesting -- a user asking to watch
a 24 byte object could watch this with three 8 byte WatchpointResources.
Or a Watchpoint on 1 byte at 0x1002 and a second watchpoint on 1 byte at
0x1003, these must both be served by a single WatchpointResource on that
doubleword at 0x1000 on a 64-bit target, if two hardware watchpoint
registers were used to track these separately, one of them may not be
hit. Or if you have one Watchpoint on a variable with a condition set,
and another Watchpoint on that same variable with a command defined or
different condition, or ignorecount, both of those Watchpoints need to
evaluate their criteria/commands when their WatchpointResource has been
hit.

There's a bit of code movement to rearrange things in the direction I'll
need for implementing this feature, so I want to start with reviewing &
landing this mostly NFC patch and we can focus on the algorithmic
choices about how WatchpointResources are shared and handled as they're
triggeed, separately.

This patch also stops printing "Watchpoint <n> hit: old value: <x>, new
vlaue: <y>" for Read watchpoints. I could make an argument for print
"Watchpoint <n> hit: current value <x>" but the current output doesn't
make any sense, and the user can print the value if they are
particularly interested. Read watchpoints are used primarily to
understand what code is reading a variable.

This patch adds more fallbacks for how to print the objects being
watched if we have types, instead of assuming they are all integral
values, so a struct will print its elements. As large watchpoints are
added, we'll be doing a lot more of those.

To track the WatchpointSP in the WatchpointResources, I changed the
internal API which took a WatchpointSP and devolved it to a Watchpoint*,
which meant touching several different Process files. I removed the
watchpoint code in ProcessKDP which only reported that watchpoints
aren't supported, the base class does that already.

I haven't yet changed how we receive a watchpoint to identify the
WatchpointResource responsible for the trigger, and identify all
Watchpoints that are using this Resource to evaluate their conditions
etc. This is the same work that a BreakpointSite needs to do when it has
been tiggered, where multiple Breakpoints may be at the same address.

There is not yet any printing of the Resources that a Watchpoint is
implemented in terms of ("watchpoint list", or
SBWatchpoint::GetDescription).

"watchpoint set var" and "watchpoint set expression" take a size
argument which was previously 1, 2, 4, or 8 (an enum). I've changed this
to an unsigned int. Most hardware implementations can only watch 1, 2,
4, 8 byte ranges, but with Resources we'll allow a user to ask for
different sized watchpoints and set them in hardware-expressble terms
soon.

I've annotated areas where I know there is work still needed with
LWP_TODO that I'll be working on once this is landed.

I've tested this on aarch64 macOS, aarch64 Linux, and Intel macOS.

https://discourse.llvm.org/t/rfc-large-watchpoint-support-in-lldb/72116
(cherry picked from commit fc6b72523f3d73b921690a713e97a433c96066c6)

1 files changed, 176 insertions, 94 deletions

diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index 9648f1f..fb67151 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -24,6 +24,7 @@
 #include <sys/types.h>
 
 #include "lldb/Breakpoint/Watchpoint.h"
+#include "lldb/Breakpoint/WatchpointResource.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
@@ -1798,24 +1799,29 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
           addr_t wp_hit_addr = desc_extractor.GetU64(LLDB_INVALID_ADDRESS);
           watch_id_t watch_id = LLDB_INVALID_WATCH_ID;
           bool silently_continue = false;
-          WatchpointSP wp_sp;
+          WatchpointResourceSP wp_resource_sp;
           if (wp_hit_addr != LLDB_INVALID_ADDRESS) {
-            wp_sp = GetTarget().GetWatchpointList().FindByAddress(wp_hit_addr);
+            wp_resource_sp =
+                m_watchpoint_resource_list.FindByAddress(wp_hit_addr);
             // On MIPS, \a wp_hit_addr outside the range of a watched
             // region means we should silently continue, it is a false hit.
             ArchSpec::Core core = GetTarget().GetArchitecture().GetCore();
-            if (!wp_sp && core >= ArchSpec::kCore_mips_first &&
+            if (!wp_resource_sp && core >= ArchSpec::kCore_mips_first &&
                 core <= ArchSpec::kCore_mips_last)
               silently_continue = true;
           }
-          if (!wp_sp && wp_addr != LLDB_INVALID_ADDRESS)
-            wp_sp = GetTarget().GetWatchpointList().FindByAddress(wp_addr);
-          if (wp_sp) {
-            watch_id = wp_sp->GetID();
-          }
-          if (watch_id == LLDB_INVALID_WATCH_ID) {
+          if (!wp_resource_sp && wp_addr != LLDB_INVALID_ADDRESS)
+            wp_resource_sp = m_watchpoint_resource_list.FindByAddress(wp_addr);
+          if (!wp_resource_sp) {
             Log *log(GetLog(GDBRLog::Watchpoints));
             LLDB_LOGF(log, "failed to find watchpoint");
+            watch_id = LLDB_INVALID_SITE_ID;
+          } else {
+            // LWP_TODO: This is hardcoding a single Watchpoint in a
+            // Resource, need to add
+            // StopInfo::CreateStopReasonWithWatchpointResource which
+            // represents all watchpoints that were tripped at this stop.
+            watch_id = wp_resource_sp->GetConstituentAtIndex(0)->GetID();
           }
           thread_sp->SetStopInfo(StopInfo::CreateStopReasonWithWatchpointID(
               *thread_sp, watch_id, silently_continue));
@@ -2239,8 +2245,8 @@ StateType ProcessGDBRemote::SetThreadStopInfo(StringExtractor &stop_packet) {
         lldb::addr_t wp_addr = LLDB_INVALID_ADDRESS;
         value.getAsInteger(16, wp_addr);
 
-        WatchpointSP wp_sp =
-            GetTarget().GetWatchpointList().FindByAddress(wp_addr);
+        WatchpointResourceSP wp_resource_sp =
+            m_watchpoint_resource_list.FindByAddress(wp_addr);
 
         // Rewrite gdb standard watch/rwatch/awatch to
         // "reason:watchpoint" + "description:ADDR",
@@ -3109,102 +3115,182 @@ Status ProcessGDBRemote::DisableBreakpointSite(BreakpointSite *bp_site) {
 }
 
 // Pre-requisite: wp != NULL.
-static GDBStoppointType GetGDBStoppointType(Watchpoint *wp) {
-  assert(wp);
-  bool watch_read = wp->WatchpointRead();
-  bool watch_write = wp->WatchpointWrite();
-  bool watch_modify = wp->WatchpointModify();
-
-  // watch_read, watch_write, watch_modify cannot all be false.
-  assert((watch_read || watch_write || watch_modify) &&
-         "watch_read, watch_write, watch_modify cannot all be false.");
-  if (watch_read && (watch_write || watch_modify))
+static GDBStoppointType
+GetGDBStoppointType(const WatchpointResourceSP &wp_res_sp) {
+  assert(wp_res_sp);
+  bool read = wp_res_sp->WatchpointResourceRead();
+  bool write = wp_res_sp->WatchpointResourceWrite();
+
+  assert((read || write) &&
+         "WatchpointResource type is neither read nor write");
+  if (read && write)
     return eWatchpointReadWrite;
-  else if (watch_read)
+  else if (read)
     return eWatchpointRead;
-  else // Must be watch_write or watch_modify, then.
+  else
     return eWatchpointWrite;
 }
 
-Status ProcessGDBRemote::EnableWatchpoint(Watchpoint *wp, bool notify) {
+Status ProcessGDBRemote::EnableWatchpoint(WatchpointSP wp_sp, bool notify) {
   Status error;
-  if (wp) {
-    user_id_t watchID = wp->GetID();
-    addr_t addr = wp->GetLoadAddress();
-    Log *log(GetLog(GDBRLog::Watchpoints));
-    LLDB_LOGF(log, "ProcessGDBRemote::EnableWatchpoint(watchID = %" PRIu64 ")",
-              watchID);
-    if (wp->IsEnabled()) {
-      LLDB_LOGF(log,
-                "ProcessGDBRemote::EnableWatchpoint(watchID = %" PRIu64
-                ") addr = 0x%8.8" PRIx64 ": watchpoint already enabled.",
-                watchID, (uint64_t)addr);
-      return error;
-    }
+  if (!wp_sp) {
+    error.SetErrorString("No watchpoint specified");
+    return error;
+  }
+  user_id_t watchID = wp_sp->GetID();
+  addr_t addr = wp_sp->GetLoadAddress();
+  Log *log(GetLog(GDBRLog::Watchpoints));
+  LLDB_LOGF(log, "ProcessGDBRemote::EnableWatchpoint(watchID = %" PRIu64 ")",
+            watchID);
+  if (wp_sp->IsEnabled()) {
+    LLDB_LOGF(log,
+              "ProcessGDBRemote::EnableWatchpoint(watchID = %" PRIu64
+              ") addr = 0x%8.8" PRIx64 ": watchpoint already enabled.",
+              watchID, (uint64_t)addr);
+    return error;
+  }
 
-    GDBStoppointType type = GetGDBStoppointType(wp);
-    // Pass down an appropriate z/Z packet...
-    if (m_gdb_comm.SupportsGDBStoppointPacket(type)) {
-      if (m_gdb_comm.SendGDBStoppointTypePacket(type, true, addr,
-                                                wp->GetByteSize(),
-                                                GetInterruptTimeout()) == 0) {
-        wp->SetEnabled(true, notify);
-        return error;
-      } else
-        error.SetErrorString("sending gdb watchpoint packet failed");
-    } else
-      error.SetErrorString("watchpoints not supported");
+  bool read = wp_sp->WatchpointRead();
+  bool write = wp_sp->WatchpointWrite() || wp_sp->WatchpointModify();
+  size_t size = wp_sp->GetByteSize();
+
+  // New WatchpointResources needed to implement this Watchpoint.
+  std::vector<WatchpointResourceSP> resources;
+
+  // LWP_TODO: Break up the user's request into pieces that can be watched
+  // given the capabilities of the target cpu / stub software.
+  // As a default, breaking the watched region up into target-pointer-sized,
+  // aligned, groups.
+  //
+  // Beyond the default, a stub can / should inform us of its capabilities,
+  // e.g. a stub that can do AArch64 power-of-2 MASK watchpoints.
+  //
+  // And the cpu may have unique capabilities. AArch64 BAS watchpoints
+  // can watch any sequential bytes in a doubleword, but Intel watchpoints
+  // can only watch 1, 2, 4, 8 bytes within a doubleword.
+  WatchpointResourceSP wp_res_sp =
+      std::make_shared<WatchpointResource>(addr, size, read, write);
+  resources.push_back(wp_res_sp);
+
+  // LWP_TODO: Now that we know the WP Resources needed to implement this
+  // Watchpoint, we need to look at currently allocated Resources in the
+  // Process and if they match, or are within the same memory granule, or
+  // overlapping memory ranges, then we need to combine them.  e.g. one
+  // Watchpoint watching 1 byte at 0x1002 and a second watchpoint watching 1
+  // byte at 0x1003, they must use the same hardware watchpoint register
+  // (Resource) to watch them.
+
+  // This may mean that an existing resource changes its type (read to
+  // read+write) or address range it is watching, in which case the old
+  // watchpoint needs to be disabled and the new Resource addr/size/type
+  // watchpoint enabled.
+
+  // If we modify a shared Resource to accomodate this newly added Watchpoint,
+  // and we are unable to set all of the Resources for it in the inferior, we
+  // will return an error for this Watchpoint and the shared Resource should
+  // be restored.  e.g. this Watchpoint requires three Resources, one which
+  // is shared with another Watchpoint.  We extend the shared Resouce to
+  // handle both Watchpoints and we try to set two new ones.  But if we don't
+  // have sufficient watchpoint register for all 3, we need to show an error
+  // for creating this Watchpoint and we should reset the shared Resource to
+  // its original configuration because it is no longer shared.
+
+  bool set_all_resources = true;
+  std::vector<WatchpointResourceSP> succesfully_set_resources;
+  for (const auto &wp_res_sp : resources) {
+    addr_t addr = wp_res_sp->GetLoadAddress();
+    size_t size = wp_res_sp->GetByteSize();
+    GDBStoppointType type = GetGDBStoppointType(wp_res_sp);
+    if (!m_gdb_comm.SupportsGDBStoppointPacket(type) ||
+        m_gdb_comm.SendGDBStoppointTypePacket(type, true, addr, size,
+                                              GetInterruptTimeout())) {
+      set_all_resources = false;
+      break;
+    } else {
+      succesfully_set_resources.push_back(wp_res_sp);
+    }
+  }
+  if (set_all_resources) {
+    wp_sp->SetEnabled(true, notify);
+    for (const auto &wp_res_sp : resources) {
+      // LWP_TODO: If we expanded/reused an existing Resource,
+      // it's already in the WatchpointResourceList.
+      wp_res_sp->AddConstituent(wp_sp);
+      m_watchpoint_resource_list.Add(wp_res_sp);
+    }
+    return error;
   } else {
-    error.SetErrorString("Watchpoint argument was NULL.");
+    // We failed to allocate one of the resources.  Unset all
+    // of the new resources we did successfully set in the
+    // process.
+    for (const auto &wp_res_sp : succesfully_set_resources) {
+      addr_t addr = wp_res_sp->GetLoadAddress();
+      size_t size = wp_res_sp->GetByteSize();
+      GDBStoppointType type = GetGDBStoppointType(wp_res_sp);
+      m_gdb_comm.SendGDBStoppointTypePacket(type, false, addr, size,
+                                            GetInterruptTimeout());
+    }
+    error.SetErrorString("Setting one of the watchpoint resources failed");
   }
-  if (error.Success())
-    error.SetErrorToGenericError();
   return error;
 }
 
-Status ProcessGDBRemote::DisableWatchpoint(Watchpoint *wp, bool notify) {
+Status ProcessGDBRemote::DisableWatchpoint(WatchpointSP wp_sp, bool notify) {
   Status error;
-  if (wp) {
-    user_id_t watchID = wp->GetID();
+  if (!wp_sp) {
+    error.SetErrorString("Watchpoint argument was NULL.");
+    return error;
+  }
+
+  user_id_t watchID = wp_sp->GetID();
 
-    Log *log(GetLog(GDBRLog::Watchpoints));
+  Log *log(GetLog(GDBRLog::Watchpoints));
 
-    addr_t addr = wp->GetLoadAddress();
+  addr_t addr = wp_sp->GetLoadAddress();
+
+  LLDB_LOGF(log,
+            "ProcessGDBRemote::DisableWatchpoint (watchID = %" PRIu64
+            ") addr = 0x%8.8" PRIx64,
+            watchID, (uint64_t)addr);
 
+  if (!wp_sp->IsEnabled()) {
     LLDB_LOGF(log,
               "ProcessGDBRemote::DisableWatchpoint (watchID = %" PRIu64
-              ") addr = 0x%8.8" PRIx64,
+              ") addr = 0x%8.8" PRIx64 " -- SUCCESS (already disabled)",
               watchID, (uint64_t)addr);
+    // See also 'class WatchpointSentry' within StopInfo.cpp. This disabling
+    // attempt might come from the user-supplied actions, we'll route it in
+    // order for the watchpoint object to intelligently process this action.
+    wp_sp->SetEnabled(false, notify);
+    return error;
+  }
 
-    if (!wp->IsEnabled()) {
-      LLDB_LOGF(log,
-                "ProcessGDBRemote::DisableWatchpoint (watchID = %" PRIu64
-                ") addr = 0x%8.8" PRIx64 " -- SUCCESS (already disabled)",
-                watchID, (uint64_t)addr);
-      // See also 'class WatchpointSentry' within StopInfo.cpp. This disabling
-      // attempt might come from the user-supplied actions, we'll route it in
-      // order for the watchpoint object to intelligently process this action.
-      wp->SetEnabled(false, notify);
-      return error;
-    }
+  if (wp_sp->IsHardware()) {
+    bool disabled_all = true;
 
-    if (wp->IsHardware()) {
-      GDBStoppointType type = GetGDBStoppointType(wp);
-      // Pass down an appropriate z/Z packet...
-      if (m_gdb_comm.SendGDBStoppointTypePacket(type, false, addr,
-                                                wp->GetByteSize(),
-                                                GetInterruptTimeout()) == 0) {
-        wp->SetEnabled(false, notify);
-        return error;
-      } else
-        error.SetErrorString("sending gdb watchpoint packet failed");
+    std::vector<WatchpointResourceSP> unused_resources;
+    for (const auto &wp_res_sp : m_watchpoint_resource_list.Sites()) {
+      if (wp_res_sp->ConstituentsContains(wp_sp)) {
+        GDBStoppointType type = GetGDBStoppointType(wp_res_sp);
+        addr_t addr = wp_res_sp->GetLoadAddress();
+        size_t size = wp_res_sp->GetByteSize();
+        if (m_gdb_comm.SendGDBStoppointTypePacket(type, false, addr, size,
+                                                  GetInterruptTimeout())) {
+          disabled_all = false;
+        } else {
+          wp_res_sp->RemoveConstituent(wp_sp);
+          if (wp_res_sp->GetNumberOfConstituents() == 0)
+            unused_resources.push_back(wp_res_sp);
+        }
+      }
     }
-    // TODO: clear software watchpoints if we implement them
-  } else {
-    error.SetErrorString("Watchpoint argument was NULL.");
+    for (auto &wp_res_sp : unused_resources)
+      m_watchpoint_resource_list.Remove(wp_res_sp->GetID());
+
+    wp_sp->SetEnabled(false, notify);
+    if (!disabled_all)
+      error.SetErrorString("Failure disabling one of the watchpoint locations");
   }
-  if (error.Success())
-    error.SetErrorToGenericError();
   return error;
 }
 
@@ -5456,16 +5542,12 @@ void ProcessGDBRemote::DidForkSwitchHardwareTraps(bool enable) {
     });
   }
 
-  WatchpointList &wps = GetTarget().GetWatchpointList();
-  size_t wp_count = wps.GetSize();
-  for (size_t i = 0; i < wp_count; ++i) {
-    WatchpointSP wp = wps.GetByIndex(i);
-    if (wp->IsEnabled()) {
-      GDBStoppointType type = GetGDBStoppointType(wp.get());
-      m_gdb_comm.SendGDBStoppointTypePacket(type, enable, wp->GetLoadAddress(),
-                                            wp->GetByteSize(),
-                                            GetInterruptTimeout());
-    }
+  for (const auto &wp_res_sp : m_watchpoint_resource_list.Sites()) {
+    addr_t addr = wp_res_sp->GetLoadAddress();
+    size_t size = wp_res_sp->GetByteSize();
+    GDBStoppointType type = GetGDBStoppointType(wp_res_sp);
+    m_gdb_comm.SendGDBStoppointTypePacket(type, true, addr, size,
+                                          GetInterruptTimeout());
   }
 }