[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.

lldb/include/lldb/Breakpoint/BreakpointLocation.h

public:
   void SetEnabled(bool enabled);
 
   /// Check the Enable/Disable state.
   ///
   /// \return
   ///     \b true if the breakpoint is enabled, \b false if disabled.
   bool IsEnabled() const;
 
+  /// Check if it is a hardware breakpoint.
+  ///
+  /// \return
+  ///     \b true if the breakpoint is a harware breakpoint.
+  bool IsHardware() const override;
+
   /// If \a auto_continue is \b true, set the breakpoint to continue when hit.
   void SetAutoContinue(bool auto_continue);
 
   /// Check the AutoContinue state.
   ///
   /// \return
   ///     \b true if the breakpoint is set to auto-continue, \b false if not.
   bool IsAutoContinue() const;

lldb/include/lldb/Breakpoint/BreakpointSite.h

 #define LLDB_BREAKPOINT_BREAKPOINTSITE_H
 
 #include <list>
 #include <mutex>
 
 
 #include "lldb/Breakpoint/BreakpointLocationCollection.h"
 #include "lldb/Breakpoint/StoppointLocation.h"
+#include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/UserID.h"
 #include "lldb/lldb-forward.h"
 
 namespace lldb_private {
 
 /// \class BreakpointSite BreakpointSite.h "lldb/Breakpoint/BreakpointSite.h"
 /// Class that manages the actual breakpoint that will be inserted into the
 /// running program.
public:
   /// Tell whether ALL the breakpoints in the location collection are
   /// internal.
   ///
   /// \result
   ///     \b true if all breakpoint locations are owned by internal breakpoints,
   ///     \b false otherwise.
   bool IsInternal() const;
 
+  bool IsHardware() const override {
+    lldbassert(BreakpointSite::Type::eHardware == GetType() ||
+               !HardwareRequired());
+    return BreakpointSite::Type::eHardware == GetType();
+  }
+
   BreakpointSite::Type GetType() const { return m_type; }
 
   void SetType(BreakpointSite::Type type) { m_type = type; }
 
 private:
   friend class Process;
   friend class BreakpointLocation;
   // The StopInfoBreakpoint knows when it is processing a hit for a thread for

lldb/include/lldb/Breakpoint/StoppointLocation.h

public:
   uint32_t GetByteSize() const { return m_byte_size; }
 
   uint32_t GetHitCount() const { return m_hit_count; }
 
   uint32_t GetHardwareIndex() const { return m_hardware_index; }
 
   bool HardwareRequired() const { return m_hardware; }
 
-  virtual bool IsHardware() const {
-    return m_hardware_index != LLDB_INVALID_INDEX32;
-  }
+  virtual bool IsHardware() const = 0;
 
   virtual bool ShouldStop(StoppointCallbackContext *context) { return true; }
 
   virtual void Dump(Stream *stream) const {}
 
   virtual void SetHardwareIndex(uint32_t index) { m_hardware_index = index; }
 
   lldb::break_id_t GetID() const { return m_loc_id; }

lldb/source/Breakpoint/BreakpointLocation.cpp

 }
 
 Address &BreakpointLocation::GetAddress() { return m_address; }
 
 Breakpoint &BreakpointLocation::GetBreakpoint() { return m_owner; }
 
 Target &BreakpointLocation::GetTarget() { return m_owner.GetTarget(); }
 
+bool BreakpointLocation::IsHardware() const {
+  if (m_bp_site_sp)
+    return m_bp_site_sp->IsHardware();
+
+  // If breakpoint location is not resolved yet, it cannot be hardware.
+  return false;
+}
+
 bool BreakpointLocation::IsEnabled() const {
   if (!m_owner.IsEnabled())
     return false;
   else if (m_options_up != nullptr)
     return m_options_up->IsEnabled();
   else
     return true;
 }

lldb/source/Breakpoint/Watchpoint.cpp

 std::string Watchpoint::GetWatchSpec() { return m_watch_spec_str; }
 
 void Watchpoint::SetWatchSpec(const std::string &str) {
   m_watch_spec_str = str;
 }
 
 // Override default impl of StoppointLocation::IsHardware() since m_is_hardware
 // member field is more accurate.
-bool Watchpoint::IsHardware() const { return m_is_hardware; }
+bool Watchpoint::IsHardware() const {
+  lldbassert(m_is_hardware || !HardwareRequired());
+  return m_is_hardware;
+}
 
 bool Watchpoint::IsWatchVariable() const { return m_is_watch_variable; }
 
 void Watchpoint::SetWatchVariable(bool val) { m_is_watch_variable = val; }
 
 bool Watchpoint::CaptureWatchedValue(const ExecutionContext &exe_ctx) {
   ConstString watch_name("$__lldb__watch_value");
   m_old_value_sp = m_new_value_sp;

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

if (bp_site->IsEnabled()) {
 
     case BreakpointSite::eHardware:
       if (m_gdb_comm.SendGDBStoppointTypePacket(eBreakpointHardware, false,
                                                 addr, bp_op_size))
         error.SetErrorToGenericError();
       break;
 
     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))
         error.SetErrorToGenericError();
     } break;
     }
     if (error.Success())
       bp_site->SetEnabled(false);
   } else {
     LLDB_LOGF(log,
               "ProcessGDBRemote::DisableBreakpointSite (site_id = %" PRIu64

lldb/test/API/functionalities/breakpoint/hardware_breakpoints/hardware_breakpoint_on_multiple_threads/TestHWBreakMultiThread.py

 from functionalities.breakpoint.hardware_breakpoints.base import *
 
 class HardwareBreakpointMultiThreadTestCase(HardwareBreakpointTestBase):
     mydir = TestBase.compute_mydir(__file__)
 
     def does_not_support_hw_breakpoints(self):
         return not super().supports_hw_breakpoints()
 
-    # LLDB on linux supports hardware breakpoints for arm and aarch64
-    # architectures.
-    @skipUnlessPlatform(oslist=['linux'])
-    @skipTestIfFn(does_not_support_hw_breakpoints)
-    def test_hw_break_set_delete_multi_thread_linux(self):
-        self.build()
-        self.setTearDownCleanup()
-        self.break_multi_thread('delete', False) # llvm.org/PR44659
-
-    # LLDB on linux supports hardware breakpoints for arm and aarch64
-    # architectures.
-    @skipUnlessPlatform(oslist=['linux'])
-    @skipTestIfFn(does_not_support_hw_breakpoints)
-    def test_hw_break_set_disable_multi_thread_linux(self):
-        self.build()
-        self.setTearDownCleanup()
-        self.break_multi_thread('disable', False) # llvm.org/PR44659
-
-    # LLDB on darwin supports hardware breakpoints for x86_64 and i386
-    # architectures.
-    @skipUnlessDarwin
     @skipIfOutOfTreeDebugserver
     @skipTestIfFn(does_not_support_hw_breakpoints)
     def test_hw_break_set_delete_multi_thread_macos(self):
         self.build()
         self.setTearDownCleanup()
         self.break_multi_thread('delete')
 
-    # LLDB on darwin supports hardware breakpoints for x86_64 and i386
-    # architectures.
-    @skipUnlessDarwin
     @skipIfOutOfTreeDebugserver
     @skipTestIfFn(does_not_support_hw_breakpoints)
     def test_hw_break_set_disable_multi_thread_macos(self):
         self.build()
         self.setTearDownCleanup()
         self.break_multi_thread('disable')
 
-
     def setUp(self):
         # Call super's setUp().
         TestBase.setUp(self)
         # Our simple source filename.
         self.source = 'main.cpp'
         # Find the line number to break inside main().
         self.first_stop = line_number(
             self.source, 'Starting thread creation with hardware breakpoint set')
 
-    def break_multi_thread(self, removal_type, check_hw_bp=True):
+    def break_multi_thread(self, removal_type):
         """Test that lldb hardware breakpoints work for multiple threads."""
         self.runCmd("file " + self.getBuildArtifact("a.out"),
                     CURRENT_EXECUTABLE_SET)
 
         # Stop in main before creating any threads.
         lldbutil.run_break_set_by_file_and_line(
             self, None, self.first_stop, num_expected_locations=1)
 
def break_multi_thread(self, removal_type):
                 substrs=[
                     'hw_break_function',
                     'stop reason = breakpoint',
                 ])
 
             # Continue the loop and test that we are stopped 4 times.
             count += 1
 
-        if check_hw_bp:
-            # Check the breakpoint list.
-            self.expect("breakpoint list", substrs=['hw_break_function', 'hardware'])
-            self.expect("breakpoint list -v", substrs=['function = hw_break_function', 'hardware = true'])
 
         if removal_type == 'delete':
             self.runCmd("settings set auto-confirm true")
 
             # Now 'breakpoint delete' should just work fine without confirmation
             # prompt from the command interpreter.
             self.expect("breakpoint delete",
                         startstr="All breakpoints removed")