[lldb] Change breakpoint interfaces for error handling

[JDevlieghere]

This RP changes some Breakpoint-related interfaces to return errors. On its own these improvements are small, but they encourage better error handling going forward. There are a bunch of other candidates, but these were the functions that I touched while working on #146602.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

This RP changes some Breakpoint-related interfaces to return errors. On its own these improvements are small, but they encourage better error handling going forward. There are a bunch of other candidates, but these were the functions that I touched while working on #146602.

---

Full diff: https://github.com/llvm/llvm-project/pull/146972.diff

5 Files Affected:

• (modified) lldb/include/lldb/Breakpoint/BreakpointLocation.h (+3-11)
• (modified) lldb/source/Breakpoint/Breakpoint.cpp (+13-3)
• (modified) lldb/source/Breakpoint/BreakpointLocation.cpp (+33-31)
• (modified) lldb/source/Breakpoint/BreakpointLocationList.cpp (+16-6)
• (modified) lldb/source/Breakpoint/BreakpointSite.cpp (+2-3)

diff --git a/lldb/include/lldb/Breakpoint/BreakpointLocation.h b/lldb/include/lldb/Breakpoint/BreakpointLocation.h
index 66274e8825ee2..ce3a21f92bd46 100644
--- a/lldb/include/lldb/Breakpoint/BreakpointLocation.h
+++ b/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.
-  bool SetEnabled(bool enabled);
+  llvm::Error SetEnabled(bool enabled);
 
   /// Check the Enable/Disable state.
   ///
@@ -163,19 +163,11 @@ class BreakpointLocation
   // The next section deals with this location's breakpoint sites.
 
   /// Try to resolve the breakpoint site for this location.
-  ///
-  /// \return
-  ///     \b true if we were successful at setting a breakpoint site,
-  ///     \b false otherwise.
-  bool ResolveBreakpointSite();
+  llvm::Error ResolveBreakpointSite();
 
   /// Clear this breakpoint location's breakpoint site - for instance when
   /// disabling the breakpoint.
-  ///
-  /// \return
-  ///     \b true if there was a breakpoint site to be cleared, \b false
-  ///     otherwise.
-  bool ClearBreakpointSite();
+  llvm::Error ClearBreakpointSite();
 
   /// Return whether this breakpoint location has a breakpoint site. \return
   ///     \b true if there was a breakpoint site for this breakpoint
