Debugger: Conditional breakpoints #9334

smurf3tte · 2020-12-17T01:03:35Z

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 Lua scripting #9205, [WIP] Add Python Scripting support #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

Source/Core/Core/PowerPC/BreakPoints.cpp

iwubcode · 2020-12-17T06:58:31Z

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 · 2020-12-17T07:31:02Z

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

CMakeLists.txt

Source/Core/Core/PowerPC/BreakPoints.cpp

smurf3tte · 2020-12-17T16:11:38Z

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 · 2020-12-17T16:14:30Z

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 · 2020-12-17T22:05:53Z

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.

- Support hex prefix 0x or OX - Support scientific notation Also, reconcile the bitwise complement operator with C (use ~ instead of ^).

To prevent the INI parser from interpreting breakpoints as key/value pairs when the condition contains an equals sign, they are now prefixed with '$' (understood by the parser to be a raw line designator). Load support is retained for breakpoints without this prefix so old INIs will still work.

Encapsulate expressions in new Expression class that stores the compiled expression and associated variable list.

This is necessary to retain precision above 32 bits, but more importantly to prevent an expression like (0x80000000 | 1) from flipping the sign of the result.

This allows for easy reinterpretation of GPRs in an expression.

…oints - Add access to FPRs - Add access PC, LR, CTR - Support modifications to all of the above + GPRs

Source/Core/Core/PowerPC/BreakPoints.cpp

Source/Core/Core/PowerPC/Expression.cpp

Source/Core/DolphinQt/Debugger/NewBreakpointDialog.cpp

sepalani · 2020-12-20T17:33:49Z

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.

There was a map here. It's gone now.

smurf3tte · 2020-12-20T22:36:41Z

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

Pokechu22 · 2021-01-29T21:02:48Z

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:

dolphin/Source/Core/Core/PowerPC/PowerPC.cpp

Lines 609 to 616 in c21581a

    
           if (PowerPC::breakpoints.IsBreakPointLogOnHit(PC)) 
        
           { 
        
             NOTICE_LOG_FMT(MEMMAP, 
        
                            "BP {:08x} {}({:08x} {:08x} {:08x} {:08x} {:08x} {:08x} {:08x} {:08x} {:08x} " 
        
                            "{:08x}) LR={:08x}", 
        
                            PC, g_symbolDB.GetDescription(PC), GPR(3), GPR(4), GPR(5), GPR(6), GPR(7), 
        
                            GPR(8), GPR(9), GPR(10), GPR(11), GPR(12), LR); 
        
           }

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 · 2022-05-18T22:02:00Z

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 · 2022-07-14T03:03:20Z

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 · 2022-11-30T13:42:26Z

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

phire reviewed Dec 17, 2020

View reviewed changes

Source/Core/Core/PowerPC/BreakPoints.cpp Outdated Show resolved Hide resolved

Source/Core/Core/PowerPC/BreakPoints.cpp Outdated Show resolved Hide resolved

lioncash reviewed Dec 17, 2020

View reviewed changes

smurf3tte force-pushed the cond_bp branch from 011ce33 to 01f5d47 Compare December 17, 2020 05:12

leoetlino reviewed Dec 17, 2020

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

Source/Core/Core/PowerPC/BreakPoints.cpp Outdated Show resolved Hide resolved

Source/Core/Core/PowerPC/BreakPoints.cpp Outdated Show resolved Hide resolved

Source/Core/Core/PowerPC/BreakPoints.cpp Outdated Show resolved Hide resolved

BhaaLseN reviewed Dec 17, 2020

View reviewed changes

Source/Core/Core/PowerPC/BreakPoints.cpp Outdated Show resolved Hide resolved

Source/Core/Core/PowerPC/BreakPoints.cpp Outdated Show resolved Hide resolved

smurf3tte force-pushed the cond_bp branch from 01f5d47 to 99713aa Compare December 17, 2020 22:03

Debugger: Initial implementation of conditional breakpoints

9b82158

smurf3tte force-pushed the cond_bp branch from 99713aa to 9b82158 Compare December 17, 2020 22:05

smurf3tte added 8 commits December 17, 2020 17:01

Expand expr's support for numeric literals

abfd6fc

- Support hex prefix 0x or OX - Support scientific notation Also, reconcile the bitwise complement operator with C (use ~ instead of ^).

Support memory reads/writes from breakpoint expressions

33d191f

Cache compiled breakpoint condition expressions

9af5845

Encapsulate expressions in new Expression class that stores the compiled expression and associated variable list.

Grow internal expr int representation to 64 bits

1db7f85

This is necessary to retain precision above 32 bits, but more importantly to prevent an expression like (0x80000000 | 1) from flipping the sign of the result.

Add cast expression functions u8(), s8(), u16(), etc.

954288c

This allows for easy reinterpretation of GPRs in an expression.

Validate condition expression in new breakpoint dialog

887bba6

Support GPR, FPR, and SPR access (read/write) from conditional breakp…

a2e0223

…oints - Add access to FPRs - Add access PC, LR, CTR - Support modifications to all of the above + GPRs

smurf3tte marked this pull request as ready for review December 19, 2020 23:21

lioncash reviewed Dec 20, 2020

View reviewed changes

smurf3tte force-pushed the cond_bp branch from 83b241d to f27ec8b Compare December 20, 2020 09:38

Bind variables at expression compile time

c21581a

There was a map here. It's gone now.

smurf3tte force-pushed the cond_bp branch from f27ec8b to c21581a Compare December 20, 2020 22:33

smurf3tte mentioned this pull request Jan 25, 2021

Breakpoint from string flag parsing using token #9456

Merged

TryTwo mentioned this pull request Aug 28, 2022

Conditional breakpoints #11015

Merged

smurf3tte closed this Nov 30, 2022

Pokechu22 mentioned this pull request Nov 18, 2023

expr: increase size of varname #12305

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Debugger: Conditional breakpoints #9334

Debugger: Conditional breakpoints #9334

smurf3tte commented Dec 17, 2020 •

edited

Loading

iwubcode commented Dec 17, 2020 •

edited

Loading

Rumi-Larry commented Dec 17, 2020

smurf3tte commented Dec 17, 2020

smurf3tte commented Dec 17, 2020

smurf3tte commented Dec 17, 2020

sepalani commented Dec 20, 2020

smurf3tte commented Dec 20, 2020

Pokechu22 commented Jan 29, 2021

TryTwo commented May 18, 2022

JoshuaMKW commented Jul 14, 2022

smurf3tte commented Nov 30, 2022

Debugger: Conditional breakpoints #9334

Debugger: Conditional breakpoints #9334

Conversation

smurf3tte commented Dec 17, 2020 • edited Loading

Who is this for

What does it do

Why is this useful

How does it work

Design considerations

TODO

Cut/postponed

iwubcode commented Dec 17, 2020 • edited Loading

Rumi-Larry commented Dec 17, 2020

smurf3tte commented Dec 17, 2020

smurf3tte commented Dec 17, 2020

smurf3tte commented Dec 17, 2020

sepalani commented Dec 20, 2020

smurf3tte commented Dec 20, 2020

Pokechu22 commented Jan 29, 2021

TryTwo commented May 18, 2022

JoshuaMKW commented Jul 14, 2022

smurf3tte commented Nov 30, 2022

smurf3tte commented Dec 17, 2020 •

edited

Loading

iwubcode commented Dec 17, 2020 •

edited

Loading