Debugger: Conditional breakpoints

[smurf3tte]

Conditional breakpoints! Like breakpoints, but they only work some of the time! Er...

Note: I am seeking feedback on the design and implementation of this feature. Don't be shy!

Who is this for

• People investigating emulation bugs
• People reverse engineering games
• ...
• Me 😀

What does it do

Allow a C-style expression to be attached to breakpoints. The breakpoint will only trigger a break in execution if the expression evaluates to true.

Why is this useful

There is often code that executes many times but is only interesting under specific conditions. This gives users a way to find the signal in the noise. It's a pretty common feature in debuggers.

How does it work

When creating a breakpoint, there is now a text box for an optional condition expression. As an example, in Final Fantasy Crystal Chronicles (GCCE01), setting a breakpoint at address 80017B18 with the following condition will trigger a break right before the game performs a texture copy that will overrun the destination buffer: r26 <= r0 && ((r5+3)&-4) * ((r6+3)&-4) * 4 > r0 (see #9329 for details).

Design considerations

• Basis for expression engine Currently using expr with a slight modification (replacing float with double, which can hold 32-bit integers at full precision), which is < 1k LOC in a single header and implements most of the standard C operators and supports variable assignment. Also considered TinyExpr, but it doesn't support bitwise, logical, comparison, or assignment operators. There's also cparse, which seems to handle a superset of what expr does but is ~3x larger.
• Allow mutation of guest state? Allowing expressions to modify guest state, such as registers or memory, would be only marginal additional work. Expressions with side effects require extra care from the user, but they seem useful nonetheless.
• Allow user-defined variables? Expression local variables e.g. ("x = r3+r4, x > 42") would permit some expressions to be simplified considerably. The bigger questions are whether variables should persist across evaluations of a given expression, and whether they should be shared between expressions. There is a lot of power in a persistent global scope for variables, but they could become frustrating to use without an additional means for inspection. We may want to persist them to save states as well.
• Future debugger enhancements? An obvious next step would be to add an immediate window, or REPL, where the user can enter any expression they like for immediate evaluation. This could be extended to support debugger commands, as well. There are other PRs that go further and wire the emulator to a full scripting language (see #9205, #7064). What I am suggesting is something a bit simpler: exposing the existing debugger functionality - currently accessible only to the GUI - through a text interface.

TODO

• ✔️ UI for condition expression on new breakpoints
• ✔️ Syntax validation at breakpoint creation time
• ✔️ Show condition in breakpoint list UI
• ✔️ Predicate breakpoint hits on result of expression evaluation
• ✔️ Register access (read/write) including GPRs, FPRs, LR, CTR, PC (read-only)
• ✔️ Guest memory read/write functions (read_u32(), write_u32(), etc.)
• ✔️ Convenience casts (u16(), s32(), etc.)
• ✔️ Integrate with breakpoint list save/load
• ✔️ Cache compiled expression for performance
• ✔️ Accept hexadecimal literals

Cut/postponed

• Support for conditional memory watches
• Edit button for breakpoints
• Watch widget integration
• Immediate widget for expression evaluation
• Persistent/global expression variables

[iwubcode]

Basis for expression engine

It would likely be a bigger task but inputs already uses an expression parser. It'd be nice in general if we could split that out and leverage it in other places (such as this). I know several times I've thought about a feature that required some complex expression parsing and shied away from it.

EDIT: we don't have to use the inputs one, I just think it'd be nice to standardize on a single expression parser.

[Rumi-Larry]

I'm guessing that this pr makes https://github.com/dolphin-emu/dolphin/pull/6728/files completely obsolete, doesn't it?

[smurf3tte]

Basis for expression engine

It would likely be a bigger task but inputs already uses an expression parser. It'd be nice in general if we could split that out and leverage it in other places (such as this). I know several times I've thought about a feature that required some complex expression parsing and shied away from it.

EDIT: we don't have to use the inputs one, I just think it'd be nice to standardize on a single expression parser.

I did notice there was a parser for input mappings, but it seemed fairly purpose-built. Trying to generalize it appeared to be more effort (and would risk breaking the input code) than introducing a general purpose (but small) expression parser. I am open to feedback on this point, though I would prefer not to write a parser from scratch or make sweeping changes to an existing one. 😀

[smurf3tte]

I'm guessing that this pr makes https://github.com/dolphin-emu/dolphin/pull/6728/files completely obsolete, doesn't it?

That PR actually looks like it would still be very useful. It would be nice to be able to disable breakpoints without deleting them. I may dust that one off if the original author has moved on.

[smurf3tte]

I believe I've addressed everything in the first round of feedback. I removed my strawman global variable implementation for now; I may add them back later. In the meantime I'll be starting down the TODO list.

[sepalani]

I'm guessing that this pr makes https://github.com/dolphin-emu/dolphin/pull/6728/files completely obsolete, doesn't it?

That PR actually looks like it would still be very useful. It would be nice to be able to disable breakpoints without deleting them. I may dust that one off if the original author has moved on.

The initial purpose of this PR is unrelated to what my PR is doing. The goal is to expose debugging features (in that case BPs) though the debugger interface. That interface could be used later for scripting languages such as python/lua. Feel free take over my PR or create your own as this one is really low on my TODO list.

[smurf3tte]

For reference, the last force push was to sneak "pc" back in as a built-in variable.

[Pokechu22]

This doesn't seem to work correctly with "write to log"; if I create a conditional breakpoint in Super Mario Sunshine at 802f3630 (J3DGDSetChanCtrl) with the condition r3 == 5, the following gets logged (heavily abbreviated):

43:27:735 Core\PowerPC\PowerPC.cpp:615 N[MI]: BP 802f3630  --- (00000000 00000000 00000000 00000001 00000000 00000002 00000001 0000100d 00000000 802d7aa8) LR=802d7b68
43:27:736 Core\PowerPC\PowerPC.cpp:615 N[MI]: BP 802f3630  --- (00000002 00000000 00000000 00000001 00000000 00000002 00000001 00000001 00000001 802d7aa8) LR=802d7b68
43:27:736 Core\PowerPC\PowerPC.cpp:615 N[MI]: BP 802f3630  --- (00000001 00000001 00000000 00000000 00000000 00000000 00000000 00000001 00000001 802d7aa8) LR=802d7b68
43:27:736 Core\PowerPC\PowerPC.cpp:615 N[MI]: BP 802f3630  --- (00000003 00000000 00000000 00000000 00000000 00000000 00000002 00000001 00000000 802d7aa8) LR=802d7b68

The code that does this logging is this, and the first value in parentheses is r3, but it's getting logged even though it's clearly not equal to 5:

https://github.com/dolphin-emu/dolphin/blob/c21581a91219b50919b21ea1938d3420fe61dc57/Source/Core/Core/PowerPC/PowerPC.cpp#L609-L616

It doesn't actually break in those cases, but this does make conditional breakpoints unusable for e.g. checking how many times a specific condition is hit per frame (which is something I wanted to do). Checking the condition in IsBreakPointLogOnHit would solve this (though that does mean that the condition is evaluated twice for "Write to Log and Break" breakpoints).

[TryTwo]

Will any more work be done on this?

I was thinking BPs with a specific call stack conditional would be really helpful for me. I don't think doing it as an individual PR would be right, but I imagine it could be added onto this after it gets merged.

[JoshuaMKW]

I would love to see this completed and merged. One of my biggest time wasters when debugging with breakpoints is pressing play 100+ times until I stumble across the correct instance of the breakpoint for what I need.

This would completely eliminate that and also make debugging with Dolphin more enticing.

[smurf3tte]

Completed with #11015. Thanks to @TryTwo for getting it across the finish line.