[lldb] Add SB API to make a breakpoint a hardware breakpoint (#146602)

This adds SBBreakpoint::SetIsHardware, allowing clients to mark an
existing breakpoint as a hardware breakpoint purely through the API.
This is safe to do after creation, as the hardware/software distinction
doesn't affect how breakpoint locations are selected.

In some cases (e.g. when writing a trap instruction would alter program
behavior), it's important to use hardware breakpoints. Ideally, we’d
extend the various `Create` methods to support this, but given their
number, this patch limits the scope to the post-creation API. As a
workaround, users can also rely on target.require-hardware-breakpoint or
use the `breakpoint set` command.

rdar://153528045

Author: JDevlieghere

lldb/include/lldb/API/SBBreakpoint.h

@@ -148,6 +148,11 @@ class LLDB_API SBBreakpoint {
 
   bool IsHardware() const;
 
+  /// Make this breakpoint a hardware breakpoint. This will replace all existing
+  /// breakpoint locations with hardware breakpoints. Returns an error if this
+  /// fails, e.g. when there aren't enough hardware resources available.
+  lldb::SBError SetIsHardware(bool is_hardware);
+
   // Can only be called from a ScriptedBreakpointResolver...
   SBError
   AddLocation(SBAddress &address);

lldb/include/lldb/Breakpoint/Breakpoint.h

@@ -519,6 +519,8 @@ class Breakpoint : public std::enable_shared_from_this<Breakpoint>,
 
   bool IsHardware() const { return m_hardware; }
 
+  llvm::Error SetIsHardware(bool is_hardware);
+
   lldb::BreakpointResolverSP GetResolver() { return m_resolver_sp; }
 
   lldb::SearchFilterSP GetSearchFilter() { return m_filter_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,6 +781,18 @@ bool SBBreakpoint::IsHardware() const {
   return false;
 }
 
+lldb::SBError 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());
+    return SBError(Status::FromError(bkpt_sp->SetIsHardware(is_hardware)));
+  }
+  return SBError();
+}
+
 BreakpointSP SBBreakpoint::GetSP() const { return m_opaque_wp.lock(); }
 
 // This is simple collection of breakpoint id's and their target.

lldb/source/Breakpoint/Breakpoint.cpp

