Remove hardware index from watchpoints and breakpoints (#72012)

The Watchpoint and Breakpoint objects try to track the hardware index
that was used for them, if they are hardware wp/bp's. The majority of
our debugging goes over the gdb remote serial protocol, and when we set
the watchpoint/breakpoint, there is no (standard) way for the remote
stub to communicate to lldb which hardware index was used. We have an
lldb-extension packet to query the total number of watchpoint registers.

When a watchpoint is hit, there is an lldb extension to the stop reply
packet (documented in lldb-gdb-remote.txt) to describe the watchpoint
including its actual hardware index,

<addr within wp range> <wp hw index> <actual accessed address>

(the third field is specifically needed for MIPS). At this point, if the
stub reported these three fields (the stub is only required to provide
the first), we can know the actual hardware index for this watchpoint.

Breakpoints are worse; there's never any way for us to be notified about
which hardware index was used. Breakpoints got this as a side effect of
inherting from StoppointSite with Watchpoints.

We expose the watchpoint hardware index through "watchpoint list -v" and
through SBWatchpoint::GetHardwareIndex.

With my large watchpoint support, there is no *single* hardware index
that may be used for a watchpoint, it may need multiple resources. Also
I don't see what a user is supposed to do with this information, or an
IDE. Knowing the total number of watchpoint registers on the target, and
knowing how many Watchpoint Resources are currently in use, is helpful.
Knowing how many Watchpoint Resources
a single user-specified watchpoint needed to be implemented is useful.
But knowing which registers were used is an implementation detail and
not available until we hit the watchpoint when using gdb remote serial
protocol.

So given all that, I'm removing watchpoint hardware index numbers. I'm
changing the SB API to always return -1.

Author: jasonmolenda

lldb/bindings/interface/SBTargetDocstrings.i

@@ -34,7 +34,7 @@ produces: ::
 
     Watchpoint 1: addr = 0x1034ca048 size = 4 state = enabled type = rw
         declare @ '/Volumes/data/lldb/svn/trunk/test/python_api/watchpoint/main.c:12'
-        hw_index = 0  hit_count = 2     ignore_count = 0"
+        hit_count = 2     ignore_count = 0"
 ) lldb::SBTarget;
 
 %feature("docstring", "

lldb/bindings/interface/SBWatchpointDocstrings.i

@@ -9,7 +9,8 @@ watchpoints of the target."
 ) lldb::SBWatchpoint;
 
 %feature("docstring", "
-    With -1 representing an invalid hardware index."
+    Deprecated.  Previously: Return the hardware index of the 
+    watchpoint register.  Now: -1 is always returned."
 ) lldb::SBWatchpoint::GetHardwareIndex;
 
 %feature("docstring", "

lldb/docs/use/tutorial.rst

@@ -431,7 +431,7 @@ a variable called 'global' for write operation, but only stop if the condition
    Watchpoint 1: addr = 0x100001018 size = 4 state = enabled type = w
       declare @ '/Volumes/data/lldb/svn/ToT/test/functionalities/watchpoint/watchpoint_commands/condition/main.cpp:12'
       condition = '(global==5)'
-      hw_index = 0  hit_count = 5     ignore_count = 0
+      hit_count = 5     ignore_count = 0
    (lldb)
 
 Starting or Attaching to Your Program

lldb/include/lldb/API/SBWatchpoint.h

@@ -45,7 +45,7 @@ class LLDB_API SBWatchpoint {
 
   watch_id_t GetID();
 
-  /// With -1 representing an invalid hardware index.
+  LLDB_DEPRECATED("Hardware index is not available, always returns -1")
   int32_t GetHardwareIndex();
 
   lldb::addr_t GetWatchAddress();

lldb/include/lldb/Breakpoint/StoppointSite.h

@@ -38,10 +38,6 @@ class StoppointSite {
 
   virtual bool IsHardware() const = 0;
 
-  uint32_t GetHardwareIndex() const { return m_hardware_index; }
-
-  void SetHardwareIndex(uint32_t index) { m_hardware_index = index; }
-
   virtual bool ShouldStop(StoppointCallbackContext* context) = 0;
 
   virtual void Dump(Stream* stream) const = 0;
@@ -59,9 +55,6 @@ class StoppointSite {
   /// the lack of resources).
   bool m_is_hardware_required;
 
-  /// The hardware resource index for this breakpoint/watchpoint.
-  uint32_t m_hardware_index;
-
   /// The size in bytes of stoppoint, e.g. the length of the trap opcode for
   /// software breakpoints, or the optional length in bytes for hardware
   /// breakpoints, or the length of the watchpoint.

lldb/source/API/SBWatchpoint.cpp

@@ -94,16 +94,11 @@ SBError SBWatchpoint::GetError() {
 int32_t SBWatchpoint::GetHardwareIndex() {
   LLDB_INSTRUMENT_VA(this);
 
-  int32_t hw_index = -1;
-
-  lldb::WatchpointSP watchpoint_sp(GetSP());
-  if (watchpoint_sp) {
-    std::lock_guard<std::recursive_mutex> guard(
-        watchpoint_sp->GetTarget().GetAPIMutex());
-    hw_index = watchpoint_sp->GetHardwareIndex();
-  }
-
-  return hw_index;
+  // For processes using gdb remote protocol,
+  // we cannot determine the hardware breakpoint
+  // index reliably; providing possibly correct
+  // guesses is not useful to anyone.
+  return -1;
 }
 
 addr_t SBWatchpoint::GetWatchAddress() {

lldb/source/Breakpoint/BreakpointLocation.cpp

@@ -626,22 +626,19 @@ void BreakpointLocation::Dump(Stream *s) const {
 
   bool is_resolved = IsResolved();
   bool is_hardware = is_resolved && m_bp_site_sp->IsHardware();
-  auto hardware_index = is_resolved ?
-      m_bp_site_sp->GetHardwareIndex() : LLDB_INVALID_INDEX32;
 
   lldb::tid_t tid = GetOptionsSpecifyingKind(BreakpointOptions::eThreadSpec)
                         .GetThreadSpecNoCreate()
                         ->GetTID();
   s->Printf("BreakpointLocation %u: tid = %4.4" PRIx64
             "  load addr = 0x%8.8" PRIx64 "  state = %s  type = %s breakpoint  "
-            "hw_index = %i  hit_count = %-4u  ignore_count = %-4u",
+            "hit_count = %-4u  ignore_count = %-4u",
             GetID(), tid,
             (uint64_t)m_address.GetOpcodeLoadAddress(&m_owner.GetTarget()),
             (m_options_up ? m_options_up->IsEnabled() : m_owner.IsEnabled())
                 ? "enabled "
                 : "disabled",
-            is_hardware ? "hardware" : "software", hardware_index,
-            GetHitCount(),
+            is_hardware ? "hardware" : "software", GetHitCount(),
             GetOptionsSpecifyingKind(BreakpointOptions::eIgnoreCount)
                 .GetIgnoreCount());
 }

lldb/source/Breakpoint/BreakpointSite.cpp

@@ -76,9 +76,9 @@ void BreakpointSite::Dump(Stream *s) const {
     return;
 
   s->Printf("BreakpointSite %u: addr = 0x%8.8" PRIx64
-            "  type = %s breakpoint  hw_index = %i  hit_count = %-4u",
+            "  type = %s breakpoint  hit_count = %-4u",
             GetID(), (uint64_t)m_addr, IsHardware() ? "hardware" : "software",
-            GetHardwareIndex(), GetHitCount());
+            GetHitCount());
 }
 
 void BreakpointSite::GetDescription(Stream *s, lldb::DescriptionLevel level) {

lldb/source/Breakpoint/StoppointSite.cpp

@@ -13,11 +13,10 @@ using namespace lldb;
 using namespace lldb_private;
 
 StoppointSite::StoppointSite(break_id_t id, addr_t addr, bool hardware)
-    : m_id(id), m_addr(addr), m_is_hardware_required(hardware),
-      m_hardware_index(LLDB_INVALID_INDEX32), m_byte_size(0), m_hit_counter() {}
+    : m_id(id), m_addr(addr), m_is_hardware_required(hardware), m_byte_size(0),
+      m_hit_counter() {}
 
-StoppointSite::StoppointSite(break_id_t id, addr_t addr,
-                             uint32_t byte_size, bool hardware)
+StoppointSite::StoppointSite(break_id_t id, addr_t addr, uint32_t byte_size,
+                             bool hardware)
     : m_id(id), m_addr(addr), m_is_hardware_required(hardware),
-      m_hardware_index(LLDB_INVALID_INDEX32), m_byte_size(byte_size),
-      m_hit_counter() {}
+      m_byte_size(byte_size), m_hit_counter() {}

lldb/source/Breakpoint/Watchpoint.cpp

@@ -324,8 +324,8 @@ void Watchpoint::DumpWithLevel(Stream *s,
   }
 
   if (description_level >= lldb::eDescriptionLevelVerbose) {
-    s->Printf("\n    hw_index = %i  hit_count = %-4u  ignore_count = %-4u",
-              GetHardwareIndex(), GetHitCount(), GetIgnoreCount());
+    s->Printf("\n    hit_count = %-4u  ignore_count = %-4u", GetHitCount(),
+              GetIgnoreCount());
   }
 }
 
@@ -350,9 +350,7 @@ bool Watchpoint::IsDisabledDuringEphemeralMode() {
 
 void Watchpoint::SetEnabled(bool enabled, bool notify) {
   if (!enabled) {
-    if (!m_is_ephemeral)
-      SetHardwareIndex(LLDB_INVALID_INDEX32);
-    else
+    if (m_is_ephemeral)
       ++m_disabled_count;
 
     // Don't clear the snapshots for now.

lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp

@@ -495,10 +495,6 @@ static StopInfoSP GetStopInfoForHardwareBP(Thread &thread, Target *target,
     lldb::WatchpointSP wp_sp =
         target->GetWatchpointList().FindByAddress((lldb::addr_t)exc_sub_code);
     if (wp_sp && wp_sp->IsEnabled()) {
-      // Debugserver may piggyback the hardware index of the fired watchpoint
-      // in the exception data. Set the hardware index if that's the case.
-      if (exc_data_count >= 3)
-        wp_sp->SetHardwareIndex((uint32_t)exc_sub_sub_code);
       return StopInfo::CreateStopReasonWithWatchpointID(thread, wp_sp->GetID());
     }
   }
@@ -511,10 +507,6 @@ static StopInfoSP GetStopInfoForHardwareBP(Thread &thread, Target *target,
         process_sp->GetBreakpointSiteList().FindByAddress(
             (lldb::addr_t)exc_sub_code);
     if (bp_sp && bp_sp->IsEnabled()) {
-      // Debugserver may piggyback the hardware index of the fired breakpoint
-      // in the exception data. Set the hardware index if that's the case.
-      if (exc_data_count >= 3)
-        bp_sp->SetHardwareIndex((uint32_t)exc_sub_sub_code);
       return StopInfo::CreateStopReasonWithBreakpointSiteID(thread,
                                                             bp_sp->GetID());
     }
@@ -681,11 +673,6 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException(
           wp_sp = target->GetWatchpointList().FindByAddress(
               (lldb::addr_t)exc_sub_code);
         if (wp_sp && wp_sp->IsEnabled()) {
-          // Debugserver may piggyback the hardware index of the fired
-          // watchpoint in the exception data. Set the hardware index if
-          // that's the case.
-          if (exc_data_count >= 3)
-            wp_sp->SetHardwareIndex((uint32_t)exc_sub_sub_code);
           return StopInfo::CreateStopReasonWithWatchpointID(thread,
                                                             wp_sp->GetID());
         } else {
@@ -762,11 +749,6 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException(
           wp_sp = target->GetWatchpointList().FindByAddress(
               (lldb::addr_t)exc_sub_code);
         if (wp_sp && wp_sp->IsEnabled()) {
-          // Debugserver may piggyback the hardware index of the fired
-          // watchpoint in the exception data. Set the hardware index if
-          // that's the case.
-          if (exc_data_count >= 3)
-            wp_sp->SetHardwareIndex((uint32_t)exc_sub_sub_code);
           return StopInfo::CreateStopReasonWithWatchpointID(thread,
                                                             wp_sp->GetID());
         }

lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp

@@ -405,10 +405,6 @@ void ProcessWindows::RefreshStateAfterStop() {
                "{1:x} with watchpoint {2}",
                m_session_data->m_debugger->GetProcess().GetProcessId(), pc, id);
 
-      if (lldb::WatchpointSP wp_sp =
-              GetTarget().GetWatchpointList().FindByID(id))
-        wp_sp->SetHardwareIndex(slot_id);
-
       stop_info = StopInfo::CreateStopReasonWithWatchpointID(
           *stop_thread, id, m_watchpoints[id].address);
       stop_thread->SetStopInfo(stop_info);

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

@@ -1789,8 +1789,12 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
           // disable/step/re-enable it, so one of the valid watchpoint
           // addresses should be provided as \a wp_addr.
           StringExtractor desc_extractor(description.c_str());
+          // FIXME NativeThreadLinux::SetStoppedByWatchpoint sends this
+          // up as
+          //  <address within wp range> <wp hw index> <actual accessed addr>
+          // but this is not reading the <wp hw index>.  Seems like it
+          // wouldn't work on MIPS, where that third field is important.
           addr_t wp_addr = desc_extractor.GetU64(LLDB_INVALID_ADDRESS);
-          uint32_t wp_index = desc_extractor.GetU32(LLDB_INVALID_INDEX32);
           addr_t wp_hit_addr = desc_extractor.GetU64(LLDB_INVALID_ADDRESS);
           watch_id_t watch_id = LLDB_INVALID_WATCH_ID;
           bool silently_continue = false;
@@ -1807,7 +1811,6 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
           if (!wp_sp && wp_addr != LLDB_INVALID_ADDRESS)
             wp_sp = GetTarget().GetWatchpointList().FindByAddress(wp_addr);
           if (wp_sp) {
-            wp_sp->SetHardwareIndex(wp_index);
             watch_id = wp_sp->GetID();
           }
           if (watch_id == LLDB_INVALID_WATCH_ID) {
@@ -2238,17 +2241,13 @@ StateType ProcessGDBRemote::SetThreadStopInfo(StringExtractor &stop_packet) {
 
         WatchpointSP wp_sp =
             GetTarget().GetWatchpointList().FindByAddress(wp_addr);
-        uint32_t wp_index = LLDB_INVALID_INDEX32;
-
-        if (wp_sp)
-          wp_index = wp_sp->GetHardwareIndex();
 
         // Rewrite gdb standard watch/rwatch/awatch to
         // "reason:watchpoint" + "description:ADDR",
         // which is parsed in SetThreadStopInfo.
         reason = "watchpoint";
         StreamString ostr;
-        ostr.Printf("%" PRIu64 " %" PRIu32, wp_addr, wp_index);
+        ostr.Printf("%" PRIu64, wp_addr);
         description = std::string(ostr.GetString());
       } else if (key.compare("library") == 0) {
         auto error = LoadModules();

lldb/source/Target/StopInfo.cpp

@@ -699,7 +699,6 @@ class StopInfoWatchpoint : public StopInfo {
                                     eVoteNoOpinion),
           m_stop_info_sp(stop_info_sp), m_watch_sp(watch_sp) {
       assert(watch_sp);
-      m_watch_index = watch_sp->GetHardwareIndex();
     }
 
     bool DoWillResume(lldb::StateType resume_state,
@@ -753,13 +752,11 @@ class StopInfoWatchpoint : public StopInfo {
         return;
       m_did_disable_wp = true;
       GetThread().GetProcess()->EnableWatchpoint(m_watch_sp.get(), true);
-      m_watch_sp->SetHardwareIndex(m_watch_index);
     }
 
   private:
     StopInfoWatchpointSP m_stop_info_sp;
     WatchpointSP m_watch_sp;
-    uint32_t m_watch_index = LLDB_INVALID_INDEX32;
     bool m_did_disable_wp = false;
   };
 

lldb/test/API/python_api/default-constructor/sb_watchpoint.py

@@ -8,7 +8,6 @@
 def fuzz_obj(obj):
     obj.GetID()
     obj.IsValid()
-    obj.GetHardwareIndex()
     obj.GetWatchAddress()
     obj.GetWatchSize()
     obj.SetEnabled(True)

lldb/test/API/python_api/watchpoint/TestWatchpointIter.py

@@ -83,20 +83,12 @@ def test_watch_iter(self):
         self.assertTrue(thread, "The thread stopped due to watchpoint")
         self.DebugSBValue(value)
 
-        # We currently only support hardware watchpoint.  Verify that we have a
-        # meaningful hardware index at this point.  Exercise the printed repr of
-        # SBWatchpointLocation.
-        print(watchpoint)
-        if not self.affected_by_radar_34564183():
-            self.assertTrue(watchpoint.GetHardwareIndex() != -1)
-
         # SBWatchpoint.GetDescription() takes a description level arg.
         print(lldbutil.get_description(watchpoint, lldb.eDescriptionLevelFull))
 
         # Now disable the 'rw' watchpoint.  The program won't stop when it reads
         # 'global' next.
         watchpoint.SetEnabled(False)
-        self.assertEqual(watchpoint.GetHardwareIndex(), -1)
         self.assertFalse(watchpoint.IsEnabled())
 
         # Continue.  The program does not stop again when the variable is being

llvm/docs/ReleaseNotes.rst

@@ -230,6 +230,8 @@ Changes to the LLVM tools
 Changes to LLDB
 ---------------------------------
 
+* ``SBWatchpoint::GetHardwareIndex`` is deprecated and now returns -1
+  to indicate the index is unavailable.
 * Methods in SBHostOS related to threads have had their implementations
   removed. These methods will return a value indicating failure.
 * ``SBType::FindDirectNestedType`` function is added. It's useful