Repository navigation
fix(shader): poison an SRT read below a pointer loaded from memory - #12208
Conversation
github-actions
Bot
added
the
unblocks title
|
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.
|
|
@XairRrR A merge-order point before I submit our indexed scalar-descriptor correction, with comparison/limits here. Your negative-immediate underflow case and our runtime-loaded scalar-offset case are distinct. Your three production lines handle AddSignedAddress failing; ours changes the later inaccessible-read predicate and planning eligibility. Both edit the flat-SRT debt paragraph, flagged at comparison base7fe69e276fc4. I propose #12208 first, then preserve your wrapped-address/console-unmeasured note when refreshing ours, as merge order only. No technical prerequisite or combined build is claimed. Our fork candidate passes12 focused processes, including3/3 new GPU runs per RTX5080/Radeon, with matched main-production failures. This is not validation of your head or the original #11469 T# report. Does that order work for you? No duplicate upstream PR opened. AI-assisted:yes(Codex); author responsible. |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The new negative-offset poison handling is consistent with the existing inaccessible-read path, and the test covers mapped, executed-null and guarded-away cases. I found no separate logic defect in the diff. This branch is 60 commits behind current main; please rebase and rerun agc_shader_memory on current main and the rebased change. Because this touches the shader recompiler/AGC, the repository rules also require the same focused run with ANYPS5_REQUIRE_VULKAN=1, ANYPS5_NO_SHADER_CACHE=1 and VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation, with logs checked for missing-layer warnings and new VUID- errors. Please report those results before approval.
|
Windows + NVIDIA result for head This PR's own tests: Host: Windows 11 26300, Intel i7-12700K, NVIDIA GeForce RTX 2070 SUPER (vendor 0x10de, device 0x1e84; Vulkan loader 1.4.357 from the NVIDIA driver 621.01), Khronos Validation Layers nightly 1.4.366 (b2b44c0d37) through Same focused run on
The tree also carried the open PRs #12298, #12272, #12262, #12256, #12250, #12234, #12228, #12211, #12210, #12209, #12207, all merged cleanly, so the run is a joint one. Not run on a PS5. AI-assisted: yes (Claude Code), the runs and the numbers above are from this machine. |
A read at a negative offset from a pointer the walker loaded from guest memory (a null pointer behind a guarded branch, offset -4) wrapped below zero in AddSignedAddress, and the walk threw "cannot evaluate" instead of recording the read as inaccessible. Uncharted: The Lost Legacy threw it while loading its first level. The read is now recorded as an SRT poison at its address wrapped to 48 bits, like any other unreadable address, so the binding faults only if the shader takes that path. verifyGuardedNullPointers gains the case for both branch outcomes; it throws the title's error without the change. AI-assisted: yes
12c0427 to
91bbd2f
Compare
|
Rebased on Rebased on @Amoenus that order works for me: #12208 first, then your indexed scalar-descriptor change refreshes the flat-SRT debt paragraph and keeps the wrapped-address note. AI-assisted: yes |
|
Checked the three-line change in |
What
A flat SRT read through a null or unmapped pointer that the walker loaded from guest memory is recorded as an SRT poison and faults only if the program executes it (the debt entry for flat SRT reads describes this). A read at a negative offset from such a pointer never got there:
AddSignedAddressrefused the address below zero, andEvaluateRawReadreturned without recording anything, so the walk threwcannot evaluate ... SRT read at flat offset ....PPSA05684 (Uncharted: Legacy of Thieves Collection) threw it in Uncharted: The Lost Legacy while loading its first level: compute programs read
s_load ... offset:-4through a pointer that is null behind a guarded branch.The read is now recorded as inaccessible at its address wrapped to 48 bits (
0xfffffffffffcfor offset -4 from null), like any other unreadable address, so the binding faults only if the shader takes that path. The debt entry says the console's fault address for it is not measured.Tested
Arch Linux 7.2.11-zen, GCC 16.2.1, Ninja, Release, on
main3ca715e:agc_shader_memory:verifyGuardedNullPointersgains a read at offset -4 below the nested pointer: mapped (holds its word, no poison), then null with the guard taken (one poison at its pc and wrapped address, reported through the fault buffer) and skipped (no poison). WithSrtEvaluator.cppfrommainit fails with the title's error.ctest -j8(this branch together with my five other branches of today, on the samemain): 2619 of 2620 pass;agc_driver_mesh_barycentric_emulationfails the same way onmainon this GPU (the pixel shader did not emulate barycentrics).tools/check_conventions.py --base origin/mainpasses.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)