Conditional breakpoints #11015

TryTwo · 2022-08-28T19:14:15Z

A rebase of abandoned #9334 and fixed the one issue posted in that PR.

To Do in future PRs:

Add callstack-based conditionals.
Add string support for symbol conditional.
Expand to memory breakpoints.
Improve writing to registers/memory. Does not seem to work conditionally.
Make a help window.

If anyone thinks I should add any of those points to this PR let me know.

Jit and JitAsm have " ABI_CallFunction(PowerPC::CheckBreakPoints); ", which I think causes the breakpoint check to happen twice and two breakpoint log messages are made for one hit. Is that intentional?

/edit Check my private branch here for a branch with callstack and memory breakpoints.

TryTwo · 2022-08-31T15:46:48Z

Can I run a format on the external expr.h? I need to edit it and it's annoying with the format style mismatches.

sepalani

Can I run a format on the external expr.h? I need to edit it and it's annoying with the format style mismatches.

I'm fine with it as long as it's on a separated commit.

I don't have the expertise to review expr, however.

Source/Core/Core/PowerPC/BreakPoints.cpp

Source/Core/DolphinLib.props

Source/Core/DolphinQt/Debugger/BreakpointWidget.cpp

Source/Core/Core/PowerPC/Expression.cpp

Source/Core/Core/PowerPC/Expression.h

TryTwo · 2022-09-03T11:42:17Z

Thanks for the review. Since the original commits aren't mine should I add the fixes in a new commit or is normal to just edit the old ones?

sepalani · 2022-09-03T11:49:48Z

Thanks for the review. Since the original commits aren't mine should I add the fixes in a new commit or is normal to just edit the old ones?

I think editing the original commit is fine as long as it retains authorship.

TryTwo · 2022-09-03T15:00:23Z

Ok great. Also, trying to add fixes to the correct commit is a pain. Is it okay if I squash it? Some newer commits are overwriting changes in older commits anyway.

sepalani · 2022-09-03T17:02:31Z

Sure feel free to as long as it doesn't make them too big or hard to review I guess.

TryTwo · 2022-09-03T23:11:08Z

Hopefully my commit squash/re-arrange is acceptable. I finally forced VS to fix the props files with an Untabify option.

Not 100% sure what includes are needed in Expression.cpp there might have been leftovers that aren't needed.

sepalani

It's working as intended with some caveats. I don't know when an expression I've written is valid. For instance, foo == bar is evaluated to true, ditto for 1 / 0. I also think there should be a help button to explain what kind of condition is intended and what is allowed/set (especially for variables) and what isn't.

If you lack of inspiration for the help message, I've written one in one of my draft.

Source/Core/Core/PowerPC/Expression.cpp

TryTwo · 2022-09-05T17:58:02Z

When I add conditionals for memory break points, it will use the same conditional box, so I will have to move things around in the UI along with the help button.

Should we have multiple help buttons at the bottom or just have the conditional one inline with the condition box?

Pokechu22 · 2022-09-05T18:01:44Z

Thanks for the review. Since the original commits aren't mine should I add the fixes in a new commit or is normal to just edit the old ones?

You may also want to look into Co-authored-by.

TryTwo · 2022-09-07T22:39:46Z

Thanks, but adding co-author seems like a big hassle for my setup. Only using windows command line + visual studio, neither of which supports adding it. I think it's okay as-is.

AdmiralCurtiss · 2022-09-07T22:43:25Z

?? What do you mean 'neither supports'? It's just part of the commit message, and you clearly can write commit messages.

TryTwo · 2022-09-08T00:35:53Z

I don't believe it's just adding a footnote to the commit. It's a special formatting to update the embedded author stuff.

sepalani · 2022-09-08T01:30:03Z

Thanks, but adding co-author seems like a big hassle for my setup. Only using windows command line + visual studio, neither of which supports adding it. I think it's okay as-is.

My current setup is similar and I don't think I had this issue. Which specific steps did you have trouble with? You can use git rebase -i or git commit --amend to edit the commit message using an interactive way. As Pokechu22 and AdmiralCurtiss said it's just part of the commit message:

https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors

JosJuice · 2022-09-08T16:22:01Z

I don't believe it's just adding a footnote to the commit. It's a special formatting to update the embedded author stuff.

Author and committer yes, co-author no.

TryTwo · 2022-09-08T16:36:58Z

Ohhh. Thanks for clearing that up. It seemed like it would change the author/committer stuff, but I guess not!

TryTwo · 2022-09-10T22:47:18Z

What's next for this? Does expr.h need a full review? And are the templates returning double acceptable as-is?

I didn't find any bugs when testing the various methods I put in the help commit.

sepalani

What's next for this? Does expr.h need a full review?

Ideally, yes. Though, it's only used in the debugger, in a single entry-point being the BP's condition so I'm not strongly opposed to the inclusion of this external library.

And are the templates returning double acceptable as-is?

I'm fine with the current state as its documented.

Source/Core/DolphinQt/Debugger/NewBreakpointDialog.cpp

Source/Core/Core/PowerPC/Expression.cpp

Expression class to store compiled expressions and associated variable list. Co-authored-by: TryTwo <taolas@gmail.com>

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

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. Co-authored-by: TryTwo <taolas@gmail.com>

Co-authored-by: TryTwo <taolas@gmail.com>

TryTwo · 2022-10-07T04:49:41Z

Rebase no changes

Source/Core/Core/PowerPC/Expression.cpp

TryTwo · 2022-10-08T07:59:50Z

Applying the standard formatting to expr.h without any other changes first. Then I will add the changes, so it is easy to see in Compare.

TryTwo · 2022-10-08T08:40:59Z

Ok I hit a problem from lack of experience. static double expr_eval(struct expr* e) is recursive. It keeps calling itself to chunk through each part of the expression until it's done. We want to see if any individual calculation comes back NaN. That's easy, but then we want to return a flag if it's NaN. If I send bool* hit_a_nan into the function, so I can flag it, then would I have to edit it into every recursive function call as well?

I feel like that's not the proper way to do it.

TryTwo · 2022-10-08T20:28:19Z

Alright, removing the changes to expr.h and going to create a new logging/error checking commit for expression.

TryTwo · 2022-10-08T22:02:46Z

Still needs a little work. Couldn't decide on the best way to continually append the string. stringstream? sprintf? fmt?

Also, I was thinking any variable that starts with an "r" could be changed into a 32 bit hexadecimal string. It would give more meaningful output to registers and some user control over the output format. Or does that seem like too much?

TryTwo · 2022-10-10T00:41:47Z

The only real issue I see is how spammy it will be if you encounter a NaN. Especially the OSD spam. Not sure if it can be told to only make an OSD if one doesn't exist or something.

I still need to update the help button.

sepalani

Beside that and the lint error, this seems to work as expected.

Source/Core/Core/PowerPC/Expression.cpp

TryTwo · 2022-11-04T12:16:24Z

@AdmiralCurtiss What do you think, ready to go?

I've got two/three more PRs ready to add more features to this.

AdmiralCurtiss

Yeah this looks good to me.

sepalani suggested changes Sep 2, 2022

View reviewed changes

TryTwo force-pushed the Conditional_Breakpoints branch 2 times, most recently from afefdae to 634ff62 Compare September 3, 2022 23:05

sepalani suggested changes Sep 4, 2022

View reviewed changes

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

TryTwo force-pushed the Conditional_Breakpoints branch from 757dae7 to 0ba3644 Compare September 5, 2022 20:19

TryTwo force-pushed the Conditional_Breakpoints branch from 0ba3644 to eb883eb Compare September 8, 2022 17:36

sepalani reviewed Oct 6, 2022

View reviewed changes

Source/Core/DolphinQt/Debugger/NewBreakpointDialog.cpp Outdated Show resolved Hide resolved

Source/Core/Core/PowerPC/Expression.cpp Show resolved Hide resolved

smurf3tte and others added 5 commits October 6, 2022 21:34

Debugger: Initial implementation of conditional breakpoints

7842f9a

Expression class to store compiled expressions and associated variable list. Co-authored-by: TryTwo <taolas@gmail.com>

Expand expr's support for numeric literals

11851a0

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

Grow internal expr int representation to 64 bits

3417920

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.

1224575

This allows for easy reinterpretation of GPRs in an expression. Co-authored-by: TryTwo <taolas@gmail.com>

Support memory reads/writes from breakpoint expressions

85cb4d1

Co-authored-by: TryTwo <taolas@gmail.com>

TryTwo force-pushed the Conditional_Breakpoints branch from eb883eb to 8197932 Compare October 7, 2022 04:49

TryTwo force-pushed the Conditional_Breakpoints branch from 8197932 to 54cf83c Compare October 7, 2022 04:50

TryTwo force-pushed the Conditional_Breakpoints branch from 54cf83c to 1a06161 Compare October 8, 2022 03:18

sepalani suggested changes Oct 8, 2022

View reviewed changes

Source/Core/Core/PowerPC/Expression.cpp Show resolved Hide resolved

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

TryTwo force-pushed the Conditional_Breakpoints branch from 1a06161 to d51cb36 Compare October 8, 2022 07:58

NewBreakpointDialog: Add a help message for conditionals.

1ed6028

TryTwo force-pushed the Conditional_Breakpoints branch from d51cb36 to 1ed6028 Compare October 8, 2022 20:27

TryTwo force-pushed the Conditional_Breakpoints branch from 54e7a92 to 144b431 Compare October 10, 2022 00:40

TryTwo force-pushed the Conditional_Breakpoints branch from 144b431 to ec80511 Compare October 11, 2022 07:47

sepalani suggested changes Oct 23, 2022

View reviewed changes

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

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

Add logging and error reporting to Breakpoint Conditionals.

e2df81b

TryTwo force-pushed the Conditional_Breakpoints branch from ec80511 to e2df81b Compare October 23, 2022 15:22

AdmiralCurtiss approved these changes Nov 13, 2022

View reviewed changes

AdmiralCurtiss merged commit 2a81fa6 into dolphin-emu:master Nov 13, 2022
11 checks passed

smurf3tte mentioned this pull request Nov 30, 2022

Debugger: Conditional breakpoints #9334

Closed

Conditional breakpoints #11015

Conditional breakpoints #11015

TryTwo commented Aug 28, 2022 •

edited

TryTwo commented Aug 31, 2022

sepalani left a comment •

edited

TryTwo commented Sep 3, 2022

sepalani commented Sep 3, 2022

TryTwo commented Sep 3, 2022

sepalani commented Sep 3, 2022

TryTwo commented Sep 3, 2022

sepalani left a comment

TryTwo commented Sep 5, 2022

Pokechu22 commented Sep 5, 2022

TryTwo commented Sep 7, 2022

AdmiralCurtiss commented Sep 7, 2022

TryTwo commented Sep 8, 2022

sepalani commented Sep 8, 2022

JosJuice commented Sep 8, 2022

TryTwo commented Sep 8, 2022

TryTwo commented Sep 10, 2022

sepalani left a comment

TryTwo commented Oct 7, 2022

TryTwo commented Oct 8, 2022

TryTwo commented Oct 8, 2022

TryTwo commented Oct 8, 2022

TryTwo commented Oct 8, 2022 •

edited

TryTwo commented Oct 10, 2022

sepalani left a comment

TryTwo commented Nov 4, 2022

AdmiralCurtiss left a comment

Conditional breakpoints #11015

Conditional breakpoints #11015

Conversation

TryTwo commented Aug 28, 2022 • edited

TryTwo commented Aug 31, 2022

sepalani left a comment • edited

Choose a reason for hiding this comment

TryTwo commented Sep 3, 2022

sepalani commented Sep 3, 2022

TryTwo commented Sep 3, 2022

sepalani commented Sep 3, 2022

TryTwo commented Sep 3, 2022

sepalani left a comment

Choose a reason for hiding this comment

TryTwo commented Sep 5, 2022

Pokechu22 commented Sep 5, 2022

TryTwo commented Sep 7, 2022

AdmiralCurtiss commented Sep 7, 2022

TryTwo commented Sep 8, 2022

sepalani commented Sep 8, 2022

JosJuice commented Sep 8, 2022

TryTwo commented Sep 8, 2022

TryTwo commented Sep 10, 2022

sepalani left a comment

Choose a reason for hiding this comment

TryTwo commented Oct 7, 2022

TryTwo commented Oct 8, 2022

TryTwo commented Oct 8, 2022

TryTwo commented Oct 8, 2022

TryTwo commented Oct 8, 2022 • edited

TryTwo commented Oct 10, 2022

sepalani left a comment

Choose a reason for hiding this comment

TryTwo commented Nov 4, 2022

AdmiralCurtiss left a comment

Choose a reason for hiding this comment

TryTwo commented Aug 28, 2022 •

edited

sepalani left a comment •

edited

TryTwo commented Oct 8, 2022 •

edited