llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Alex Langford (bulbazord) <details> <summary>Changes</summary> The lifetime of these BreakpointEventData objects is difficult to reason about. These BreakpointEventData pointers are created and passed along to `Event` which takes the raw pointer and sticks them in a shared pointer. Instead of manually managing the lifetime and memory, it would be simpler to have them be shared pointers from the start. --- Full diff: https://github.com/llvm/llvm-project/pull/78508.diff 2 Files Affected: - (modified) lldb/include/lldb/Breakpoint/Breakpoint.h (+1-1) - (modified) lldb/source/Breakpoint/Breakpoint.cpp (+23-30) ``````````diff diff --git a/lldb/include/lldb/Breakpoint/Breakpoint.h b/lldb/include/lldb/Breakpoint/Breakpoint.h index 3a8b29aee544c63..8c4308ab0bc12d5 100644 --- a/lldb/include/lldb/Breakpoint/Breakpoint.h +++ b/lldb/include/lldb/Breakpoint/Breakpoint.h @@ -672,7 +672,7 @@ class Breakpoint : public std::enable_shared_from_this<Breakpoint>, void SendBreakpointChangedEvent(lldb::BreakpointEventType eventKind); - void SendBreakpointChangedEvent(BreakpointEventData *data); + void SendBreakpointChangedEvent(const lldb::EventDataSP &breakpoint_data_sp); Breakpoint(const Breakpoint &) = delete; const Breakpoint &operator=(const Breakpoint &) = delete; diff --git a/lldb/source/Breakpoint/Breakpoint.cpp b/lldb/source/Breakpoint/Breakpoint.cpp index 6e6b51b562496c6..af5dcc9cd88d4fa 100644 --- a/lldb/source/Breakpoint/Breakpoint.cpp +++ b/lldb/source/Breakpoint/Breakpoint.cpp @@ -460,17 +460,13 @@ void Breakpoint::ResolveBreakpointInModules(ModuleList &module_list, // If this is not an internal breakpoint, set up to record the new // locations, then dispatch an event with the new locations. if (!IsInternal() && send_event) { - BreakpointEventData *new_locations_event = new BreakpointEventData( - eBreakpointEventTypeLocationsAdded, shared_from_this()); - + std::shared_ptr<BreakpointEventData> new_locations_event = + std::make_shared<BreakpointEventData>( + eBreakpointEventTypeLocationsAdded, shared_from_this()); ResolveBreakpointInModules( module_list, new_locations_event->GetBreakpointLocationCollection()); - - if (new_locations_event->GetBreakpointLocationCollection().GetSize() != - 0) { + if (new_locations_event->GetBreakpointLocationCollection().GetSize() != 0) SendBreakpointChangedEvent(new_locations_event); - } else - delete new_locations_event; } else { ElapsedTime elapsed(m_resolve_time); m_resolver_sp->ResolveBreakpointInModules(*m_filter_sp, module_list); @@ -565,12 +561,10 @@ void Breakpoint::ModulesChanged(ModuleList &module_list, bool load, // the module list, then remove their breakpoint sites, and their locations // if asked to. - BreakpointEventData *removed_locations_event; + std::shared_ptr<BreakpointEventData> removed_locations_event; if (!IsInternal()) - removed_locations_event = new BreakpointEventData( + removed_locations_event = std::make_shared<BreakpointEventData>( eBreakpointEventTypeLocationsRemoved, shared_from_this()); - else - removed_locations_event = nullptr; for (ModuleSP module_sp : module_list.Modules()) { if (m_filter_sp->ModulePasses(module_sp)) { @@ -795,31 +789,30 @@ void Breakpoint::ModuleReplaced(ModuleSP old_module_sp, // about telling the world about removing a location we didn't tell them // about adding. - BreakpointEventData *locations_event; + std::shared_ptr<BreakpointEventData> removed_locations_event; if (!IsInternal()) - locations_event = new BreakpointEventData( + removed_locations_event = std::make_shared<BreakpointEventData>( eBreakpointEventTypeLocationsRemoved, shared_from_this()); - else - locations_event = nullptr; for (BreakpointLocationSP loc_sp : locations_to_remove.BreakpointLocations()) { m_locations.RemoveLocation(loc_sp); - if (locations_event) - locations_event->GetBreakpointLocationCollection().Add(loc_sp); + if (removed_locations_event) + removed_locations_event->GetBreakpointLocationCollection().Add(loc_sp); } - SendBreakpointChangedEvent(locations_event); + SendBreakpointChangedEvent(removed_locations_event); // And announce the new ones. if (!IsInternal()) { - locations_event = new BreakpointEventData( - eBreakpointEventTypeLocationsAdded, shared_from_this()); + std::shared_ptr<BreakpointEventData> added_locations_event = + std::make_shared<BreakpointEventData>( + eBreakpointEventTypeLocationsAdded, shared_from_this()); for (BreakpointLocationSP loc_sp : locations_to_announce.BreakpointLocations()) - locations_event->GetBreakpointLocationCollection().Add(loc_sp); + added_locations_event->GetBreakpointLocationCollection().Add(loc_sp); - SendBreakpointChangedEvent(locations_event); + SendBreakpointChangedEvent(added_locations_event); } m_locations.Compact(); } @@ -989,22 +982,22 @@ void Breakpoint::SendBreakpointChangedEvent( if (!m_being_created && !IsInternal() && GetTarget().EventTypeHasListeners( Target::eBroadcastBitBreakpointChanged)) { - BreakpointEventData *data = - new Breakpoint::BreakpointEventData(eventKind, shared_from_this()); + std::shared_ptr<BreakpointEventData> data = + std::make_shared<BreakpointEventData>(eventKind, shared_from_this()); GetTarget().BroadcastEvent(Target::eBroadcastBitBreakpointChanged, data); } } -void Breakpoint::SendBreakpointChangedEvent(BreakpointEventData *data) { - if (data == nullptr) +void Breakpoint::SendBreakpointChangedEvent( + const lldb::EventDataSP &breakpoint_data_sp) { + if (!breakpoint_data_sp) return; if (!m_being_created && !IsInternal() && GetTarget().EventTypeHasListeners(Target::eBroadcastBitBreakpointChanged)) - GetTarget().BroadcastEvent(Target::eBroadcastBitBreakpointChanged, data); - else - delete data; + GetTarget().BroadcastEvent(Target::eBroadcastBitBreakpointChanged, + breakpoint_data_sp); } const char *Breakpoint::BreakpointEventTypeAsCString(BreakpointEventType type) { `````````` </details> https://github.com/llvm/llvm-project/pull/78508 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
