[lldb] Change breakpoint interfaces for error handling (#146972)

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.

Author: JDevlieghere

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

lldb/source/API/SBBreakpointLocation.cpp

@@ -102,7 +102,7 @@ void SBBreakpointLocation::SetEnabled(bool enabled) {
   if (loc_sp) {
     std::lock_guard<std::recursive_mutex> guard(
         loc_sp->GetTarget().GetAPIMutex());
-    loc_sp->SetEnabled(enabled);
+    llvm::consumeError(loc_sp->SetEnabled(enabled));
   }
 }
 

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)
@@ -570,11 +577,11 @@ void Breakpoint::ModulesChanged(ModuleList &module_list, bool load,
           if (!seen)
             seen = true;
 
-          if (!break_loc_sp->ResolveBreakpointSite()) {
-            LLDB_LOGF(log,
-                      "Warning: could not set breakpoint site for "
-                      "breakpoint location %d of breakpoint %d.\n",
-                      break_loc_sp->GetID(), GetID());
+          if (llvm::Error error = break_loc_sp->ResolveBreakpointSite()) {
+            LLDB_LOG_ERROR(log, std::move(error),
+                           "could not set breakpoint site for "
+                           "breakpoint location {1} of breakpoint {2}: {0}",
+                           break_loc_sp->GetID(), GetID());
           }
         }
       }
@@ -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);

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,

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(

lldb/source/Breakpoint/BreakpointResolverAddress.cpp

@@ -116,6 +116,7 @@ void BreakpointResolverAddress::ResolveBreakpointInModules(
 
 Searcher::CallbackReturn BreakpointResolverAddress::SearchCallback(
     SearchFilter &filter, SymbolContext &context, Address *addr) {
+  Log *log = GetLog(LLDBLog::Breakpoints);
   BreakpointSP breakpoint_sp = GetBreakpoint();
   Breakpoint &breakpoint = *breakpoint_sp;
 
@@ -140,7 +141,6 @@ Searcher::CallbackReturn BreakpointResolverAddress::SearchCallback(
       if (bp_loc_sp && !breakpoint.IsInternal()) {
         StreamString s;
         bp_loc_sp->GetDescription(&s, lldb::eDescriptionLevelVerbose);
-        Log *log = GetLog(LLDBLog::Breakpoints);
         LLDB_LOGF(log, "Added location: %s\n", s.GetData());
       }
     } else {
@@ -149,8 +149,10 @@ Searcher::CallbackReturn BreakpointResolverAddress::SearchCallback(
           m_addr.GetLoadAddress(&breakpoint.GetTarget());
       if (cur_load_location != m_resolved_addr) {
         m_resolved_addr = cur_load_location;
-        loc_sp->ClearBreakpointSite();
-        loc_sp->ResolveBreakpointSite();
+        if (llvm::Error error = loc_sp->ClearBreakpointSite())
+          LLDB_LOG_ERROR(log, std::move(error), "{0}");
+        if (llvm::Error error = loc_sp->ResolveBreakpointSite())
+          LLDB_LOG_ERROR(log, std::move(error), "{0}");
       }
     }
   }

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() {

lldb/source/Commands/CommandObjectBreakpoint.cpp

@@ -28,6 +28,7 @@
 #include "lldb/Target/ThreadSpec.h"
 #include "lldb/Utility/RegularExpression.h"
 #include "lldb/Utility/StreamString.h"
+#include "llvm/Support/FormatAdapters.h"
 
 #include <memory>
 #include <optional>
@@ -956,7 +957,10 @@ class CommandObjectBreakpointEnable : public CommandObjectParsed {
               BreakpointLocation *location =
                   breakpoint->FindLocationByID(cur_bp_id.GetLocationID()).get();
               if (location) {
-                location->SetEnabled(true);
+                if (llvm::Error error = location->SetEnabled(true))
+                  result.AppendErrorWithFormatv(
+                      "failed to enable breakpoint location: {0}",
+                      llvm::fmt_consume(std::move(error)));
                 ++loc_count;
               }
             } else {
@@ -1062,7 +1066,10 @@ the second re-enables the first location.");
               BreakpointLocation *location =
                   breakpoint->FindLocationByID(cur_bp_id.GetLocationID()).get();
               if (location) {
-                location->SetEnabled(false);
+                if (llvm::Error error = location->SetEnabled(false))
+                  result.AppendErrorWithFormatv(
+                      "failed to disable breakpoint location: {0}",
+                      llvm::fmt_consume(std::move(error)));
                 ++loc_count;
               }
             } else {
@@ -1508,7 +1515,10 @@ class CommandObjectBreakpointDelete : public CommandObjectParsed {
           // It makes no sense to try to delete individual locations, so we
           // disable them instead.
           if (location) {
-            location->SetEnabled(false);
+            if (llvm::Error error = location->SetEnabled(false))
+              result.AppendErrorWithFormatv(
+                  "failed to disable breakpoint location: {0}",
+                  llvm::fmt_consume(std::move(error)));
             ++disable_count;
           }
         } else {

lldb/source/Commands/CommandObjectProcess.cpp

@@ -35,6 +35,7 @@
 #include "lldb/Utility/Args.h"
 #include "lldb/Utility/ScriptedMetadata.h"
 #include "lldb/Utility/State.h"
+#include "llvm/Support/FormatAdapters.h"
 
 #include "llvm/ADT/ScopeExit.h"
 
@@ -642,8 +643,12 @@ class CommandObjectProcessContinue : public CommandObjectParsed {
             BreakpointLocationSP loc_sp = bp_sp->GetLocationAtIndex(loc_idx);
             tmp_id.SetBreakpointLocationID(loc_idx);
             if (!with_locs.Contains(tmp_id) && loc_sp->IsEnabled()) {
-              locs_disabled.push_back(tmp_id);
-              loc_sp->SetEnabled(false);
+              if (llvm::Error error = loc_sp->SetEnabled(false))
+                result.AppendErrorWithFormatv(
+                    "failed to disable breakpoint location: {0}",
+                    llvm::fmt_consume(std::move(error)));
+              else
+                locs_disabled.push_back(tmp_id);
             }
           }
         }
@@ -698,8 +703,12 @@ class CommandObjectProcessContinue : public CommandObjectParsed {
         if (bp_sp) {
           BreakpointLocationSP loc_sp
               = bp_sp->FindLocationByID(bkpt_id.GetLocationID());
-          if (loc_sp)
-            loc_sp->SetEnabled(true);
+          if (loc_sp) {
+            if (llvm::Error error = loc_sp->SetEnabled(true))
+              result.AppendErrorWithFormatv(
+                  "failed to enable breakpoint location: {0}",
+                  llvm::fmt_consume(std::move(error)));
+          }
         }
       }