<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Feb 21, 2017, at 9:10 AM, Zachary Turner via lldb-dev <<a href="mailto:lldb-dev@lists.llvm.org" class="">lldb-dev@lists.llvm.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class="">No comments on this specifically, but +1 to reducing shared_ptr usage in general. We use way too many, and often it feels like shared_ptr is used as a way to avoid having to think about ownership, which leads to more problems than it solves imo<br class=""></div></blockquote><div><br class=""></div>We do a pretty good job of using shared pointers where they are needed, so I don't agree with the above statement. We have strong memory models in LLDB and certain things need to keep certain things alive. LLVM is quite the wild west with regard to memory management, so I don't want to base any changes on it. We just need to use std::weak_ptr when needed. There should be no two items that contain strong references to each other. </div><div><br class=""></div><div>In general think about what should keep things alive. If I have a SBModule variable, I should expect that it won't let my module go away. If I have a SBTarget, I would expect it to keep the target around as long as I have a reference. Things that belong to any of these items, like breakpoints, watchpoints, compile units, functions, shouldn't keep the module or target around.</div><div><br class=""></div><div><blockquote type="cite" class=""><div class=""><div class="gmail_quote"><div dir="ltr" class="">On Tue, Feb 21, 2017 at 9:03 AM Pavel Labath via lldb-dev <<a href="mailto:lldb-dev@lists.llvm.org" class="">lldb-dev@lists.llvm.org</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello all,<br class="gmail_msg">
<br class="gmail_msg">
I've been debugging the newly added TestStepOverBreakpoint.py, which<br class="gmail_msg">
has been failing on windows, for a very prosaic reason: after the test<br class="gmail_msg">
completes, we are unable to run "make clean" because lldb still holds<br class="gmail_msg">
the file open.<br class="gmail_msg">
<br class="gmail_msg">
After some debugging, I've found that this happens because the test<br class="gmail_msg">
case stores the SBBreakpoint object in a member variable of the python<br class="gmail_msg">
test case class. The breakpoint class ends up transitively holding a<br class="gmail_msg">
reference to the main executable module, which prevents the module<br class="gmail_msg">
from being garbage-collected when the target is destroyed.<br class="gmail_msg">
<br class="gmail_msg">
For reference, the full ownership chain is something like:<br class="gmail_msg">
StepOverBreakpointTestCase(python) => SBBreakpoint => Breakpoint =><br class="gmail_msg">
BreakpointLocation => LLVMUserExpression => IRExecutionUnit =><br class="gmail_msg">
SymbolContext => Module.<br class="gmail_msg">
<br class="gmail_msg">
To get the test working, we need to break this chain somewhere. A<br class="gmail_msg">
couple of places I see are:<br class="gmail_msg">
- BreakpointLocation: Remove the compiled expression reference when<br class="gmail_msg">
the target is destroyed (AFAICS, it is used as a cache to avoid<br class="gmail_msg">
recomputing the expression every time. It can be theoretically<br class="gmail_msg">
recomputed if needed, but that shouldn't be necessary as the target is<br class="gmail_msg">
destroyed anyway)<br class="gmail_msg"></blockquote></div></div></blockquote><div><br class=""></div>We should absolutely be clearing these useless expressions when the process is killed. Compiling expressions take 300-500 ms at the very least and we do cache the breakpoint expressions in each location, which is good for the lifetime of the process, but these locations should go away when the process goes away or execs.</div><div><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
- SBBreakpoint: make SBBreakpoint hold a weak_ptr to the Breakpoint<br class="gmail_msg">
object. When the target is destroyed, the SBBreakpoint object becomes<br class="gmail_msg">
invalid (One doesn't cannot do anything useful with the breakpoint<br class="gmail_msg">
once the target has been deleted anyway).<br class="gmail_msg"></blockquote></div></div></blockquote><div><br class=""></div>This is also valid. If the breakpoint is still around, you will be able to use it as the weak_ptr will make a shared pointer with a valid value, and if it isn't it won't be able to do anything anyway.</div><div><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
- StepOverBreakpointTestCase: Have the test not store the breakpoints<br class="gmail_msg">
in the test case object. Basically, declare that this is not a bug,<br class="gmail_msg">
and it's the users responsibility to clean up necessary objects.<br class="gmail_msg"></blockquote></div></div></blockquote><div><br class=""></div><div>It would be nice to avoid this.</div><div><br class=""></div><blockquote type="cite" class=""><div class=""><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Any thoughts on what is the appropriate solution here?</blockquote></div></div></blockquote></div><br class=""><div class=""><br class=""></div><div class="">So I vote for:</div><div class="">1 - remove all breakpoint expressions when the target stops and the locations becomes unresolved.</div><div class="">2 - SBBreakpoint, SBBreakpointLocation, SBWatchpoint and can switch over to using std::weak_ptr</div><div class=""><br class=""></div><div class="">The other fix is that might be worth it is to make a SymbolContextRef, kind of like we have the ExecutionContext (with shared pointers) and ExecutionContextRef (with weak pointers). We would then have things like IRExecutionUnit switch over to use a SymbolContextRef. We should also look for other places where people are storing SymbolContext as member variables and possibly switch them over to use SymbolContextRef.</div><div class=""><br class=""></div><div class="">class SymbolContextRef</div><div class="">{</div><div style="margin: 0px; font-size: 11px; line-height: normal; color: rgb(0, 132, 0);" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> </span><span style="font-variant-ligatures: no-common-ligatures; color: #4f8187" class="">lldb</span><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class="">::</span><span style="font-variant-ligatures: no-common-ligatures; color: #4f8187" class="">TargetWP</span><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> target_wp; </span><span style="font-variant-ligatures: no-common-ligatures" class="">///< The Target for a given query</span></div><div style="margin: 0px; font-size: 11px; line-height: normal; color: rgb(0, 132, 0);" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> </span><span style="font-variant-ligatures: no-common-ligatures; color: #4f8187" class="">lldb</span><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class="">::</span><span style="font-variant-ligatures: no-common-ligatures; color: #4f8187" class="">ModuleWP</span><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> module_wp; </span><span style="font-variant-ligatures: no-common-ligatures" class="">///< The Module for a given query</span></div><div style="margin: 0px; font-size: 11px; line-height: normal; color: rgb(0, 132, 0);" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> </span><span style="font-variant-ligatures: no-common-ligatures; color: #703daa" class="">CompileUnit</span><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> *comp_unit; </span><span style="font-variant-ligatures: no-common-ligatures" class="">///< The CompileUnit for a given query</span></div><div style="margin: 0px; font-size: 11px; line-height: normal; color: rgb(0, 132, 0);" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> </span><span style="font-variant-ligatures: no-common-ligatures; color: #4f8187" class="">Function</span><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> *function; </span><span style="font-variant-ligatures: no-common-ligatures" class="">///< The Function for a given query</span></div><div style="margin: 0px; font-size: 11px; line-height: normal; color: rgb(0, 132, 0);" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> </span><span style="font-variant-ligatures: no-common-ligatures; color: #4f8187" class="">Block</span><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> *block; </span><span style="font-variant-ligatures: no-common-ligatures" class="">///< The Block for a given query</span></div><div style="margin: 0px; font-size: 11px; line-height: normal; color: rgb(0, 132, 0);" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> </span><span style="font-variant-ligatures: no-common-ligatures; color: #4f8187" class="">LineEntry</span><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> line_entry; </span><span style="font-variant-ligatures: no-common-ligatures" class="">///< The LineEntry for a given query</span></div><div style="margin: 0px; font-size: 11px; line-height: normal; color: rgb(0, 132, 0);" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> </span><span style="font-variant-ligatures: no-common-ligatures; color: #4f8187" class="">Symbol</span><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> *symbol; </span><span style="font-variant-ligatures: no-common-ligatures" class="">///< The Symbol for a given query</span></div><div style="margin: 0px; font-size: 11px; line-height: normal; color: rgb(0, 132, 0);" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> </span><span style="font-variant-ligatures: no-common-ligatures; color: #4f8187" class="">Variable</span><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> *variable; </span><span style="font-variant-ligatures: no-common-ligatures" class="">///< The global variable matching the given query</span></div><div style="margin: 0px; font-size: 11px; line-height: normal;" class=""><span style="font-variant-ligatures: no-common-ligatures" class="">};</span></div><div class=""> </div><div class="">Then a SymbolContext would add a constructor that takes a SymbolContextRef:</div><div class=""><br class=""></div><div class="">SymbolContextRef sym_ctx_ref = ...;</div><div class=""><br class=""></div><div class="">Anytime you want to use a SymbolContextRef, you first must turn it into a SymbolContext:</div><div class=""><br class=""></div><div class="">SymbolContext sym_ctx(sym_ctx_ref);</div><div class=""><br class=""></div><div class="">If the module is gone, then we need to NULL out comp_unit, function, block, symbol and variable (since they are owned by the Module) and clear the line_entry. If the module is still around, then these things can stay as is and the local SymbolContext now has a strong reference to the module and or target.</div><div class=""><br class=""></div><div class="">IRExecutionUnit currently uses the symbol context for name lookups, so I am not even sure if it is a good idea for it to store one permanently, but if it we use SymbolContextRef, then I don't care as much.</div><div class=""><br class=""></div><div class="">So in general think about who owns what and what should keep things alive. In this case it sounds like making SBBreakpoint, SBBreakpointLocation, and SBWatchpoint use std::weak_ptr would solve this part of this issue, but I still don't like a IRExecutionUnit being able to keep a module around, so we should probably make a SymbolContextRef. I am guessing if you switch SBBreakpoint over to use std::weak_ptr all the expressions will go away on their own.</div><div class=""><br class=""></div><div class="">Greg Clayton</div><div class=""><br class=""></div></body></html>