[lldb] Add SB API to make a breakpoint a hardware breakpoint

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