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

[Tatyana Krasnukha via Phabricator via lldb-commits]

tatyana-krasnukha added a comment.





================
Comment at: lldb/source/Breakpoint/BreakpointLocation.cpp:73
+  if (m_bp_site_sp)
+    return m_bp_site_sp->IsHardware();
+
----------------
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.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:3208
     case BreakpointSite::eExternal: {
-      GDBStoppointType stoppoint_type;
-      if (bp_site->IsHardware())
-        stoppoint_type = eBreakpointHardware;
-      else
-        stoppoint_type = eBreakpointSoftware;
-
-      if (m_gdb_comm.SendGDBStoppointTypePacket(stoppoint_type, false, addr,
-                                                bp_op_size))
+      if (m_gdb_comm.SendGDBStoppointTypePacket(eBreakpointSoftware, false,
+                                                addr, bp_op_size))
----------------
>>! 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84257