Address Jim's feedback

Author: JDevlieghere

lldb/include/lldb/API/SBBreakpoint.h

@@ -148,7 +148,7 @@ class LLDB_API SBBreakpoint {
 
   bool IsHardware() const;
 
-  void SetIsHardware(bool is_hardware);
+  bool SetIsHardware(bool is_hardware);
 
   // Can only be called from a ScriptedBreakpointResolver...
   SBError

lldb/include/lldb/Breakpoint/Breakpoint.h

@@ -518,7 +518,8 @@ class Breakpoint : public std::enable_shared_from_this<Breakpoint>,
                       lldb::break_id_t bp_loc_id);
 
   bool IsHardware() const { return m_hardware; }
-  void SetIsHardware(bool is_hardware);
+
+  bool SetIsHardware(bool is_hardware);
 
   lldb::BreakpointResolverSP GetResolver() { return m_resolver_sp; }
 

lldb/include/lldb/Breakpoint/BreakpointLocation.h

@@ -69,7 +69,7 @@ class BreakpointLocation
   // The next section deals with various breakpoint options.
 
   /// If \a enabled is \b true, enable the breakpoint, if \b false disable it.
-  void SetEnabled(bool enabled);
+  bool SetEnabled(bool enabled);
 
   /// Check the Enable/Disable state.
   ///

lldb/source/API/SBBreakpoint.cpp

@@ -781,15 +781,16 @@ bool SBBreakpoint::IsHardware() const {
   return false;
 }
 
-void SBBreakpoint::SetIsHardware(bool is_hardware) {
+bool SBBreakpoint::SetIsHardware(bool is_hardware) {
   LLDB_INSTRUMENT_VA(this, is_hardware);
 
   BreakpointSP bkpt_sp = GetSP();
   if (bkpt_sp) {
     std::lock_guard<std::recursive_mutex> guard(
         bkpt_sp->GetTarget().GetAPIMutex());
-    bkpt_sp->SetIsHardware(is_hardware);
+    return bkpt_sp->SetIsHardware(is_hardware);
   }
+  return false;
 }
 
 BreakpointSP SBBreakpoint::GetSP() const { return m_opaque_wp.lock(); }

lldb/source/Breakpoint/Breakpoint.cpp

@@ -251,25 +251,34 @@ const lldb::TargetSP Breakpoint::GetTargetSP() {
 
 bool Breakpoint::IsInternal() const { return LLDB_BREAK_ID_IS_INTERNAL(m_bid); }
 
-void Breakpoint::SetIsHardware(bool is_hardware) {
+bool Breakpoint::SetIsHardware(bool is_hardware) {
   if (is_hardware == m_hardware)
-    return;
+    return true;
 
-  // Remember all the breakpoint locations we've disabled.
+  // Disable all non-hardware breakpoint locations.
   std::vector<BreakpointLocationSP> locations;
-  for (BreakpointLocationSP location : m_locations.BreakpointLocations()) {
-    if (location->IsEnabled()) {
-      locations.push_back(location);
-      location->SetEnabled(false);
-    }
+  for (BreakpointLocationSP location_sp : m_locations.BreakpointLocations()) {
+    if (!location_sp || !location_sp->IsEnabled())
+      continue;
+
+    lldb::BreakpointSiteSP breakpoint_site_sp =
+        location_sp->GetBreakpointSite();
+    if (!breakpoint_site_sp ||
+        breakpoint_site_sp->GetType() == BreakpointSite::eHardware)
+      continue;
+
+    locations.push_back(location_sp);
+    location_sp->SetEnabled(false);
   }
 
   // Toggle the hardware mode.
   m_hardware = is_hardware;
 
-  // Re-enable the breakpoint locations.
-  for (BreakpointLocationSP location : locations)
-    location->SetEnabled(true);
+  // Re-enable all breakpoint locations.
+  bool success = true;
+  for (BreakpointLocationSP location_sp : locations)
+    success &= location_sp->SetEnabled(true);
+  return success;
 }
 
 BreakpointLocationSP Breakpoint::AddLocation(const Address &addr,

lldb/source/Breakpoint/BreakpointLocation.cpp

@@ -74,15 +74,13 @@ bool BreakpointLocation::IsEnabled() const {
     return true;
 }
 
-void BreakpointLocation::SetEnabled(bool enabled) {
+bool BreakpointLocation::SetEnabled(bool enabled) {
   GetLocationOptions().SetEnabled(enabled);
-  if (enabled) {
-    ResolveBreakpointSite();
-  } else {
-    ClearBreakpointSite();
-  }
+  const bool success =
+      enabled ? ResolveBreakpointSite() : ClearBreakpointSite();
   SendBreakpointLocationChangedEvent(enabled ? eBreakpointEventTypeEnabled
                                              : eBreakpointEventTypeDisabled);
+  return success;
 }
 
 bool BreakpointLocation::IsAutoContinue() const {

lldb/test/API/functionalities/breakpoint/hardware_breakpoints/simple_hw_breakpoints/TestSimpleHWBreakpoints.py

@@ -22,7 +22,7 @@ def test(self):
 
         breakpoint = target.BreakpointCreateByLocation("main.c", 1)
         self.assertFalse(breakpoint.IsHardware())
-        breakpoint.SetIsHardware(True)
+        self.assertTrue(breakpoint.SetIsHardware(True))
         self.assertTrue(breakpoint.IsHardware())
 
         self.runCmd("run", RUN_SUCCEEDED)