diff --git a/lldb/source/Breakpoint/Breakpoint.cpp b/lldb/source/Breakpoint/Breakpoint.cpp
index 8fc93cc8e0e51..b569c92ececa9 100644
--- a/lldb/source/Breakpoint/Breakpoint.cpp
+++ b/lldb/source/Breakpoint/Breakpoint.cpp
@@ -255,6 +255,8 @@ llvm::Error Breakpoint::SetIsHardware(bool is_hardware) {
   if (is_hardware == m_hardware)
     return llvm::Error::success();
 
+  Log *log = GetLog(LLDBLog::Breakpoints);
+
   // Disable all non-hardware breakpoint locations.
   std::vector<BreakpointLocationSP> locations;
   for (BreakpointLocationSP location_sp : m_locations.BreakpointLocations()) {
@@ -268,7 +270,9 @@ llvm::Error Breakpoint::SetIsHardware(bool is_hardware) {
       continue;
 
     locations.push_back(location_sp);
-    location_sp->SetEnabled(false);
+    if (llvm::Error error = location_sp->SetEnabled(false))
+      LLDB_LOG_ERROR(log, std::move(error),
+                     "Failed to disable breakpoint location: {0}");
   }
 
   // Toggle the hardware mode.
@@ -277,8 +281,11 @@ llvm::Error Breakpoint::SetIsHardware(bool is_hardware) {
   // Re-enable all breakpoint locations.
   size_t num_failures = 0;
   for (BreakpointLocationSP location_sp : locations) {
-    if (!location_sp->SetEnabled(true))
+    if (llvm::Error error = location_sp->SetEnabled(true)) {
+      LLDB_LOG_ERROR(log, std::move(error),
+                     "Failed to re-enable breakpoint location: {0}");
       num_failures++;
+    }
   }
 
   if (num_failures != 0)
@@ -613,7 +620,10 @@ void Breakpoint::ModulesChanged(ModuleList &module_list, bool load,
             // Remove this breakpoint since the shared library is unloaded, but
             // keep the breakpoint location around so we always get complete
             // hit count and breakpoint lifetime info
-            break_loc_sp->ClearBreakpointSite();
+            if (llvm::Error error = break_loc_sp->ClearBreakpointSite())
+              LLDB_LOG_ERROR(log, std::move(error),
+                             "Failed to clear breakpoint locations on library "
+                             "unload: {0}");
             if (removed_locations_event) {
               removed_locations_event->GetBreakpointLocationCollection().Add(
                   break_loc_sp);
diff --git a/lldb/source/Breakpoint/BreakpointLocation.cpp b/lldb/source/Breakpoint/BreakpointLocation.cpp
index 7553315946043..7ac9c8f5ddc4d 100644
--- a/lldb/source/Breakpoint/BreakpointLocation.cpp
+++ b/lldb/source/Breakpoint/BreakpointLocation.cpp
@@ -45,7 +45,9 @@ BreakpointLocation::BreakpointLocation(break_id_t loc_id, Breakpoint &owner,
   SetThreadIDInternal(tid);
 }
 
-BreakpointLocation::~BreakpointLocation() { ClearBreakpointSite(); }
+BreakpointLocation::~BreakpointLocation() {
+  llvm::consumeError(ClearBreakpointSite());
+}
 
 lldb::addr_t BreakpointLocation::GetLoadAddress() const {
   return m_address.GetOpcodeLoadAddress(&m_owner.GetTarget());
@@ -72,13 +74,12 @@ bool BreakpointLocation::IsEnabled() const {
   return true;
 }
 
-bool BreakpointLocation::SetEnabled(bool enabled) {
+llvm::Error BreakpointLocation::SetEnabled(bool enabled) {
   GetLocationOptions().SetEnabled(enabled);
-  const bool success =
-      enabled ? ResolveBreakpointSite() : ClearBreakpointSite();
+  llvm::Error error = enabled ? ResolveBreakpointSite() : ClearBreakpointSite();
   SendBreakpointLocationChangedEvent(enabled ? eBreakpointEventTypeEnabled
                                              : eBreakpointEventTypeDisabled);
-  return success;
+  return error;
 }
 
 bool BreakpointLocation::IsAutoContinue() const {
@@ -422,25 +423,27 @@ lldb::BreakpointSiteSP BreakpointLocation::GetBreakpointSite() const {
   return m_bp_site_sp;
 }
 
-bool BreakpointLocation::ResolveBreakpointSite() {
+llvm::Error BreakpointLocation::ResolveBreakpointSite() {
   if (m_bp_site_sp)
-    return true;
+    return llvm::Error::success();
 
   Process *process = m_owner.GetTarget().GetProcessSP().get();
   if (process == nullptr)
-    return false;
+    return llvm::createStringError("no process");
 
   lldb::break_id_t new_id =
       process->CreateBreakpointSite(shared_from_this(), m_owner.IsHardware());
 
-  if (new_id == LLDB_INVALID_BREAK_ID) {
-    LLDB_LOGF(GetLog(LLDBLog::Breakpoints),
-              "Failed to add breakpoint site at 0x%" PRIx64 "(resolved=%s)",
-              m_address.GetOpcodeLoadAddress(&m_owner.GetTarget()),
-              IsResolved() ? "yes" : "no");
-  }
+  if (new_id == LLDB_INVALID_BREAK_ID)
+    return llvm::createStringError(
+        llvm::formatv("Failed to add breakpoint site at {0:x}",
+                      m_address.GetOpcodeLoadAddress(&m_owner.GetTarget())));
+
+  if (!IsResolved())
+    return llvm::createStringError(
+        "breakpoint site created but location is still unresolved");
 
-  return IsResolved();
+  return llvm::Error::success();
 }
 
 bool BreakpointLocation::SetBreakpointSite(BreakpointSiteSP &bp_site_sp) {
@@ -449,22 +452,21 @@ bool BreakpointLocation::SetBreakpointSite(BreakpointSiteSP &bp_site_sp) {
   return true;
 }
 
-bool BreakpointLocation::ClearBreakpointSite() {
-  if (m_bp_site_sp.get()) {
-    ProcessSP process_sp(m_owner.GetTarget().GetProcessSP());
-    // If the process exists, get it to remove the owner, it will remove the
-    // physical implementation of the breakpoint as well if there are no more
-    // owners.  Otherwise just remove this owner.
-    if (process_sp)
-      process_sp->RemoveConstituentFromBreakpointSite(GetBreakpoint().GetID(),
-                                                      GetID(), m_bp_site_sp);
-    else
-      m_bp_site_sp->RemoveConstituent(GetBreakpoint().GetID(), GetID());
-
-    m_bp_site_sp.reset();
-    return true;
-  }
-  return false;
+llvm::Error BreakpointLocation::ClearBreakpointSite() {
+  if (!m_bp_site_sp)
+    return llvm::createStringError("no breakpoint site to clear");
+
+  // If the process exists, get it to remove the owner, it will remove the
+  // physical implementation of the breakpoint as well if there are no more
+  // owners.  Otherwise just remove this owner.
+  if (ProcessSP process_sp = m_owner.GetTarget().GetProcessSP())
+    process_sp->RemoveConstituentFromBreakpointSite(GetBreakpoint().GetID(),
+                                                    GetID(), m_bp_site_sp);
+  else
+    m_bp_site_sp->RemoveConstituent(GetBreakpoint().GetID(), GetID());
+
+  m_bp_site_sp.reset();
+  return llvm::Error::success();
 }
 
 void BreakpointLocation::GetDescription(Stream *s,
diff --git a/lldb/source/Breakpoint/BreakpointLocationList.cpp b/lldb/source/Breakpoint/BreakpointLocationList.cpp
index 1d8b4c1ccfaeb..44d1eb5bf7140 100644
--- a/lldb/source/Breakpoint/BreakpointLocationList.cpp
+++ b/lldb/source/Breakpoint/BreakpointLocationList.cpp
@@ -15,6 +15,8 @@
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/ArchSpec.h"
+#include "lldb/Utility/LLDBLog.h"
+#include "lldb/Utility/Log.h"
 
 using namespace lldb;
 using namespace lldb_private;
@@ -151,17 +153,24 @@ const BreakpointLocationSP BreakpointLocationList::GetByIndex(size_t i) const {
 void BreakpointLocationList::ClearAllBreakpointSites() {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
   collection::iterator pos, end = m_locations.end();
-  for (pos = m_locations.begin(); pos != end; ++pos)
-    (*pos)->ClearBreakpointSite();
+  Log *log = GetLog(LLDBLog::Breakpoints);
+
+  for (pos = m_locations.begin(); pos != end; ++pos) {
+    if (llvm::Error error = (*pos)->ClearBreakpointSite())
+      LLDB_LOG_ERROR(log, std::move(error), "{0}");
+  }
 }
 
 void BreakpointLocationList::ResolveAllBreakpointSites() {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
   collection::iterator pos, end = m_locations.end();
+  Log *log = GetLog(LLDBLog::Breakpoints);
 
   for (pos = m_locations.begin(); pos != end; ++pos) {
-    if ((*pos)->IsEnabled())
-      (*pos)->ResolveBreakpointSite();
+    if ((*pos)->IsEnabled()) {
+      if (llvm::Error error = (*pos)->ResolveBreakpointSite())
+        LLDB_LOG_ERROR(log, std::move(error), "{0}");
+    }
   }
 }
 
@@ -212,7 +221,8 @@ BreakpointLocationSP BreakpointLocationList::AddLocation(
   if (!bp_loc_sp) {
     bp_loc_sp = Create(addr, resolve_indirect_symbols);
     if (bp_loc_sp) {
-      bp_loc_sp->ResolveBreakpointSite();
+      if (llvm::Error error = bp_loc_sp->ResolveBreakpointSite())
+        LLDB_LOG_ERROR(GetLog(LLDBLog::Breakpoints), std::move(error), "{0}");
 
       if (new_location)
         *new_location = true;
@@ -234,7 +244,7 @@ void BreakpointLocationList::SwapLocation(
   to_location_sp->SwapLocation(from_location_sp);
   RemoveLocation(from_location_sp);
   m_address_to_location[to_location_sp->GetAddress()] = to_location_sp;
-  to_location_sp->ResolveBreakpointSite();
+  llvm::consumeError(to_location_sp->ResolveBreakpointSite());
 }
 
 bool BreakpointLocationList::RemoveLocation(
diff --git a/lldb/source/Breakpoint/BreakpointSite.cpp b/lldb/source/Breakpoint/BreakpointSite.cpp
index 8b964c5711468..d430e3de788f0 100644
--- a/lldb/source/Breakpoint/BreakpointSite.cpp
+++ b/lldb/source/Breakpoint/BreakpointSite.cpp
@@ -33,9 +33,8 @@ BreakpointSite::BreakpointSite(const BreakpointLocationSP &constituent,
 BreakpointSite::~BreakpointSite() {
   BreakpointLocationSP bp_loc_sp;
   const size_t constituent_count = m_constituents.GetSize();
-  for (size_t i = 0; i < constituent_count; i++) {
-    m_constituents.GetByIndex(i)->ClearBreakpointSite();
-  }
+  for (size_t i = 0; i < constituent_count; i++)
+    llvm::consumeError(m_constituents.GetByIndex(i)->ClearBreakpointSite());
 }
 
 break_id_t BreakpointSite::GetNextID() {

[jasonmolenda]

Seems like a reasonable RP to me.