Repository navigation
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed the retargeting and direct-memory changes. One CI-blocking test issue: the new Windows-only writer tests have unbounded while (stores.load() < ...) std::this_thread::yield() waits (GuestMemory.cpp around lines 801/804 and 876/880). If a regression leaves a writer waiting in the AV handler, the test can hang indefinitely, contrary to CONTRIBUTING.md’s 30-second per-test limit. Please give guest_memory a hard CTest timeout below 30 seconds (and bound the progress waits where possible), so a regression fails the job instead of occupying a runner indefinitely. I have not approved this head; I could not independently run the Windows-only path here.
MarcZX10
left a comment
There was a problem hiding this comment.
One contribution-policy issue as well: this diff adds many explanatory comments in code (WindowsMappings.hpp, DirectMemory.cpp, CrashReport.cpp, and GuestMemory.cpp). docs/dev/CONVENTIONS.md allows code comments only for technical-debt notes, #endif, or namespace endings. The new TechnicalDebt entry is appropriate, but it does not make the implementation walkthroughs and test comments compliant. Please remove those comments or keep only comments that point to the specific documented debt; the PR checklist currently says there are no code comments, which does not match the diff.
|
Two things before this can go in:
It conflicts with main in TechnicalDebt now, so please rebase when you push. |
|
Tested this PR on Arch Linux in WSL2 on Windows 11 (Intel Core i7-13700K), using GCC 16.2.1 20260810, CMake 4.4.4, Ninja, Python 3.14.7 and Mesa lavapipe ( I applied head
Additional Windows integration check (WinLibs GCC 15.2.0 r7, Release with Validation performed with OpenAI Codex assistance. No game binaries or proprietary code are attached. |
…l it is aliased Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
46d6acb to
630b18f
Compare
|
Done, rebased on current
|
|
Please rebase onto current main: #2236 changed |
|
@esatturan I retested the current head (630b18f) against the crash I described on #361 (RetroArch's Dolphin core, a red-zone clobber during write-watch faults on direct memory). Startup works again. An earlier version of this PR stopped RetroArch at startup with The red-zone crash: I can't tell whether this fixes it. On my current base the One test interaction with Its second half:
With this PR that mapping is a private write-watched allocation, and the collect over both pages succeeds. Without the PR, on the same build, the whole How I ran it: Caveats:
AI-assisted (Claude Code). |
|
Tested head Windows 11, MinGW-w64 GCC 15.2.0, Release:
Arch Linux under WSL2, GCC 16.2.1, Release: This is focused validation, not a full-suite or game compatibility claim. The new main regression appears to need reconciliation with private direct-memory collection before merging. |
|
Follow-up to my Windows result above: the failure comes from private-region collection accepting I prepared a separate focused fix and regression in the fork: https://github.com/Mouhand-Kaddo/AnyPS5/tree/prep/private-write-watch-inaccessible (commit With this PR's head No upstream PR opened for the separate fix while intake is paused. |
|
Additional Windows runtime evidence, separate from the isolated regression checks reported above: I tested a local integration of current main During the bounded runtime comparison, host committed memory rose from 24.68 GB to 46.23 GB in 9.5 s; the commit limit was 46.99 GB at that point. I stopped the process when headroom fell below 1 GB. The process had about 7.51 GB of private committed memory then. This run ended before the engine loop, so it does not establish a boot or rendering result. The physical allocator had also created a shared pagefile-backed section for a 12 GiB physical allocation. The implementation currently creates that section eagerly, before private views are mapped. I am investigating delaying shared backing creation until a shared view or retained private data needs it, and will keep that separate from this PR's mapping behavior. No payloads or executable data are attached. Windows 11 x64, GCC 15.2.0 / MinGW-w64, Release; original pagefile settings retained. This is a commit-pressure observation on an integration, not an isolated functional failure of the PR. |
|
Follow-up on the host-commit observation: I prepared a separate, single-topic fork change that defers Windows shared backing creation until it is first mapped: On main I also tested a local integration with this PR's head Bounded runtime probes of the combined changes progress through substantially more shader work than the earlier run and now stop at an unsupported depth-maintenance draw. This does not isolate the earlier startup fault's cause or establish rendering success, since the integration also includes newer main and shader work. No new upstream PR or payload upload. |
|
Runtime result on Windows 11 10.0.26300, i7-13700K, RTX 4080, WinLibs GCC 15.2.0 r7: with On my branch |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the updates. Still open:
CheckFailedCollectKeepsWrites(already onmain) fails on Windows with this PR, as reported by @Mouhand-Kaddo and @johnmaia: private-region collection acceptsPAGE_NOACCESSin preflight, so a failed collect no longer preserves earlier writes. The fix is the separate change @Mouhand-Kaddo prepared (reject inaccessible private regions before any write-watch reset, https://github.com/Mouhand-Kaddo/AnyPS5/tree/prep/private-write-watch-inaccessible); please include it here or land it first, and keep the test expectation asmainhas it.- Rebase onto current
main(DirectMemory.cppconstructor andGuestMemory.cpptest list conflict). It rebases cleanly apart from those two spots. - Release of a page mapped once: checked in the code, not run.
ForgetDirectMemoryerasesg_physPagesand the mappings beforeUnmapDirectMemoryViews, soEraseMappingsfinds no private mapping andSavePrivateis not called on the freed range. Please add a test for it: map once, write,sceKernelReleaseDirectMemory, allocate the same range again, map it, and require zeroes (the old address must no longer be committed). - PR CI no longer builds Windows: please post a
guest_memoryrun of the rebased head with the first point fixed.
|
Test data for this PR with DREDGE (PPSA09076, Unity), which showed the red-zone crash described in #361 and #3915 on Rebase. Line 1159 is Crash rate (inconclusive). 10 alternating pairs of 120 s runs, plain AI-assisted (Claude Code). |
|
Second batch for the DREDGE comparison above, same method: 10 alternating pairs, 60 s each, the same relinked
The three faults on What this does and does not show: a one-sided Fisher exact test on 3 of 20 against 0 of 20 gives p of about 0.12, so it is consistent with the PR removing the crash but is not significant, and the rate on AI-assisted (Claude Code). |
|
I did not run anything beyond AI-assisted (Claude Code). |
|
@esatturan a rendering regression with this PR under host import, found and bisected together with @parl0v. What we see. Sword of the Sea (PPSA25344, Unreal Engine 5) on Windows. Our integration branch is
Reverting the R_X XOR fixes, #3372 and #3425 left it red (#3425 is a separate slowdown). Mechanism. @parl0v traced it in the code. Possible fixes. Invalidate and re-import any host import over the range in AI-assisted: yes (Claude Code) |
|
Second 60 s batch of the DREDGE comparison (10 more alternating pairs, same setup as the two comments above). The result is less clear than the first batch:
One-sided Fisher exact test: p of about 0.09 for 5 of 20 against 1 of 20, and about 0.10 for 5 of 30 against 1 of 30. So the rate with this PR was lower in these runs but the difference is not significant, and the crash did not go away: the one fault with this PR is in So this data does not support "this PR removes the DREDGE crash"; it is compatible with it removing some of the write faults that overwrite the red zone while others remain. A guess that I did not check: write-watched memory that is not direct memory mapped once (the guest arena's watched commits, or direct memory that is aliased) still takes resumed write faults. I did not check which memory the faulting write in DREDGE targets. AI-assisted (Claude Code). |
|
@esatturan a correction to our red-menu report above (Sword of the Sea, PPSA25344): the mechanism we gave there is wrong for this title. The red is real and comes with this PR plus host import, but it is not caused by What we got wrong. We said
What it is instead, as far as we can tell. The last row keeps Windows host imports write-watched. That takes a local switch of ours (
So some CPU write that the driver relies on write tracking to see is missed. That fits both changes from our first report: Cost. The comparisons are also expensive. In the four red rows The gap we described in Windows 11, RTX 5070 Laptop (616.92). The base is AI-assisted: yes (Claude Code) |
|
Question on testing: DREDGE (PPSA09076, Windows) faults intermittently in a function whose loop variables live below AI-assisted (Claude Code). |
|
Thanks for this. Closing in favour of #637, which handles singly mapped direct memory the same way and is being ported onto main. The points from the 10-11 review (inaccessible private pages in collect, the release test) apply there too. If you want to continue this design, please reopen. |
What
On Windows, direct memory was always mapped as views of a section. Views have no page write watch, so the GPU driver tracked CPU writes to them by arming pages read-only and taking a write fault. With live AVX state, the exception frame Windows builds for such a fault overwrites the stack from RSP-0x20 down: most of the System V red zone, where guest leaf functions keep live values. Measured on Windows 11 with AVX2; without live AVX state the frame starts at RSP-0x120.
PPSA29141 took about 5,000 such faults a second and crashed in roughly a quarter of 90-second runs, in a Unity job, on a red-zone slot read back as 0.
MEM_WRITE_WATCHmemory, one allocation per mapping, with the section as its store.GuestArenaAwaitRetarget) and retry.WindowsMappings::Collectfinds Commit's write-watched allocations withoutVirtualQuery.HostWritealso refuses private pages the guest left read-only (reads into them returnEFAULT).Tested
Windows 11, i7-13650HX, RTX 4070 Laptop, MinGW-w64 GCC 15.2.
guest_memorygains tests for: a page mapped once is private and never armed; content survives an unmap and remap; a second mapping converts to views while another thread keeps writing; a partial unmap while another thread writes. The writer waits are bounded (5 s) andguest_memoryhas a 20 s CTest timeout. The existing view tests now alias their pages.cteston currentmain(c6e3aa76) merged with this branch, the other LEGO Party! PRs and feat(libkernel): implement _umtx_op wait and wake operations #1482: 547/547 pass. On Windows,main'slibsstep currently fails withduplicate patched export name: LibwuIonIBw: feat(videoout): implement sceVideoOutGetResolutionStatus #1714 addedAPS5_EXPORTs forsceVideoOutAddVrrActiveStatusEventandsceVideoOutAddVrrStatusFlagsPrivilege, whose names the NID table already maps. Those two lines were removed locally for the run.--module-dirfor its plugins, and feat(relinker): add --module-dir for modules loaded at run time #846 no longer merges with today'smain, so the title was not converted and run again on it.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt.mdfiles or images added to the repository (attach them to this PR)