Skip to content

fix(agc): own resolved dispatch state through thread exit - #3661

Merged
oneandonlydean merged 1 commit into
boykopovar:mainfrom
Celegans12:fix/agc-own-resolved-dispatch-state
Oct 12, 2026
Merged

oneandonlydean merged 1 commit into
boykopovar:mainfrom
Celegans12:fix/agc-own-resolved-dispatch-state

Conversation

@Celegans12

Copy link
Copy Markdown
Contributor

What

A queue worker can access freed TLS storage while destroying its resolved-dispatch map on Windows. I traced a shutdown access violation from a draw-cache test to that map's destructor in MinGW's TLS callback. The test still returned zero and CTest reported a pass.

The map now uses the existing ThreadOwned helper: a trivial TLS pointer refers to a heap-owned map destroyed by its pthread-key callback. This applies the ownership rule from #590 to the map introduced by #1864. Dispatch lookup and clearing stay unchanged.

Both draw-cache test modes now fail on FATAL: output, including a background crash followed by exit zero. #3528's shader-ABI shutdown handling and #1202's draw-pipeline work leave this map unchanged.

The observed failure is intermittent; no deterministic RED is claimed. The separate stackless heap-corruption failure is not proven to have the same cause, and finite passing runs do not prove universal absence of the fault.

Tested

  • Based on main a11c8f7. Windows 11/NVIDIA and Linux/llvmpipe: Release -DBUILD_TESTING=ON, builds, libs and registration pass.
  • Four focused cases: Windows five serial runs each, 20/20 in 9.14 s; Linux 4/4 in 0.77 s. No skips or fatal output; both draw modes have the configured fatal-output guard.
  • Windows full: 827 passed, 4 failed, 5 expected skips, 86.34 s; CTest exit 8.
  • Linux full: 801 passed, 4 failed, 33 skipped, 347.51 s; CTest exit 8.
  • Both full suites fail; audits found no hidden unexpected native faults.
  • Windows failures: guest_kernel_sys, windows_exceptions, guest_filesystem, agc_driver_emulated_color_compare. All match exact main 1e8fff68, whose core matches a11c8f7f.
  • Linux failures: agc_driver_emulated_color_compare matches 1e8fff68; agc_driver_depth_compare_copy fails unchanged write-watch admission. agc_driver_image_plane_address_on_line_implicit_lod and guest_kernel_stack_protect only have historical 0f863c10 signature matches, not exact-current-main controls.
  • WSL lacks asynchronous write watching and compares memory; the skip checker rejects agc_write_tracking (exit 1).
  • No game or FPS claim.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • AI-assisted: yes

The resolved-dispatch map was a nontrivial thread_local object.
MinGW can run its destructor after emutls frees the backing storage,
causing an access violation when a queue worker exits.
Use the existing ThreadOwned helper to destroy the heap-owned map
through its pthread key while keeping a trivial TLS pointer.
Reject FATAL output in both draw-cache test modes so a background
shutdown failure cannot pass only because the process returns zero.
@Dextroid17

Copy link
Copy Markdown
Contributor

The fix is the right shape, and I checked whether it is complete rather than partial — it is.

The interesting part is not the map itself but that the same file holds five more thread_locals, so this could easily have been a partial fix. It isn't: GroupCaptureStats (Driver.hpp:226-230) is uint64_t/double/time_point members, and currentPacketOffset, resolvingAhead and gateOpened are scalars — all trivially destructible, so the compiler's TLS callback for them has nothing to do at teardown and they never had this problem. The std::map was the only object in this file — and in the rest of the Dispatch/ directory, where DispatchTiming.cpp's PendingDispatchPhases is likewise trivially destructible — whose destructor the late callback could run against freed storage, and it is the one this changes. ThreadOwned does what the description says it does: ThreadOwned.hpp keeps a pthread_key_t whose destructor walks a per-thread vector<Owned> in reverse and destroys each entry, so the map is released by pthread's thread-exit path instead of the compiler's TLS callback, and the TLS object itself is now a bare pointer with a trivial destructor.

The FAIL_REGULAR_EXPRESSION "FATAL:" line is the part I would not drop in review. A background access violation whose process still exits zero is invisible to CTest — the suite reports a pass while a thread died badly — so adding the fatal-output guard turns a silent pass into a failure for both draw-cache modes. That is worth as much as the fix itself, because it is what makes the next occurrence visible, and it is the same reasoning that makes the honest "no deterministic RED is claimed" line credible. (The one caveat: the guard only bites if the crash's text reaches the output CTest captures, and a hard fault mid-buffer can lose it — so it makes the next occurrence visible more reliably rather than guaranteeing it.)

The ordering question is settled by this repo's own #590, not open. That PR describes exactly this hazard: with MinGW GCC, thread_local is emutls, and thread exit happens in two steps in the wrong order — winpthread's key destructors, including libgcc's emutls one, free the thread's TLS blocks first, and the CRT's tls_callback → run_dtor_list then runs the C++ thread_local destructors on those already freed blocks. ThreadOwned removes that dependency rather than reordering it: the object is reached through a key the helper owns, so nothing reads emutls at teardown, and the window is closed rather than narrowed. That makes the FATAL: guard defence in depth for the next occurrence rather than a required companion to this fix.

Written with AI assistance.

@github-actions github-actions Bot added build agc libSceAgc* labels Oct 11, 2026
@Amoenus

Amoenus commented Oct 11, 2026

Copy link
Copy Markdown
Contributor

Independent Windows validation of unchanged head 09894076 against exact main a11c8f7f (AI-assisted).

Release, GCC 15.2/MinGW, Ryzen 9 9950X3D, RTX 5080; bundled dependencies, SPIRV-Tools OFF. Fresh full build, libs, registration and conventions passed. CTest used --timeout 30, full suites -j4; Vulkan required and shader cache disabled. No test reached 30 s.

  • Draw-cache reuse (both modes), pipeline cache and guest thread atexit: five serial repetitions each passed on peer, main and restored peer (60 runs, no skips).
  • Actual configured fatal-output rules reject a synthetic zero-exit FATAL: fixture on peer/restored peer; main accepts it. This checks the CTest guard, not the original native shutdown fault.
  • Exact ThreadOwned helper probe: 2,560 native thread lifetimes destroyed their owned maps; omitting ownership was detected in five controls. This is a helper check, not a resolved-dispatch fault reproducer.
  • Full peer: 824 passed / 7 failed / 5 skipped. Main: 827 / 4 / 5. Shared failures: guest_kernel_sys, windows_exceptions, guest_filesystem, agc_driver_emulated_color_compare.

The three extra peer failures reported 0xc0000374. Repeated serial controls also reproduced that signature on main for agc_driver_prepared_shaders_debug and agc_driver_prepared_shaders_deferred_undecodable, and on restored peer. agc_driver_compute_user_data_calls_registered subsequently passed 20 repetitions on each. These results do not establish a regression caused by this PR; they also do not show that all heap faults are fixed.

I did not independently reproduce or capture a stack for the specific TLS callback access violation. Linux and game startup were not run for this validation. The focused tests and output guard pass, with the full-suite failures and causal limitation above.

@github-actions

github-actions Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.

Pull request Conflicting files (lines on main) Same exports Same new files
#4029 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Dispatch/DispatchAhead.cpp L13-14

@oneandonlydean
oneandonlydean merged commit 9913780 into boykopovar:main Oct 12, 2026
26 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* build

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants