Repository navigation
Conversation
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
jotitadev
left a comment
jotitadev
left a comment
There was a problem hiding this comment.
The new guest_ult_finalize_allocation test deadlocks (hits its 20s TIMEOUT) on Linux. I root-caused it and it is the test harness, not the fix.
Machine: Linux, Ubuntu 26.04, GCC 15.2.0 (libstdc++ 6.0.35), x86-64. Merged this PR (65eb017f9) with git merge --no-commit onto current main, full build (BUILD_TESTING=ON, Ninja Release), ran ./tests/guest_ult_finalize_allocation_tests directly.
Symptom: the process hangs forever on the very first budget (budget=0), single-threaded (no fork/clone), blocked in a futex with no timeout:
futex(0x…860, FUTEX_WAIT_PRIVATE, 2, NULL) = ? ERESTARTSYS # never returns; wchan=futex_do_wait
Root cause — a re-entrant throw from the replaced global operator new. The futex is a non-recursive lock inside the C++ exception runtime. By instrumenting the test's own operator new I captured the sequence (this is the first loop iteration, allocationBudget == 0):
operator new size=16 budget=0 -> throw std::bad_alloc() # from semaphores.reserve(gSemaphores.size())
operator new size=72 budget=0 -> throw std::bad_alloc() # re-entrant: the runtime allocates through
# the replaced operator new WHILE the first
# exception is being thrown
<hang on futex>
While it is throwing/propagating the first bad_alloc, the C++ runtime performs another allocation, which is routed through the same replaced global operator new. Because allocationBudget is still 0, that re-entrant call throws a second bad_alloc from inside the runtime's exception machinery, while a non-recursive internal lock is held → self-deadlock on the futex. There is no second thread to ever wake it, so it waits forever.
Proof it is the re-entrancy (not the fix, not the environment):
- Reproduces identically with and without the production change (reverted
Export.cpp, kept only the test) — so it is not themutexes.reserve(...)reorder. - It is not
.11/environment: other libSceUlt guest tests (e.g.guest_ult_sync) and other guest tests run fine here. They do not replaceoperator new. - The hang is independent of libSceUlt: a bare
try { ::operator new(16); } catch (const std::bad_alloc&) {}atbudget=0, outside any lock, hangs the same way. - Adding a one-line re-entrancy guard so the replaced
operator newdoes not throw while it is already throwing makes all 8 budgets pass:finalization allocation budgets: 8 cases, 1 rejections, 0 incorrect lifecycles. So the fix itself behaves correctly; only the harness deadlocks.
This likely passed on Windows because the MSVC/MinGW exception runtime does not route that intermediate allocation back through the user-replaced operator new the way this libstdc++ does.
Suggestion: make the allocation-failure injection re-entrancy-safe so the runtime's own allocations during unwinding are allowed to succeed. The smallest fix is to disarm the budget right before throwing, e.g.:
void* operator new(std::size_t size) {
if (allocationBudget >= 0) {
++allocationAttempts;
if (allocationBudget == 0) {
allocationBudget = -1; // let the EH runtime allocate while unwinding
throw std::bad_alloc();
}
--allocationBudget;
}
if (void* memory = std::malloc(size ? size : 1)) return memory;
throw std::bad_alloc();
}(or a thread_local "in-throw" guard). The test then re-arms allocationBudget per iteration as it already does.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
jotitadev
left a comment
jotitadev
left a comment
There was a problem hiding this comment.
Re-ran the final head on Linux: the one-shot disarm in 3b60592ce fixes the deadlock, and the test is red on base and green with the fix.
Machine: Linux, Ubuntu 26.04, GCC 15.2.0, x86-64, i7-8700. Base d6eafa54d, head 64d0571a2, full build (BUILD_TESTING=ON, ANYPS5_RELINKER_ONLY=OFF, Ninja Release).
guest_ult_finalize_allocation |
|
|---|---|
Base Export.cpp + final test |
Failed (no hang, 0.00 s): budget=1 rejected=1 attempts=2 semaphore=-2139029498 mutex=0 / 8 cases, 2 rejections, 1 incorrect lifecycles |
| Final head | Passed: 8 cases, 2 rejections, 0 incorrect lifecycles |
ctest -R "^guest_ult_": 26/26 passed. patched_libraries passed. check_conventions.py --base d6eafa54d passed.
One correction to the description: with the final harness, Linux also records 2 rejected budgets, the same as Windows. The "1 rejections" in my earlier review came from my own re-entrancy guard variant, not from a difference in the allocator call pattern. The sentence "the independent Linux follow-up recorded one because allocator call patterns differ" can be dropped.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
There was a problem hiding this comment.
The production fix reserves both wake lists before marking or removing registered objects, so allocation failure leaves semaphores and mutexes untouched for retry. The injected allocator disarms before throwing; the independent Linux review confirms that avoids libstdc++ exception-runtime re-entrancy and that the final test is red on base and green on the fix. The author reports an eight-budget lifecycle check, 27/27 selected Windows runtime tests and a full Windows build; the independent Linux report gives 26/26 ULT tests plus patched-library loading. I independently ran exact-head conventions and git diff --check; both pass. Latest product jobs were skipped, so the Windows runtime evidence is from the author’s local run and Linux from the reviewer. I did not rerun the runtime suite locally. The change is generic (Unblocks: none).
I’ve noticed that holding this PR because of #5239 isn’t necessary. It’s fine for PRs to keep coming in; they’ll be handled in due course. Please disregard my earlier request to hold or defer it.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I withdraw the earlier #5239 intake-hold objection. The allocation-failure fix and regression have substantial evidence: the author reports the Windows eight-budget red/green run, selected ULT suite and full build; an independent Linux reviewer reports the final harness is red on base and green on the fix, with the ULT tests and patched-library loading passing. I have not rerun that runtime test locally.
I checked exact head 64d0571a2067409a153a58213513817f140504b4 against current main 3ca07d086d5f44fbe3c00370e7f03953ab46ca1b. git diff --check and current-main conventions pass. The changed C++ files pass local -fsyntax-only compilation with GCC 12.2 (-Wall -Wextra -Werror; the existing unused-parameter warnings in unrelated pending exports were suppressed for Export.cpp). git merge-tree --write-tree with current main succeeds without conflicts. CMake/runtime tests were not run locally; current hosted product jobs are skipped.
The branch base is d6eafa54d0464258574fd2f71889754d05a5bc93, substantially behind current main. The repository branch rule requires keeping PRs up to date with main. Please rebase/update and refresh the focused allocation regression and selected ULT test report on the resulting head. This request is for the current-main requirement and resulting validation only, not the obsolete intake hold.
|
I’ve noticed that my earlier request to hold this PR based on #5239 was unnecessary. It’s fine to keep submitting PRs; maintainers will handle them in due course. Please disregard that request. |
What
sceUltFinalizereserves a semaphore wake list, marks registered semaphores dead and removes them, then reserves the mutex wake list. If the second reservation throwsstd::bad_alloc, finalization fails after the semaphores are gone while mutexes remain live. Any semaphore waiters removed at that point also miss the notification scheduled after the critical section.I reserve both lists before changing any registered state. A reservation failure therefore leaves the semaphore and mutex usable for a retry. A successful call retains the existing invalidation and wake behavior. This is a host lifecycle correction; the PS5 allocator-failure contract remains unmeasured and is recorded in debt. The test allocator now disarms its injected budget immediately before throwing, allowing C++ exception machinery to allocate while propagating that fault.
Tested
Windows 11 Pro 10.0.26300; AMD Ryzen 9 7900X, 31.66 GiB RAM, NVIDIA GeForce RTX 4070 Ti SUPER. WinLibs GCC 15.2.0, Ninja Release,
BUILD_TESTING=ON,ANYPS5_RELINKER_ONLY=OFF, bundled dependencies.Current main base:
b7d1d47a5f9f32c9c388ad8d47a0055a90ae6e33. Tested head:e359c22d452190be61cd61c6470cf74f95d9dbe6. The new executable compiles production ULT sources directly so its replacement C++ allocator controls the real finalization path. The red control and the Before value below were measured on earlier based6eafa54d0464258574fd2f71889754d05a5bc93; I did not rerun that control on the new base. The ULT implementation and regression source were unchanged by the main merges. The regression exercises eight allocation budgets: rejected calls leave both objects usable for a retry; successful calls invalidate both.At budget 1 on base, finalization threw after two allocation attempts; the semaphore returned
ULT_ERROR_STATEwhile the mutex still locked. With the one-shot injector, the base test exited in 0.15 s with one incorrect lifecycle and the final-head test exited cleanly with zero. The final test recorded two rejected budgets on Windows and in the independent Linux rerun. This is a synthetic host component probe; no console or title was run.The full all-target Windows build passed on the final head with
BUILD_TESTING=ON, bundled dependencies and the default complete AGC execution-test set. #8310 is merged in the base, so no temporary source overlay was used. The earlier temporary base-source substitution for the red control was restored byte-for-byte before these builds.Final selected runtime tests: 27/27 passed, 0 skipped, including every registered
guest_ult_case andpatched_libraries. The loader passed with 367 libraries, 0 failures. Both changed translation units compile; conventions, registration (1,176 test executables, none built and unregistered), main-sync and whitespace checks pass. Registration is an executable count, not an executed-test count.The overlap scanner could not compare shared files with #6044, #8644 and #8677 because those PRs conflict with
main. I rechecked their exact shared-file patches: #6044 relocates queue/thread test paths and registration, #8644 implementssceUltConditionVariableSignalAll, and #8677 adds underscored option exports. None changessceUltFinalizeor the new allocation regression. Their changes remain untested in joint composition; they create no implementation dependency for this fix.Commands, with the supported WinLibs toolchain on
PATHand the full Release build configured in a sibling../builddirectory:The build, test and convention commands above used final submitted sources. The overlap scan preceded the last AGC-only main merge; no ULT source, test or debt file changed in that merge. The earlier red control temporarily compiled the regression target with unchanged earlier-base
Export.cpp, then restored the source before the current builds.An independent Linux review identified a recursive synthetic throw in the previous test harness. The follow-up review ran the earlier code head
64d0571a2067409a153a58213513817f140504b4on Ubuntu 26.04 with GCC 15.2: unchanged baseExport.cppplus the final test failed one of eight lifecycles without hanging, while the fix passed all eight with zero incorrect lifecycles. The Linux run also passed 26/26guest_ult_tests,patched_libraries, and the conventions check. I did not run Linux myself, and the independent reviewer has not tested the current merged head.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)