[lldb] Don't use hardware index to determine whether a breakpoint site is hardware

[tatyana-krasnukha]

Summary

Most process plugins (if not all) don't set hardware index for breakpoints. They even are not able to determine this index.
This patch makes StoppointLocation::IsHardware pure virtual and lets BreakpointSite override it using more accurate BreakpointSite::Type.

It also adds assertions to be sure that a breakpoint site is hardware when this is required.

[tatyana-krasnukha]

Probably fixes llvm.org/PR44659, though I cannot check on arm/aarch64.

[JDevlieghere]

lldb/source/Breakpoint/BreakpointLocation.cpp

73

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());

[labath]

Could you clarify what's the functional change here? Does this change the kind of breakpoint that lldb sets, or just how it gets reported? E.g., does the ProcessGDBRemote acutally cause lldb to send a different packet, or is that just reacting to the change in the semantics of other code?

[labath]

In D84257#2164967, @tatyana-krasnukha wrote:

Probably fixes llvm.org/PR44659, though I cannot check on arm/aarch64.

@omjavaid might be interested in checking that.

[tatyana-krasnukha]

lldb/source/Breakpoint/BreakpointLocation.cpp

73

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.

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

3207–3208

In D84257#2166081, @labath wrote:

Could you clarify what's the functional change here? Does this change the kind of breakpoint that lldb sets, or just how it gets reported? E.g., does the ProcessGDBRemote acutally cause lldb to send a different packet, or is that just reacting to the change in the semantics of other code?

In the ProcessGDBRemote::EnableBreakpointSite (lines 3098 - 3103) it always creates external breakpoint with eBreakpointSoftware, so nothing is going to be changed in ProcessGDBRemote behavior. And now, when bp_site->IsHardware() checks whether BreakpontSite::Type is BreakpointSite::eHardware , the condition if (bp_site->IsHardware()) doesn't make sense here anymore.

[jingham]

lldb/source/Breakpoint/BreakpointLocation.cpp

73

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.

[omjavaid]

In D84257#2166215, @labath wrote:

In D84257#2164967, @tatyana-krasnukha wrote:

Probably fixes llvm.org/PR44659, though I cannot check on arm/aarch64.

@omjavaid might be interested in checking that.

Yes, I have verified this also fixes llvm.org/PR44659

[tatyana-krasnukha]

@omjavaid, thank you for verifying this!

Added the checks that IsHardware is consistent with m_hardware_index.

[tatyana-krasnukha]

I went further than just moving m_hardware_index from StoppointLocation. Please, take a look at D84527 and let me know what you think about that.

[tatyana-krasnukha]

Ups... Pushed it occationaly with the other patchs.