[Lldb-commits] [PATCH] D84257: [lldb] Don't use hardware index to determine whether a breakpoint site is hardware

[Jim Ingham via Phabricator via lldb-commits]

jingham added inline comments.


================
Comment at: lldb/source/Breakpoint/BreakpointLocation.cpp:73
+  if (m_bp_site_sp)
+    return m_bp_site_sp->IsHardware();
+
----------------
tatyana-krasnukha wrote:
> JDevlieghere wrote:
> > Should we sanity check that this is true when `m_hardware_index != LLDB_INVALID_INDEX32`? 
> > 
> > ```lldbassert(m_hardware_index == LLDB_INVALID_INDEX32 || m_bp_site_sp->IsHardware());```
> This is a good point. Though, should we check BreakpointLocation's m_hardware_index or BreakpointSite's m_hardware_index? Both of them inherit this member from StoppointLocation. To be honest, I don't think that BreakpointLocation should have software/hardware trait at all.
I hadn't noticed that StoppointLocation had picked up the m_hardware_index, that seems the wrong place for it.  StoppointLocation is a shared base class between BreakpointLocations, which don't know how the breakpoint got set, and BreakpointSite's which do.  Seems like StoppointLocation picked up some features really proper to BreakpointSites.

The BreakpointLocation does need to say whether it requested a hardware breakpoint or not (StoppointLocation::m_hardware).  But the location can't know whether it IS hardware or not.

For instance, I could set two breakpoints that both have a location pointing to the same address, one requesting Hardware and one not.  In that case, I would hope the two Locations would resolve to a single breakpoint site that is hardware.  It would be silly to do otherwise.  But that means the second location could reasonably think it was a software breakpoint, but it would be wrong.

Seems to me m_hardware_index should move out of StoppointLocation and into BreakpointSite.  You'll also have to move the field to Watchpoint as well - it doesn't have a notion of locations and sites and so was relying on StoppointLocation to store the hardware index.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84257/new/

https://reviews.llvm.org/D84257