[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

This revision was automatically updated to reflect the committed changes. Closed by commit rL330163: Make sure deleting all breakpoints clears their sites first (authored by eugene, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit:

[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. Sure. https://reviews.llvm.org/D45554 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

eugene added a comment. If nobody minds, I'd appreciate if somebody would accept this patch. https://reviews.llvm.org/D45554 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

jingham added a comment. That sounds like a good plan to me. https://reviews.llvm.org/D45554 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

eugene added a comment. Well, I agree that breakpoints, locations and sites could benefit from ownership refactoring. shared_ptr cycles are bad. Let's discuss it at lldb-dev. Meanwhile I think it's still ok to fix this bug right now, by doing what has already been done in other places.

[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

jingham added a comment. Sure, if somebody has the time fixing this to use weak pointers would be great. But that doesn't seem like the real issue to me. When a breakpoint gets removed from the Target BreakpointList(s), regardless of who else is holding onto it, it needs to get its breakpoint

[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

clayborg added a comment. In https://reviews.llvm.org/D45554#1066730, @eugene wrote: > There is an ownership cycle between BreakpointSite::m_owners and > BreakpointLocation::m_bp_site_sp. > We should probably make m_owners a collection of weak references. > But currently most of the code

[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

eugene marked 2 inline comments as done. eugene added a comment. There is an ownership cycle between BreakpointSite::m_owners and BreakpointLocation::m_bp_site_sp. We should probably make m_owners a collection of weak references. But currently most of the code just works it around by calling

[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

eugene updated this revision to Diff 142319. eugene added a comment. Got rid of the printf in the test https://reviews.llvm.org/D45554 Files: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py

[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

jingham added a comment. Calling ClearAllBreakpointSites twice does no harm, it just sees the list is empty and goes on. So even though Target::RemoveBreakpointByID clears the breakpoint sites by disabling them and then calls BreakpointList::Remove, it is fine for BreakpointList::Remove to

[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

davide added inline comments. Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c:14-15 +// This line adds a real body to the function, so we can set more than one +// breakpoint in it. +printf("Observable side effect\n");

[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

davide added inline comments. Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c:14-15 +// This line adds a real body to the function, so we can set more than one +// breakpoint in it. +printf("Observable side effect\n");

[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

eugene added inline comments. Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c:13 { +printf("Observable side effect\n"); return 0; // Set break point at this line. labath wrote: > Why did you need to add

[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

eugene updated this revision to Diff 142209. eugene marked 2 inline comments as done. eugene added a comment. add comment to the test https://reviews.llvm.org/D45554 Files: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py

[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

labath added a comment. The fix seems simple enough, but Jim needs to say whether this is the right way to fix this bug, as I am not sure about what are our assumptions about Breakpoint object lifetimes. Comment at:

[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

eugene created this revision. eugene added reviewers: labath, jingham. Fixing crash after "breakpoint delete". Bug: https://bugs.llvm.org/show_bug.cgi?id=36430 https://reviews.llvm.org/D45554 Files: