Skip to content

fix(agc): accept bound copies of a registered shader header that differ only in user_data - #1837

Merged
adriadam10 merged 1 commit into
boykopovar:mainfrom
EdouardSence:fix/agc-shader-header-copies
Oct 10, 2026
Merged

adriadam10 merged 1 commit into
boykopovar:mainfrom
EdouardSence:fix/agc-shader-header-copies

Conversation

@EdouardSence

Copy link
Copy Markdown
Contributor

What

The static, graphics and rectangle ABI resolvers look up the registered shader by its code address. #639 made them require the bound Shader header to be at the registered header's address, and otherwise throw ... refers to a replaced shader header.

Astro Bot (PPSA21567) registers a vertex shader with its header at 0x50000e500, then binds it for a draw through a copy at 0x307ae3920. The copy's 96 fixed Shader bytes match the registered header except user_data: 0x50000e668 in the registered header, null in the copy. The cx_registers, sh_registers, specials and semantic pointers still point into the registered header. The bytes after the fixed fields are unrelated memory, so only sizeof(Shader) is compared.

SameHeader now accepts a bound header when it is at the registered address, or when its fixed fields equal the registered header's with user_data ignored. Any other difference still throws. The registered header's metadata is used, as before. This is recorded in TechnicalDebt, because whether the console reads the copy's user_data is unknown.

Tested

Linux (CachyOS, GCC), RX 9070 XT, Mesa 26.2.4 RADV.

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 (it is reached in Astro Bot only after fix(agc): defer shaders that cannot be prepared at registration to their first use #1836, but the two are independent)
  • AI-assisted: yes. The investigation, code, tests and this description were written by Claude (Claude Code) on my machine.

🤖 Generated with Claude Code

@fisherjoey

Copy link
Copy Markdown
Contributor

Tested on Stray (PPSA02100 v1.05, Unreal Engine 4). It hits the same rectangle ABI refers to a replaced shader header (from sceAgcCreateInterpolantMapping -> ResolveGraphicsAbi), but this PR alone doesn't get it through.

Setup: Linux (Nobara 44, kernel 7.2), RTX 5070 Ti, NVIDIA 595.91.07. Base is main 1be6f9f + #861 + #1836 + #1893 + #1902 + #1904.

Stray binds a copy of a registered vertex shader header (registered at +0x0, copy at +0x250). I dumped both 96-byte Shader structs at the failing compare. They differ in only two places:

[8..13]  user_data: a pointer in the registered header, null in the copy (this PR ignores it)
[93]     7f / 00

Byte 93 is padding: num_sh_registers is at offset 92 and sizeof(Shader) is 96. The game's copy zeroes it and the registered header has 0x7f there, so the memcmp over sizeof(Shader) still fails.

Comparing only up to the end of the last field fixes it:

return std::memcmp(&used, &registered, offsetof(Shader, num_sh_registers) + sizeof(Shader::num_sh_registers)) == 0;

With that one change on top of this PR, Stray gets past the interpolant mapping and stops later on an unrelated prepared rectangle artifacts are missing. Without it, it aborts at the same point every run (runs 8 to 11).

The test in this PR doesn't catch it because Shader copy = header.shader; copies the padding too. Writing a nonzero byte at offset 93 of the registered header (or zeroing the copy's padding) before the compare would cover it. Happy to send the change and a test as a suggestion on this PR if that helps.

AI-assisted: yes

@NahuelGenchi

Copy link
Copy Markdown
Contributor

Sonic Origins (PPSA05324) hits the static ABI variant of this, static ABI refers to a replaced shader header, on main f145139 + #2004 once #1789 is worked around locally.

In one run, 14 shaders (header type 1) are bound 46 times through a copy of their registered header 0x78 bytes below it. All 96 fixed bytes of the copy equal the registered header, user_data included, so the comparison in this PR would accept them. Accepting byte-identical copies locally gets the title past this stop.

Bring-up thread: #2021

@johnmaia

johnmaia commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Tested on a second title: Disney Epic Mickey: Rebrushed (PPSA15657, UE4) stops on the same rectangle ABI refers to a replaced shader header. With this PR it still stops there, because the comparison also covers the struct's tail padding.

I logged the 96-byte comparison in SameHeader locally. Every field matches, user_data included, and the only difference is in the last 8 bytes:

bound=0x…1220  registered=0x…0f80  sizeof(Shader)=96
+58  bound=00007d0b0a020030  reg=0000000b0a020030   <-- differs

At +0x58 the fields are special_sizes_bytes = 0x0030, type = 2, num_cx_registers = 10 and num_sh_registers = 11. They end at +0x5d. Bytes +0x5d..+0x5f are padding, and the title's copy has a leftover 0x7d there. std::memcmp(&used, &registered, sizeof(Shader)) therefore rejects a copy whose fields all match.

Comparing up to the end of the last field fixes it here:

return std::memcmp(&used, &registered, offsetof(Shader, num_sh_registers) + sizeof(registered.num_sh_registers)) == 0;

A field-by-field comparison would also work. With that change, the title gets past this point and stops at an unrelated stub.

Measured on RX 7600 (gfx1102), Linux, with main f145139 plus a local stack of open PRs including #1836.

Tested and written by Claude (AI assistant) on a contributor's machine.

@EdouardSence

Copy link
Copy Markdown
Contributor Author

@johnmaia thanks, you're right. Shader ends at +0x5d, so the last 3 bytes are padding, and memcmp over sizeof(Shader) compared them. 8d9bcb7 compares up to the end of num_sh_registers instead. The struct has no interior padding, so this covers every field and nothing else.

