https://github.com/JDevlieghere updated
https://github.com/llvm/llvm-project/pull/146972
>From b593c9b4f8791f2b5562ca301f77af72619b9c30 Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jo...@devlieghere.com>
Date: Tue, 8 Jul 2025 15:32:13 -0700
Subject: [PATCH 1/2] [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/API/SBBreakpointLocation.cpp | 2 +-
lldb/source/Breakpoint/Breakpoint.cpp | 16 ++++-
lldb/source/Breakpoint/BreakpointLocation.cpp | 64 ++++++++++---------
.../Breakpoint/BreakpointLocationList.cpp | 22 +++++--
.../Breakpoint/BreakpointResolverAddress.cpp | 8 ++-
lldb/source/Breakpoint/BreakpointSite.cpp | 5 +-
.../Commands/CommandObjectBreakpoint.cpp | 17 +++--
lldb/source/Commands/CommandObjectProcess.cpp | 17 +++--
9 files changed, 99 insertions(+), 66 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/API/SBBreakpointLocation.cpp
b/lldb/source/API/SBBreakpointLocation.cpp
index b2d1da3927c6e..479354a62627d 100644
--- a/lldb/source/API/SBBreakpointLocation.cpp
+++ b/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));
}
}
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/BreakpointResolverAddress.cpp
b/lldb/source/Breakpoint/BreakpointResolverAddress.cpp
index 367e172b5e9c1..828647ceef637 100644
--- a/lldb/source/Breakpoint/BreakpointResolverAddress.cpp
+++ b/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}");
}
}
}
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() {
diff --git a/lldb/source/Commands/CommandObjectBreakpoint.cpp
b/lldb/source/Commands/CommandObjectBreakpoint.cpp
index 4631c97bf50ae..f703466209969 100644
--- a/lldb/source/Commands/CommandObjectBreakpoint.cpp
+++ b/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>
@@ -1062,8 +1063,12 @@ the second re-enables the first location.");
BreakpointLocation *location =
breakpoint->FindLocationByID(cur_bp_id.GetLocationID()).get();
if (location) {
- location->SetEnabled(false);
- ++loc_count;
+ if (llvm::Error error = location->SetEnabled(false))
+ result.AppendErrorWithFormatv(
+ "failed to disable breakpoint location: {0}",
+ llvm::fmt_consume(std::move(error)));
+ else
+ ++loc_count;
}
} else {
breakpoint->SetEnabled(false);
@@ -1508,8 +1513,12 @@ class CommandObjectBreakpointDelete : public
CommandObjectParsed {
// It makes no sense to try to delete individual locations, so we
// disable them instead.
if (location) {
- location->SetEnabled(false);
- ++disable_count;
+ if (llvm::Error error = location->SetEnabled(false))
+ result.AppendErrorWithFormatv(
+ "failed to disable breakpoint location: {0}",
+ llvm::fmt_consume(std::move(error)));
+ else
+ ++disable_count;
}
} else {
target.RemoveBreakpointByID(cur_bp_id.GetBreakpointID());
diff --git a/lldb/source/Commands/CommandObjectProcess.cpp
b/lldb/source/Commands/CommandObjectProcess.cpp
index 0a1744277d7dc..1181b2d95c8b4 100644
--- a/lldb/source/Commands/CommandObjectProcess.cpp
+++ b/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)));
+ }
}
}
>From 4c8c902826654b50efbf0eec49cd7287b251a666 Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jo...@devlieghere.com>
Date: Tue, 8 Jul 2025 23:52:01 +0000
Subject: [PATCH 2/2] Handle error
---
lldb/source/Breakpoint/Breakpoint.cpp | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/lldb/source/Breakpoint/Breakpoint.cpp
b/lldb/source/Breakpoint/Breakpoint.cpp
index b569c92ececa9..2e9ae0d67d28f 100644
--- a/lldb/source/Breakpoint/Breakpoint.cpp
+++ b/lldb/source/Breakpoint/Breakpoint.cpp
@@ -577,10 +577,10 @@ 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",
+ 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());
}
}
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
