From c8ef88c446a3ff773c5be2fbf3df84b8b40c0c41 Mon Sep 17 00:00:00 2001 From: Alex Langford Date: Fri, 12 Jan 2024 13:52:59 -0800 Subject: [PATCH] [lldb][NFCI] Remove CommandReturnObject from BreakpointIDList (#77858) BreakpointIDList does not need to know about CommandReturnObject. BreakpointIDList::FindAndReplaceIDRanges is the last place that uses it in BreakpointIDList. Instead of passing in a CommandReturnObject, it now returns an llvm::Error. The callsite uses the Error to populate the CommandReturnObject as needed. --- .../lldb/Breakpoint/BreakpointIDList.h | 15 +++-- lldb/source/Breakpoint/BreakpointIDList.cpp | 57 +++++++++---------- .../Commands/CommandObjectBreakpoint.cpp | 52 +++++++++-------- 3 files changed, 61 insertions(+), 63 deletions(-) diff --git a/lldb/include/lldb/Breakpoint/BreakpointIDList.h b/lldb/include/lldb/Breakpoint/BreakpointIDList.h index 6910024695d8..3cda1860dc16 100644 --- a/lldb/include/lldb/Breakpoint/BreakpointIDList.h +++ b/lldb/include/lldb/Breakpoint/BreakpointIDList.h @@ -12,12 +12,13 @@ #include #include - -#include "lldb/lldb-enumerations.h" #include "lldb/Breakpoint/BreakpointID.h" #include "lldb/Breakpoint/BreakpointName.h" +#include "lldb/lldb-enumerations.h" #include "lldb/lldb-private.h" +#include "llvm/Support/Error.h" + namespace lldb_private { // class BreakpointIDList @@ -54,12 +55,10 @@ public: static std::pair SplitIDRangeExpression(llvm::StringRef in_string); - static void FindAndReplaceIDRanges(Args &old_args, Target *target, - bool allow_locations, - BreakpointName::Permissions - ::PermissionKinds purpose, - CommandReturnObject &result, - Args &new_args); + static llvm::Error + FindAndReplaceIDRanges(Args &old_args, Target *target, bool allow_locations, + BreakpointName::Permissions ::PermissionKinds purpose, + Args &new_args); private: BreakpointIDArray m_breakpoint_ids; diff --git a/lldb/source/Breakpoint/BreakpointIDList.cpp b/lldb/source/Breakpoint/BreakpointIDList.cpp index 05c461827cad..51185c30daba 100644 --- a/lldb/source/Breakpoint/BreakpointIDList.cpp +++ b/lldb/source/Breakpoint/BreakpointIDList.cpp @@ -11,9 +11,9 @@ #include "lldb/Breakpoint/Breakpoint.h" #include "lldb/Breakpoint/BreakpointLocation.h" -#include "lldb/Interpreter/CommandReturnObject.h" #include "lldb/Target/Target.h" #include "lldb/Utility/Args.h" +#include "lldb/Utility/StreamString.h" using namespace lldb; using namespace lldb_private; @@ -93,12 +93,9 @@ bool BreakpointIDList::FindBreakpointID(const char *bp_id_str, // NEW_ARGS should be a copy of OLD_ARGS, with and ID range specifiers replaced // by the members of the range. -void BreakpointIDList::FindAndReplaceIDRanges(Args &old_args, Target *target, - bool allow_locations, - BreakpointName::Permissions - ::PermissionKinds purpose, - CommandReturnObject &result, - Args &new_args) { +llvm::Error BreakpointIDList::FindAndReplaceIDRanges( + Args &old_args, Target *target, bool allow_locations, + BreakpointName::Permissions ::PermissionKinds purpose, Args &new_args) { llvm::StringRef range_from; llvm::StringRef range_to; llvm::StringRef current_arg; @@ -109,11 +106,11 @@ void BreakpointIDList::FindAndReplaceIDRanges(Args &old_args, Target *target, current_arg = old_args[i].ref(); if (!allow_locations && current_arg.contains('.')) { - result.AppendErrorWithFormat( + new_args.Clear(); + return llvm::createStringError( + llvm::inconvertibleErrorCode(), "Breakpoint locations not allowed, saw location: %s.", current_arg.str().c_str()); - new_args.Clear(); - return; } Status error; @@ -125,8 +122,8 @@ void BreakpointIDList::FindAndReplaceIDRanges(Args &old_args, Target *target, } else if (BreakpointID::StringIsBreakpointName(current_arg, error)) { if (!error.Success()) { new_args.Clear(); - result.AppendError(error.AsCString()); - return; + return llvm::createStringError(llvm::inconvertibleErrorCode(), + error.AsCString()); } else names_found.insert(std::string(current_arg)); } else if ((i + 2 < old_args.size()) && @@ -152,9 +149,10 @@ void BreakpointIDList::FindAndReplaceIDRanges(Args &old_args, Target *target, breakpoint_sp = target->GetBreakpointByID(bp_id->GetBreakpointID()); if (!breakpoint_sp) { new_args.Clear(); - result.AppendErrorWithFormat("'%d' is not a valid breakpoint ID.\n", - bp_id->GetBreakpointID()); - return; + return llvm::createStringError( + llvm::inconvertibleErrorCode(), + "'%d' is not a valid breakpoint ID.\n", + bp_id->GetBreakpointID()); } const size_t num_locations = breakpoint_sp->GetNumLocations(); for (size_t j = 0; j < num_locations; ++j) { @@ -180,17 +178,17 @@ void BreakpointIDList::FindAndReplaceIDRanges(Args &old_args, Target *target, if (!start_bp || !target->GetBreakpointByID(start_bp->GetBreakpointID())) { new_args.Clear(); - result.AppendErrorWithFormat("'%s' is not a valid breakpoint ID.\n", - range_from.str().c_str()); - return; + return llvm::createStringError(llvm::inconvertibleErrorCode(), + "'%s' is not a valid breakpoint ID.\n", + range_from.str().c_str()); } if (!end_bp || !target->GetBreakpointByID(end_bp->GetBreakpointID())) { new_args.Clear(); - result.AppendErrorWithFormat("'%s' is not a valid breakpoint ID.\n", - range_to.str().c_str()); - return; + return llvm::createStringError(llvm::inconvertibleErrorCode(), + "'%s' is not a valid breakpoint ID.\n", + range_to.str().c_str()); } break_id_t start_bp_id = start_bp->GetBreakpointID(); break_id_t start_loc_id = start_bp->GetLocationID(); @@ -201,11 +199,11 @@ void BreakpointIDList::FindAndReplaceIDRanges(Args &old_args, Target *target, ((start_loc_id != LLDB_INVALID_BREAK_ID) && (end_loc_id == LLDB_INVALID_BREAK_ID))) { new_args.Clear(); - result.AppendError("Invalid breakpoint id range: Either " - "both ends of range must specify" - " a breakpoint location, or neither can " - "specify a breakpoint location."); - return; + return llvm::createStringError(llvm::inconvertibleErrorCode(), + "Invalid breakpoint id range: Either " + "both ends of range must specify" + " a breakpoint location, or neither can " + "specify a breakpoint location."); } // We have valid range starting & ending breakpoint IDs. Go through all @@ -221,13 +219,13 @@ void BreakpointIDList::FindAndReplaceIDRanges(Args &old_args, Target *target, (end_loc_id != LLDB_INVALID_BREAK_ID)) { if (start_bp_id != end_bp_id) { new_args.Clear(); - result.AppendErrorWithFormat( + return llvm::createStringError( + llvm::inconvertibleErrorCode(), "Invalid range: Ranges that specify particular breakpoint " "locations" " must be within the same major breakpoint; you specified two" " different major breakpoints, %d and %d.\n", start_bp_id, end_bp_id); - return; } } @@ -302,8 +300,7 @@ void BreakpointIDList::FindAndReplaceIDRanges(Args &old_args, Target *target, } } } - - result.SetStatus(eReturnStatusSuccessFinishNoResult); + return llvm::Error::success(); } std::pair diff --git a/lldb/source/Commands/CommandObjectBreakpoint.cpp b/lldb/source/Commands/CommandObjectBreakpoint.cpp index f9ba68eda3ff..1661d5d9b743 100644 --- a/lldb/source/Commands/CommandObjectBreakpoint.cpp +++ b/lldb/source/Commands/CommandObjectBreakpoint.cpp @@ -2488,8 +2488,12 @@ void CommandObjectMultiwordBreakpoint::VerifyIDs( // breakpoint ids in the range, and shove all of those breakpoint id strings // into TEMP_ARGS. - BreakpointIDList::FindAndReplaceIDRanges(args, target, allow_locations, - purpose, result, temp_args); + if (llvm::Error err = BreakpointIDList::FindAndReplaceIDRanges( + args, target, allow_locations, purpose, temp_args)) { + result.SetError(std::move(err)); + return; + } + result.SetStatus(eReturnStatusSuccessFinishNoResult); // NOW, convert the list of breakpoint id strings in TEMP_ARGS into an actual // BreakpointIDList: @@ -2501,33 +2505,31 @@ void CommandObjectMultiwordBreakpoint::VerifyIDs( // At this point, all of the breakpoint ids that the user passed in have // been converted to breakpoint IDs and put into valid_ids. - if (result.Succeeded()) { - // Now that we've converted everything from args into a list of breakpoint - // ids, go through our tentative list of breakpoint id's and verify that - // they correspond to valid/currently set breakpoints. + // Now that we've converted everything from args into a list of breakpoint + // ids, go through our tentative list of breakpoint id's and verify that + // they correspond to valid/currently set breakpoints. - const size_t count = valid_ids->GetSize(); - for (size_t i = 0; i < count; ++i) { - BreakpointID cur_bp_id = valid_ids->GetBreakpointIDAtIndex(i); - Breakpoint *breakpoint = - target->GetBreakpointByID(cur_bp_id.GetBreakpointID()).get(); - if (breakpoint != nullptr) { - const size_t num_locations = breakpoint->GetNumLocations(); - if (static_cast(cur_bp_id.GetLocationID()) > num_locations) { - StreamString id_str; - BreakpointID::GetCanonicalReference( - &id_str, cur_bp_id.GetBreakpointID(), cur_bp_id.GetLocationID()); - i = valid_ids->GetSize() + 1; - result.AppendErrorWithFormat( - "'%s' is not a currently valid breakpoint/location id.\n", - id_str.GetData()); - } - } else { + const size_t count = valid_ids->GetSize(); + for (size_t i = 0; i < count; ++i) { + BreakpointID cur_bp_id = valid_ids->GetBreakpointIDAtIndex(i); + Breakpoint *breakpoint = + target->GetBreakpointByID(cur_bp_id.GetBreakpointID()).get(); + if (breakpoint != nullptr) { + const size_t num_locations = breakpoint->GetNumLocations(); + if (static_cast(cur_bp_id.GetLocationID()) > num_locations) { + StreamString id_str; + BreakpointID::GetCanonicalReference( + &id_str, cur_bp_id.GetBreakpointID(), cur_bp_id.GetLocationID()); i = valid_ids->GetSize() + 1; result.AppendErrorWithFormat( - "'%d' is not a currently valid breakpoint ID.\n", - cur_bp_id.GetBreakpointID()); + "'%s' is not a currently valid breakpoint/location id.\n", + id_str.GetData()); } + } else { + i = valid_ids->GetSize() + 1; + result.AppendErrorWithFormat( + "'%d' is not a currently valid breakpoint ID.\n", + cur_bp_id.GetBreakpointID()); } } }