Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Debugger: Stability improvements #11009

Merged
merged 5 commits into from
Apr 8, 2024
Merged

Debugger: Stability improvements #11009

merged 5 commits into from
Apr 8, 2024

Conversation

CookiePLMonster
Copy link
Contributor

@CookiePLMonster CookiePLMonster commented Mar 31, 2024 •
edited
Loading

Description of Changes

This PR fixes numerous issues related to the debugger:

  1. Fixes an use-after-free where breakpoints were updated after DebuggerWindow shut down already.
  2. Adds a Hardcore Mode disable prompt when starting Boot and Debug with HC enabled.
  3. Marks addresses < 0xBFC00000 as invalid, fixing asserts in Debug and possible crashes in Release (by @F0bes).
  4. Fixes numerous data races in the code, such as:
    • Unbinding a debug interface update CB while it's in use, causing a possible use-after-free.
    • Binding breakpoints via the disassembly widget that would read a stale local variable, and bind the breakpoint to a bogus address.

Rationale behind Changes

Stability good.

Suggested Testing Steps

  1. Test Boot and Debug with Hardcore Mode disabled.
  2. Test Boot and Debug with Hardcore Mode enabled.
  3. Test binding and unbinding breakpoints by double clicking on the Disassembly Widget.
  4. Test binding and unbinding breakpoints by adding them to the Breakpoints tab manually.

@CookiePLMonster CookiePLMonster marked this pull request as draft March 31, 2024 21:45
@CookiePLMonster CookiePLMonster force-pushed the boot-and-debug-crash-fix branch from 311f3f8 to 6969fe0 Compare April 1, 2024 16:19
@CookiePLMonster
Copy link
Contributor Author

Updated the PR to prompt for HC disable when using Boot and Debug, same as when resume savestates are used.

Left it as a draft still because it introduces an issue where clicking Yes and disabling HC with Boot and Debug brings up an assertion:
image

Once I fix this defect, I'll un-draft.

@CookiePLMonster CookiePLMonster force-pushed the boot-and-debug-crash-fix branch from 6969fe0 to 4fd1169 Compare April 6, 2024 18:26
@CookiePLMonster CookiePLMonster marked this pull request as ready for review April 6, 2024 18:26
@CookiePLMonster
Copy link
Contributor Author

Updated the PR and it's now ready for review. The above bug is not fixed, but I have identified that:

  1. It only triggers in Debug.
  2. It triggers without my PR in the exact same conditions - just try to go to the address 0xbfc00000, and the same assertion hits.

Therefore, I see no reason to block this PR on this other, unrelated debugger issue.

@F0bes F0bes force-pushed the boot-and-debug-crash-fix branch from 7678c55 to 708c25a Compare April 6, 2024 21:36
F0bes
F0bes approved these changes Apr 6, 2024
View reviewed changes
Copy link
Member

@F0bes F0bes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

708c25a Fixes the assert issue.
Everything else looks good to me.

Daniel-McCarthy
Daniel-McCarthy approved these changes Apr 7, 2024
View reviewed changes
Copy link
Contributor

@Daniel-McCarthy Daniel-McCarthy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, great catch!

@stenzek
Copy link
Contributor

stenzek commented Apr 7, 2024

Achievement-related changes seem fine. But the race scares me, I'd prefer we avoid that if possible. I know Silent had some fun issues debugging this, it's best to not create even more of them ;)

@CookiePLMonster CookiePLMonster marked this pull request as draft April 7, 2024 11:46
@CookiePLMonster CookiePLMonster changed the title Debugger: Fix Boot and Debug when Hardcore Mode is enabled Debugger: Stability improvements Apr 7, 2024
@CookiePLMonster CookiePLMonster requested review from stenzek and F0bes April 7, 2024 20:51
@CookiePLMonster CookiePLMonster force-pushed the boot-and-debug-crash-fix branch from f963bdc to 895bd34 Compare April 7, 2024 20:56
@CookiePLMonster CookiePLMonster marked this pull request as ready for review April 7, 2024 21:06
@CookiePLMonster
Copy link
Contributor Author

Updated the PR with some heavy-handed changes to data flow that completely breakpoint update handler and replaces it with proper Qt slots and signals. Everything breakpoints-related should now be done on the correct threads.

I have tested breakpoints a bit and they appear to work fine, but please test them for regressions!!!

@CookiePLMonster
Copy link
Contributor Author

Snuck one more improvement in - now Go to in Memory View opens the Memory View tab. It was already doing that in one case (Saved Addresses tab), but that shortcut can be used from more places.

F0bes
F0bes approved these changes Apr 7, 2024
View reviewed changes
Copy link
Member

@F0bes F0bes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested it and the code looks good to me.
I didn't look for possible races.
I'm glad that breakpoint update handler code (that I was not proud of) was able to be removed.

stenzek
stenzek reviewed Apr 8, 2024
View reviewed changes
Copy link
Contributor

@stenzek stenzek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems okay otherwise I think.

pcsx2-qt/Debugger/CpuWidget.cpp Outdated Show resolved Hide resolved
@CookiePLMonster CookiePLMonster force-pushed the boot-and-debug-crash-fix branch from 6e0e939 to c8561ca Compare April 8, 2024 12:44
@CookiePLMonster CookiePLMonster requested a review from stenzek April 8, 2024 12:45
@CookiePLMonster CookiePLMonster force-pushed the boot-and-debug-crash-fix branch from c8561ca to fd01c7e Compare April 8, 2024 12:46
F0bes and others added 3 commits April 8, 2024 20:36
@F0bes @CookiePLMonster
DebugTools: Properly Mark Addresses < 0xBFC00000 as Invalid
4b8b3d6 
Fixes asserts and possible crashes in release when these addresses are accessed in the debugger.
@CookiePLMonster
Debugger: Untangle the breakpoints data flow to resolve races
3742c36 
Tightens the data flow between the CPU and UI threads
to resolve multiple race conditions, such as:
1. Unbinding a debug interface update CB while it's in use,
    causing a possible use-after-free.
2. Binding breakpoints via the disassembly widget that would read
    a stale local variable, and bind the breakpoint to a bogus address

+ probably more subtle races that are now resolved
@CookiePLMonster
Debugger/CPUWidget: Make "Go to in Memory View" open the Memory View
8399e4b
@CookiePLMonster CookiePLMonster force-pushed the boot-and-debug-crash-fix branch from 19c2017 to 8399e4b Compare April 8, 2024 18:36
@refractionpcsx2 refractionpcsx2 merged commit 91f16ae into PCSX2:master Apr 8, 2024
12 checks passed
@CookiePLMonster CookiePLMonster deleted the boot-and-debug-crash-fix branch April 8, 2024 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lightningterror lightningterror lightningterror left review comments

@F0bes F0bes F0bes approved these changes

@Mrlinkwii Mrlinkwii Mrlinkwii approved these changes

@kamfretoz kamfretoz kamfretoz approved these changes

@Daniel-McCarthy Daniel-McCarthy Awaiting requested review from Daniel-McCarthy

@stenzek stenzek Awaiting requested review from stenzek

Assignees
No one assigned
Labels
Debugger GUI/Qt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@CookiePLMonster @stenzek @Daniel-McCarthy @kamfretoz @Mrlinkwii @lightningterror @F0bes @refractionpcsx2