Repository navigation
fix(libSceAgcDriver): zero the overhanging record of a vertex fetch - #5926
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.
|
7b9bfc9 to
3666457
Compare
|
@adriadam10 a short request for review of this PR. It had a merge conflict in AI-assisted (Claude Code). |
jotitadev
left a comment
jotitadev
left a comment
There was a problem hiding this comment.
A Linux run, since the description says the mmap branch of the test was not built.
Machine: Ubuntu 26.04 (kernel 7.0.0-38, 4 KiB pages), i7-8700, GCC 15.2, Release, BUILD_TESTING=ON; NVIDIA GeForce RTX 3070, driver 595.99.02. Head 3666457f0 merged onto main 5d9dcc42d, no conflicts.
GPUs tested: NVIDIA GeForce RTX 3070 (595.99.02), Linux
- The
mmap/mprotectbranch ofvertexOverhangTestsbuilds with no warnings from the changed files, andagc_driver_graphicspasses 3/3. - Fault check on Linux: with the clamp line in
CopyZeroPaddedDrawInputtaken out (if (validBytes - prefix < stride) readable = prefix;, same signature so the test still builds),agc_driver_graphicsfails 3/3 withAGC driver: guest memory is not readable at 0x...5e000 (range 0x...4e650+0xf9b8), the 8 bytes past the readable page. With the line restored, it passes. SoGuestMemory::Accessible/CommittedRangessee thePROT_NONEhalf of the block on Linux as well, and the test guards the fix there. ctest -R "^agc" -E "agc_display_buffer_dcc|agc_driver_flip|agc_driver_lds_atomics64" -j1 --timeout 60: 591/593 pass, the same result as unchangedmainon this machine (agc_driver_stencil_ref_exportskipped,agc_driver_store_over_untrackedfails as onmain, #6385).
Written with AI assistance (Claude Code). I ran the tests above on my machine.
|
Re-checked on Linux (RADV) with the PR merged onto current |
|
The fix reads right. Could the debt entry drop the title (PPSA26223; it's in the PR body), per the no-titles-in-docs rule? |
github-actions
Bot
added
the
unblocks title
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
3666457 to
55b197d
Compare
|
Author address of the first commit corrected (it carried a local test address, so GitHub did not attribute it to my account); no code change: the tree of both commits is identical before and after ( The second commit drops the title from the debt entry, as asked: the entry now describes the observed case (a fetch of 14858 records of 44 bytes that ends 8 bytes past the end of the 0xa0000-byte mapping holding it) without naming the game; the title stays in the PR body. AI-assisted (Claude Code). |
What
The Mound: Omen of Cthulhu (PPSA26223) sometimes aborts in the video-out thread while it prepares a draw:
(
PPSA26223_rM2/run_175628.errlines 4201-4206; the[VBDBG]line is a local debug print, not part of this change.) The vertex descriptor says 14858 records of 44 bytes, 0x9f9b8 bytes, starting at offset 0x650 of a mapping that the title made withmap direct 0000001378f00000+0xa0000(same file, line 3895). The mapping holds 0x9f9b0 bytes from that address: 14857 whole records and 36 of the 44 bytes of the last one.CopyZeroPaddedDrawInputasks for the whole 0x9f9b8 bytes and the draw fails 8 bytes past the mapping.This pull request copies only the readable bytes of such a fetch and reads the rest as zeros, when the part past the readable memory is less than one stride. A longer overhang, and a fetch without a stride, still fail as before.
docs/dev/debt/libSceAgcDriver.md(a descriptor that ends less than one stride past the readable allocation is bound only up to the allocation's end, assuming the extra bytes are the rounded-upnum_recordsof the last record). It is taken from there and from fix(agc): bound a strided buffer's rounded-up last record by its allocation #2912, not measured on hardware; whether the console reads the missing 8 bytes of the last record, and whether this draw uses that record, is not known. The debt entry says so.PPSA26223_rL/run_155033.erronmaine736e5bdb,PPSA26223_rM1/run_160909.err, and the run above) and not in 16 other runs of the title that I made (on three different builds ofmain), so it depends on something in the run that I could not identify. The first two hits name the same range shape (0x650+0x9f9b8, 8 bytes over a 0xa0000 boundary); only the third has the stride and record count.Tested
Windows 10 (19045), MinGW-w64 GCC 15.2.0, Release, on
mainfa9365ca7+ this commit (head3666457f0):agc_driver_graphics(new casevertexOverhangTests: a reserved block with one committed and onePAGE_NOACCESSpage; a fetch of stride 44 that overhangs the committed page by 8 bytes is copied with its last 8 bytes zero; an overhang of one stride, and a fetch without a stride, fail with "is not readable") passes. With the clamp removed inDraw.cpp(same signature, so the test still builds; the unchangedmainDraw.cppdoes not compile against the new call) it fails with "guest memory is not readable".python tools/check_conventions.py --base upstream/mainwith this title and body passes.main979ad7b6awith a localfprintfinDraw.cppthat prints the fetch when it is unreadable;ninja -C build -j4 relinkerplus all 126core/libs/libs/*.prx,libSceAgcDriver.prx17:01:45 local time;relink_title_trace.ps1(relinker.exe --windows --registry --trace-imports),run_title.ps1 -Seconds 120,APS5_TRACE_MEMORY=1,APS5_TRACE_UNREADABLE=1. In the earlier hitPPSA26223_rM1/run_160909.err(offsets+0xc4cd4and+0x120186of that build, resolved withnmon itslibSceAgcDriver.prx) the caller isAgcDriver::Graphics::CopyZeroPaddedDrawInput; the[VBDBG]line above is printed from the same code path.mmapbranch that I did not build), a console, other titles.Overlap: #5778 (open) also deals with V# ranges, but in
ShaderResources::addGuestBufferfor a size pastmaxStorageBufferRange; this change is in the staging copyCopyZeroPaddedDrawInputfor a record that overhangs readable memory by less than one stride. merge-tree of both heads conflicts only indocs/dev/debt/libSceAgcDriver.md. Whether #5778 changes the Mound case was not tested.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt (docs/dev/debt/libSceAgcDriver.md).mdfiles or images added to the repositoryAI-assisted (Claude Code).
🤖 Generated with Claude Code