@@ -61,7 +61,7 @@ Breakpoint::Breakpoint(Target &new_target, const Breakpoint &source_bp)
 Breakpoint::~Breakpoint() = default;
 
 BreakpointSP Breakpoint::CopyFromBreakpoint(TargetSP new_target,
-    const Breakpoint& bp_to_copy_from) {
+                                            const Breakpoint &bp_to_copy_from) {
   if (!new_target)
     return BreakpointSP();
 
@@ -163,7 +163,7 @@ lldb::BreakpointSP Breakpoint::CreateFromStructuredData(
         std::make_shared<SearchFilterForUnconstrainedSearches>(target_sp);
   else {
     filter_sp = SearchFilter::CreateFromStructuredData(target_sp, *filter_dict,
-        create_error);
+                                                       create_error);
     if (create_error.Fail()) {
       error = Status::FromErrorStringWithFormat(
           "Error creating breakpoint filter from data: %s.",
@@ -174,7 +174,7 @@ lldb::BreakpointSP Breakpoint::CreateFromStructuredData(
 
   std::unique_ptr<BreakpointOptions> options_up;
   StructuredData::Dictionary *options_dict;
-  Target& target = *target_sp;
+  Target &target = *target_sp;
   success = breakpoint_dict->GetValueForKeyAsDictionary(
       BreakpointOptions::GetSerializationKey(), options_dict);
   if (success) {
@@ -192,8 +192,8 @@ lldb::BreakpointSP Breakpoint::CreateFromStructuredData(
   success = breakpoint_dict->GetValueForKeyAsBoolean(
       Breakpoint::GetKey(OptionNames::Hardware), hardware);
 
-  result_sp = target.CreateBreakpoint(filter_sp, resolver_sp, false,
-                                      hardware, true);
+  result_sp =
+      target.CreateBreakpoint(filter_sp, resolver_sp, false, hardware, true);
 
   if (result_sp && options_up) {
     result_sp->m_options = *options_up;
@@ -251,6 +251,45 @@ const lldb::TargetSP Breakpoint::GetTargetSP() {
 
 bool Breakpoint::IsInternal() const { return LLDB_BREAK_ID_IS_INTERNAL(m_bid); }
 
+llvm::Error Breakpoint::SetIsHardware(bool is_hardware) {
+  if (is_hardware == m_hardware)
+    return llvm::Error::success();
+
+  // Disable all non-hardware breakpoint locations.
+  std::vector<BreakpointLocationSP> locations;
+  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 all breakpoint locations.
+  size_t num_failures = 0;
+  for (BreakpointLocationSP location_sp : locations) {
+    if (!location_sp->SetEnabled(true))
+      num_failures++;
+  }
+
+  if (num_failures != 0)
+    return llvm::createStringError(
+        "%ull out of %ull breakpoint locations left disabled because they "
+        "couldn't be converted to hardware",
+        num_failures, locations.size());
+
+  return llvm::Error::success();
+}
+
 BreakpointLocationSP Breakpoint::AddLocation(const Address &addr,
                                              bool *new_location) {
   return m_locations.AddLocation(addr, m_resolve_indirect_symbols,
@@ -952,8 +991,7 @@ void Breakpoint::GetResolverDescription(Stream *s) {
     m_resolver_sp->GetDescription(s);
 }
 
-bool Breakpoint::GetMatchingFileLine(ConstString filename,
-                                     uint32_t line_number,
+bool Breakpoint::GetMatchingFileLine(ConstString filename, uint32_t line_number,
                                      BreakpointLocationCollection &loc_coll) {
   // TODO: To be correct, this method needs to fill the breakpoint location
   // collection
@@ -1010,19 +1048,32 @@ void Breakpoint::SendBreakpointChangedEvent(
 
 const char *Breakpoint::BreakpointEventTypeAsCString(BreakpointEventType type) {
   switch (type) {
-    case eBreakpointEventTypeInvalidType: return "invalid";
-    case eBreakpointEventTypeAdded: return "breakpoint added";
-    case eBreakpointEventTypeRemoved: return "breakpoint removed";
-    case eBreakpointEventTypeLocationsAdded: return "locations added";
-    case eBreakpointEventTypeLocationsRemoved: return "locations removed";
-    case eBreakpointEventTypeLocationsResolved: return "locations resolved";
-    case eBreakpointEventTypeEnabled: return "breakpoint enabled";
-    case eBreakpointEventTypeDisabled: return "breakpoint disabled";
-    case eBreakpointEventTypeCommandChanged: return "command changed";
-    case eBreakpointEventTypeConditionChanged: return "condition changed";
-    case eBreakpointEventTypeIgnoreChanged: return "ignore count changed";
-    case eBreakpointEventTypeThreadChanged: return "thread changed";
-    case eBreakpointEventTypeAutoContinueChanged: return "autocontinue changed";
+  case eBreakpointEventTypeInvalidType:
+    return "invalid";
+  case eBreakpointEventTypeAdded:
+    return "breakpoint added";
+  case eBreakpointEventTypeRemoved:
+    return "breakpoint removed";
+  case eBreakpointEventTypeLocationsAdded:
+    return "locations added";
+  case eBreakpointEventTypeLocationsRemoved:
+    return "locations removed";
+  case eBreakpointEventTypeLocationsResolved:
+    return "locations resolved";
+  case eBreakpointEventTypeEnabled:
+    return "breakpoint enabled";
+  case eBreakpointEventTypeDisabled:
+    return "breakpoint disabled";
+  case eBreakpointEventTypeCommandChanged:
+    return "command changed";
+  case eBreakpointEventTypeConditionChanged:
+    return "condition changed";
+  case eBreakpointEventTypeIgnoreChanged:
+    return "ignore count changed";
+  case eBreakpointEventTypeThreadChanged:
+    return "thread changed";
+  case eBreakpointEventTypeAutoContinueChanged:
+    return "autocontinue changed";
   };
   llvm_unreachable("Fully covered switch above!");
 }
@@ -1060,7 +1111,7 @@ void Breakpoint::BreakpointEventData::Dump(Stream *s) const {
   BreakpointEventType event_type = GetBreakpointEventType();
   break_id_t bkpt_id = GetBreakpoint()->GetID();
   s->Format("bkpt: {0} type: {1}", bkpt_id,
-      BreakpointEventTypeAsCString(event_type));
+            BreakpointEventTypeAsCString(event_type));
 }
 
 const Breakpoint::BreakpointEventData *

lldb/source/Breakpoint/BreakpointLocation.cpp

@@ -72,15 +72,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 {
@@ -436,10 +434,10 @@ bool BreakpointLocation::ResolveBreakpointSite() {
       process->CreateBreakpointSite(shared_from_this(), m_owner.IsHardware());
 
   if (new_id == LLDB_INVALID_BREAK_ID) {
-    Log *log = GetLog(LLDBLog::Breakpoints);
-    if (log)
-      log->Warning("Failed to add breakpoint site at 0x%" PRIx64,
-                   m_address.GetOpcodeLoadAddress(&m_owner.GetTarget()));
+    LLDB_LOGF(GetLog(LLDBLog::Breakpoints),
+              "Failed to add breakpoint site at 0x%" PRIx64 "(resolved=%s)",
+              m_address.GetOpcodeLoadAddress(&m_owner.GetTarget()),
+              IsResolved() ? "yes" : "no");
   }
 
   return IsResolved();

lldb/test/API/functionalities/breakpoint/hardware_breakpoints/simple_hw_breakpoints/Makefile

@@ -0,0 +1,7 @@
+C_SOURCES := main.c
+
+ifeq ($(CC_TYPE), icc)
+    CFLAGS_EXTRAS := -debug inline-debug-info
+endif
+
+include Makefile.rules

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

@@ -0,0 +1,50 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+from functionalities.breakpoint.hardware_breakpoints.base import *
+
+
+class SimpleHWBreakpointTest(HardwareBreakpointTestBase):
+    def does_not_support_hw_breakpoints(self):
+        # FIXME: Use HardwareBreakpointTestBase.supports_hw_breakpoints
+        if super().supports_hw_breakpoints() is None:
+            return "Hardware breakpoints are unsupported"
+        return None
+
+    @skipTestIfFn(does_not_support_hw_breakpoints)
+    def test(self):
+        """Test SBBreakpoint::SetIsHardware"""
+        self.build()
+
+        # Set a breakpoint on main.
+        target, process, _, main_bp = lldbutil.run_to_source_breakpoint(
+            self, "main", lldb.SBFileSpec("main.c")
+        )
+
+        break_on_me_bp = target.BreakpointCreateByLocation("main.c", 1)
+
+        self.assertFalse(main_bp.IsHardware())
+        self.assertFalse(break_on_me_bp.IsHardware())
+        self.assertGreater(break_on_me_bp.GetNumResolvedLocations(), 0)
+
+        error = break_on_me_bp.SetIsHardware(True)
+
+        # Regardless of whether we succeeded in updating all the locations, the
+        # breakpoint will be marked as a hardware breakpoint.
+        self.assertTrue(break_on_me_bp.IsHardware())
+
+        if super().supports_hw_breakpoints():
+            self.assertSuccess(error)
+
+            # Continue to our Hardware breakpoint and verify that's the reason
+            # we're stopped.
+            process.Continue()
+            self.expect(
+                "thread list",
+                STOPPED_DUE_TO_BREAKPOINT,
+                substrs=["stopped", "stop reason = breakpoint"],
+            )
+        else:
+            self.assertFailure(error)

lldb/test/API/functionalities/breakpoint/hardware_breakpoints/simple_hw_breakpoints/main.c

@@ -0,0 +1,7 @@
+int break_on_me() {
+  int i = 10;
+  i++;
+  return i;
+}
+
+int main() { return break_on_me(); }