[Lldb-commits] [lldb] [lldb] Speed up evaluating breakpoint conditions by using DIL (PR #224740)

[via lldb-commits]

jimingham wrote:

> > Actually, a more convenient presentation might be:
> > --condition-mode <dil, expr, dwim> - defaults to dwim
> > --condition <condition_text>
> > That's better, because you don't have to outlaw incompatible options: you can't use both --dil-condition and --expr-condition at the same time.
> > If you are tempted by this solution, I'd suggest only doing it for "breakpoint add" and not "breakpoint set". Just leave -c in breakpoint set to be dwim-condition. Adding options to break set is kind of a mooks game at this point...
> 
> If I understood correctly, there is no option group for all "breakpoint add" subcommands. So I just added `--condition-mode` next to `-c` option, so that it can be accessed from other subcommands like `breakpoint set` etc. Might as well, doesn't seem to hurt?
> 
> > Again, I think the help for -c (or for the --condition-mode if you go that way). I can't remember whether break add and break set share the same condition option. If they don't, make sure you add it to both.
> 
> Done. Same option, that's why I made the `--condition-mode` shared as well.

Yes, I made a separate OptionGroup for the "configuration options on a breakpoint that don't specify the Resolver".  And then that's shared with "breakpoint set", "breakpoint add" and "breakpoint modify".  This is such an option so that's the right place to put it.

> 
> > I also think that we will need the "use expr for conditions" switch permanently - since we don't have any plans to make DIL expressions always produce the same result as expr expressions.
> 
> One thing I had in mind when I added the setting: what if someone uses LLDB from an IDE? They won't be able to change the mode with command line options. With the setting, it can be changed globally. Should I still add the setting that changes the default mode? Can have both that and the options. And on this note, should we also make it possible to choose the mode via `SBBreakpoint` API?

There should definitely be an `SBBreakpoint::{Set,Get}ConditionMode`.  Since this doesn't affect the resolver, you don't need to have a way to set it on construction, so this should be a straightforward addition - just these two methods.  

It would be okay to also have a global setting, but not for the purposes of an IDE.  IDE's should be managing breakpoints using the SB API's so they can use SetConditionMode directly.  That's also a more robust way to do this, since then users can't change the behavior out from under the UI by changing the setting.

The main reason for the property would be for people who don't agree that dwim should be the default, and don't want to have to alias all the breakpoint commands to change this per-invocation.

> 
> > Ignore this comment. I had actually already solved this problem by having the parsed conditions live in locations, not in the breakpoint. So the context in which the parsed expression is rerun is guaranteed to be the same.
> 
> So, if we ask to create a breakpoint on a symbol that can have multiple contexts, it creates several breakpoint locations? I assume this is why the code is in `BreakpointLocation` class. I don't need to worry about context changing within the location after all?

Exactly.

> 
> > DIL will never be able to call user functions, though, right? That would require actually running code, which I thought we agreed DIL wasn't going to do? But this is fine. Making it a static_cast makes the fact that this can't be run in DIL a slightly more esoteric bit of knowledge, but that's neither here nor there.
> 
> Well... we might want to discuss that again at some point. It was suggested in the initial DIL RFC, along with Swift properties. I already made a proof of concept implementation of calling C++ functions via ABI. I feel like it could be very useful to at least be able to call functions without parameters, so much data lookup is dependent on `.GetSomething()` methods.

I think having a data inspection mode that doesn't cause the target to run is pretty valuable.  Once you start accessing properties, you start having to deal with "what if a property acquires a lock - a very reasonable thing for a property accessor to do - and someone else has the lock".  That would require the DIL expression to run all threads to clear the lock.  

That can be pretty bad If this DIL expression is done in the summary provider for some ValueObject.  Those usually get rendered automatically when presenting the Locals on a stop.  That allows the interaction where you select thread A in some GUI, and it's at Frame B.  Then you switch to Thread C, and its at Frame F which has a Local Variable whose summary formatter has to allow all threads to run.  If you are unlucky, thread A will be allowed to run, so then you switch back to Thread A and the stack frame is no longer at Frame B, even though the user never explicitly ran the program.

So even if we end up allowing this we're going to have to be careful how we trigger it, and be willing to fail when we can't safely do it.  And if I know I have properties that acquire more than one lock in the process of figuring out their values I 100% will want to turn this off to avoid debugger induced AB lock inversions.


https://github.com/llvm/llvm-project/pull/224740