llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) <details> <summary>Changes</summary> 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 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() { `````````` </details> https://github.com/llvm/llvm-project/pull/146972 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits