[Lldb-commits] [lldb] [lldb] Change breakpoint interfaces for error handling (PR #146972)
Jonas Devlieghere via lldb-commits
lldb-commits at lists.llvm.org
Thu Jul 3 16:24:26 PDT 2025
https://github.com/JDevlieghere created https://github.com/llvm/llvm-project/pull/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.
>From 8b15026e70395721deffc114a2780fab1a041b93 Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jonas at devlieghere.com>
Date: Thu, 3 Jul 2025 16:20:27 -0700
Subject: [PATCH] [lldb] Change breakpoint interfaces for error handling
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.
---
.../lldb/Breakpoint/BreakpointLocation.h | 14 +---
lldb/source/Breakpoint/Breakpoint.cpp | 16 ++++-
lldb/source/Breakpoint/BreakpointLocation.cpp | 64 ++++++++++---------
.../Breakpoint/BreakpointLocationList.cpp | 22 +++++--
lldb/source/Breakpoint/BreakpointSite.cpp | 5 +-
5 files changed, 67 insertions(+), 54 deletions(-)
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() {
More information about the lldb-commits
mailing list