The test in Registration() now also binds a copy with 0x7d in its last padding byte. It fails before the change and passes after.

Rebased onto main a8b032b. ctest -j8: 494 of 495 pass. The one failure is import_audit, which can't read the commit hash in my worktree and is unrelated.

@NahuelGenchi thanks for the Sonic Origins data. The static ABI path uses the same comparison.

AI-assisted: the fix, the test and this reply were written by Claude (Claude Code) on my machine.

@oneandonlydean

Copy link
Copy Markdown
Collaborator

This conflicts with main in ShaderRegistry.cpp since #1836 merged; could you rebase? It passed testing at 8d9bcb7. If Pipodi's relocation commit comes in with the rebase, it gets tested again before merging.

…er only in user_data

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@adriadam10

Copy link
Copy Markdown
Collaborator

Rebased onto current main as requested: the only conflict was two adjacent includes in ShaderRegistry.cpp (<cstdio> from main, <cstddef> from this PR), both kept. The diff is otherwise unchanged. Partial build and ctest (libSceAgcDriver, libs, libkernel) pass 380/380. Pipodi's relocation commit is not included. Merging once CI is green.

@adriadam10
adriadam10 force-pushed the fix/agc-shader-header-copies branch from 8d9bcb7 to cd1f76c Compare October 10, 2026 07:11
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 10, 2026
@github-actions

github-actions Bot commented Oct 10, 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
#1825 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Shaders/ShaderRegistry.cpp L14
#2169 docs/dev/TechnicalDebt.md L247

@adriadam10
adriadam10 merged commit 5596e5b into boykopovar:main Oct 10, 2026
14 checks passed
@Pipodi

Pipodi commented Oct 10, 2026

Copy link
Copy Markdown
Contributor

Thanks for merging. The relocation extension discussed above isn't part of this merge: whole-header copies whose pointers into the header all move by the header's own distance, which PPSA26344 binds and which still fail with "static ABI refers to a replaced shader header".

It's rebased onto current main as one commit, 44a80d4f, on https://github.com/Pipodi/AnyPS5/tree/fix/relocated-shader-header-copies. It touches ShaderRegistry.cpp, adds a PreparedShaders case, and updates the TechnicalDebt line. All seven agc_driver_prepared_shaders modes pass. With ShaderRegistry.cpp from main, the new case fails with that error.

pr-intake would close a new PR from me, and @EdouardSence is in the same position. How would you like us to carry it on?

SleglJan pushed a commit to SleglJan/AnyPS5 that referenced this pull request Oct 10, 2026
…eader-copies

fix(agc): accept bound copies of a registered shader header that differ only in user_data
@EdouardSence

Copy link
Copy Markdown
Contributor Author

Data for Pipodi's relocation commit (44a80d4f, https://github.com/Pipodi/AnyPS5/tree/fix/relocated-shader-header-copies), from Ghost of Yōtei (PPSA26344) on Linux.

Setup: CachyOS (kernel 7.2.9), RX 9070 XT (RADV), Release, headless gamescope with scripted input. Main b5f5362d3 + #1940 (needed to get past operand 235, #1920).

Without 44a80d4f: the title stops about 21 s in on the graphics path:

AGC driver: graphics ABI refers to a replaced shader header

A local print (not pushed) of the words that differ between the bound header and the registered snapshot:

used 0x800005cbf0 registered 0x8000031b38 snapshot header bytes 376 code 0x8000410b00 type 2
+0x08 used 0005cd00 registered 00031c48   (user_data, already ignored)
+0x18 used 0005ccb0 registered 00031bf8   (cx_registers)
+0x20 used 0005cc80 registered 00031bc8   (sh_registers)
+0x28 used 0005cc50 registered 00031b98   (specials)
+0x30 used 0005cc80 registered 00031bc8   (input_semantics)
+0x38 used 0005cc80 registered 00031bc8   (output_semantics)

Every pointer is at the same distance from its own header (+0xc0, +0x90, +0x60, ...), so this is the whole-header copy the commit describes, on the graphics ABI and not only the static one.

With 44a80d4f cherry-picked: that stop is gone. The title runs on to about 26 s and stops on an unrelated recompiler refusal (compute shader 0x80003aa700: GetImageResource dword 0 is not a valid runtime value, an image descriptor read from a buffer at a computed index).

Tested and written by Claude (AI assistant) on a contributor's machine.

@Pipodi

Pipodi commented Oct 10, 2026

Copy link
Copy Markdown
Contributor

Thanks @EdouardSence, and @Dextroid17 in #675 on lavapipe: 44a80d4f clears the replaced-header stop on RADV and llvmpipe too, on the graphics ABI as well as the static one. The commit is unchanged on https://github.com/Pipodi/AnyPS5/tree/fix/relocated-shader-header-copies whenever the maintainers decide how they'd like it carried.

TheBlenDed pushed a commit to TheBlenDed/AnyPS5 that referenced this pull request Oct 25, 2026
Adopt Pipodi's relocated-header normalization from commit
56b154a, extending boykopovar#1837 for whole-header
copies reported in PPSA26344. Keep code and fixed metadata fields exact.

Require newly relocated metadata spans to remain inside both headers and
match the immutable registered snapshot before using its ABI. Preserve the
existing same-address, shallow-copy and user_data behavior. Cover mixed
copies, modified metadata, incorrect pointers and legacy/extended specials.

Source: boykopovar#1837 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants