Repository navigation
fix(agc): clear a reused pooled buffer's unused tail on macOS - #11303
Conversation
MoltenVK has no robustBufferAccess2, so a robust read past the end of a pooled buffer is clamped into its slot instead of returning zero. A 100-byte buffer placed in a 256-byte slot then read the bytes an earlier user left there (0xabababab), and agc_driver_pooled_buffer_range failed. On macOS, when a host-coherent mapped slot is handed to a new buffer, the bytes past the requested size are cleared to zero. Linux and Windows compile the same code as before.
|
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.
|
|
Tested on an M5 Pro, macOS 27.2 (26B5086k), x86_64 Release under Rosetta 2, MoltenVK and the validation layer from the LunarG SDK 1.4.363, with Base is mugurc's
AI-assisted: yes |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed exact head 8a9a741b684469cce9997266ed44d7e2d0e53aea. The Apple-only helper clears only the mapped, host-coherent allocation tail and bounds the range by both the buffer capacity and allocation size. The author and an independent M5 Pro run report the targeted regression passing with the patch and failing without it; the independent run also reports no new validation errors (the listed validation messages are present in both states).
Before approval, please refresh this branch onto current main (9b197b6ce4583e64bf2427e6beb7270bcfa1528e) and rerun the focused macOS test on that exact head. tools/check_main_sync.py reports this head is 1 commit ahead and 101 behind (merge base a3987a1ec), although a merge preview is clean. The hosted checks currently pass only policy/conventions/intake/labels/report; product jobs are path-skipped. I also manually checked the overlap bot's Resources.cpp warnings against #7642, #9508 and #10193: their changes in that file do not conflict with this tail-clear hunk. I have not approved pending the current-main refresh required by the repository's branch-up-to-date rule.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The independent M5 Pro validation is useful: it reports the target test failing 16/16 without the change and passing 16/16 with it, with no new validation-layer errors. The tested tree is still based on #8658’s 27e9689743, though. Against current upstream/main 14c99b52ace303eef1667764670d1d1540140b3d, this exact PR head is 1 ahead and 121 behind (merge preview clean). Please refresh onto current main and rerun agc_driver_pooled_buffer_range on that exact head; the previous approval blocker is current-main validation, not the quality of the reported M5 reproduction.
maximilianfv
left a comment
maximilianfv
left a comment
There was a problem hiding this comment.
Requesting changes for the PR text only. The code reads correct, and my A/B confirms the fix in both directions with no new validation errors. The body still needs the validation-layer statement and the GPU line that CONTRIBUTING asks for. The run below has the data.
Tested on Apple M1 Pro, macOS 27.0.1 (x86_64 build under Rosetta 2)
Applied at 8a9a741b6 to mugurc's macOS series (27e968974) merged with main at 2742004ed2, with other open macOS fixes, then toggled only this PR. MoltenVK from the Vulkan SDK 1.4.363.0, with ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation. The layer loaded in every run (no was not found but was requested line).
agc_driver_pooled_buffer_range |
agc_static_buffer |
agc_write_tracking |
Validation Error lines per run |
|
|---|---|---|---|---|
| Without this PR | fails 23 of 23: "a robust read of word 28 of a 100-byte pooled buffer returned 0xabababab, the bytes a 256-byte earlier user left in its memory" | bus error 3 of 3 (EXC_BAD_ACCESS in sceAgcGetStaticBuffer) |
fails 3 of 3: "a block the import window did not touch reads as written" | 1 |
| With this PR | passes 23 of 23 | same bus error 3 of 3 | same failure 3 of 3 | 1 |
On each side, 3 runs of all three tests plus 20 more runs of agc_driver_pooled_buffer_range alone. The one validation error on both sides is VUID-VkDeviceCreateInfo-pProperties-04451, from the test creating its own device without VK_KHR_portability_subset (#10920). The PR adds no VUID-. agc_static_buffer and agc_write_tracking fail the same way without the PR on this tree and do not involve Resources.cpp.
I also checked the premise. MoltenVK reports vkGetBufferMemoryRequirements().size = 100 for the 100-byte buffer: I added a temporary print in BindExactBuffer, then removed it. So word 28 (byte 112) lies past the memory range bound to the buffer. Under robustBufferAccess, MoltenVK should not return the earlier user's bytes there. The read is a driver deviation, not behaviour the spec allows, and clearing the tail is a fair workaround.
Findings
-
PR body,
TestedandGPUs tested: CONTRIBUTING.md:68 is not met. An AGC change needs a run withVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation,ANYPS5_NO_SHADER_CACHE=1andANYPS5_REQUIRE_VULKAN=1onmainand on the change, a statement that the layer loaded and that the change adds no newVUID-, and vendor, model, driver and OS on theGPUs testedline. The body has none of the first three, and its GPU line lacks the OS. Fix: add your own validation run, or cite this one or Gonic0's, and write the line as e.g.Apple M2 Pro, MoltenVK 1.4.2, macOS 26. As MarcZX10 asked, refresh onto currentmainbefore that run. The merge base isa3987a1ec, andResources.cpphas not changed onmainsince. -
Suggestion, not blocking,
Resources.cpp:9, 18, 66, 110: key the clear on the driver rather than on__APPLE__. The deviation belongs to MoltenVK, not to the platform. On this Mac,vulkaninfoalso lists KosmicKrisp, withrobustBufferAccess2 = true, and the#ifdefruns the memset there for nothing. A check ofVkPhysicalDeviceDriverProperties::driverID == VK_DRIVER_ID_MOLTENVK(or of the device lackingrobustBufferAccess2, whichVulkanDevice.cpp:2086already computes butGraphics::Contextdoes not carry) would be exact and would drop the#ifdef. The test's own fixture fillsContextitself, so it would need to set the flag. -
Nit,
Resources.cpp:9-12, 23:<cstddef>is included only under__APPLE__, butstd::byteis used unconditionally inBuffer::Bytes(). There is also a blank line before the#endifat line 24 and none after it.
AI-assisted: yes (Claude Code applied the PR, ran the tests, read the code and wrote this review).
main's Resources.cpp already uses std::byte in Buffer::Bytes() on every platform without including <cstddef>; it arrives through the project headers this file includes first. Guarding a second include under __APPLE__ suggested otherwise, so drop it. Also keep the blank line inside the __APPLE__ block so the text outside it stays identical to main.
|
@MarcZX10 @maximilianfv thanks both. Done on head
On checking for MoltenVK instead of AI-assisted: yes (Cana) |
|
Windows run of this head
AI-assisted: yes (Claude Code) |
|
@iamthehimansh Building current |
|
@imellag yes, I'll split those four out of #8658's branch into one #7201 also changes AI-assisted: yes (Cana) |
|
@iamthehimansh Thanks. A correction to my question: #7201 isn't the only open PR on those files. My #10676 already makes the same |
|
@imellag the four AI-assisted: yes (Cana) |
maximilianfv
left a comment
maximilianfv
left a comment
There was a problem hiding this comment.
Approving at 1ac3e3561. Findings 1 (body) and 3 (<cstddef>) from my earlier review are done. The body now states that the validation layer loaded and that the change adds no VUID-, and the GPU line gives the OS. The macOS-only <cstddef> is gone. You asked about finding 2 (the gate), and I answer it in note 2 below. Apart from the main merge, dropping <cstddef> is the only change since 8a9a741b6. My A/B at this head confirms the fix both ways. Current main (185596faff) does not fix the bug: its Resources.cpp is the same blob as this PR's base (ea37fed7ef). The notes below do not block the merge.
Tested on Apple M1 Pro, macOS 27.0.1 (x86_64 build under Rosetta 2)
Applied at 1ac3e3561 to mugurc's macOS series (27e968974) merged with main at 2742004ed2, with other open macOS fixes, then toggled only this PR. The only commit to touch Resources.cpp on main since that base (4bee82086f) adds an include and a DestroyDeferredAtIdle call in CommandBatch::Submit(). Both are unrelated to the Buffer constructor, and BufferPool and the test have not changed. Both states were rebuilt before their runs. MoltenVK from the Vulkan SDK 1.4.363.0, with ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation. The layer loaded in every run (no was not found but was requested line).
agc_driver_pooled_buffer_range |
agc_static_buffer |
agc_write_tracking |
Validation Error lines per run |
|
|---|---|---|---|---|
| Without this PR | fails 23 of 23: "a robust read of word 28 of a 100-byte pooled buffer returned 0xabababab, the bytes a 256-byte earlier user left in its memory" | bus error 3 of 3 (AGC driver memory at 0xfe0000000 is held by Rosetta) |
fails 3 of 3: "a block the import window did not touch reads as written" | 1 |
| With this PR | passes 23 of 23 | same bus error 3 of 3 | same failure 3 of 3 | 1 |
On each side, I ran all three tests 3 times and agc_driver_pooled_buffer_range alone 20 more times. The one validation error on both sides is VUID-VkDeviceCreateInfo-pProperties-04451. It comes from the test fixture, which creates its own device without enabling VK_KHR_portability_subset (#10920). The PR adds no VUID-. agc_static_buffer and agc_write_tracking fail the same way without the PR and do not involve Resources.cpp.
Notes (non-blocking)
-
Tested: "reported noVUIDmessages". With the same SDK, the layer reports this 04451 error once per run of this test on both sides for me (M1 Pro). Gonic0 counted 81 of them across 89 tests on an M5 Pro, even with the MoltenVK ICD marked non-portability. Please recheck how your logs were counted (grep -c 'Validation Error'). If the count is not 0, write "the same validation errors on both sides, none new". CONTRIBUTING's "no newVUID-" requirement is met either way, by my run and Gonic0's. -
Resources.cpp:9, 17, 65, 109:__APPLE__compared with a check on the driver. You asked whether I'd prefer the runtime check, so here is my answer. Yes, but as a follow-up, not on this PR.mainhas no__APPLE__block inResources.cppor anywhere underGraphics/src. It keys MoltenVK workarounds ondriverID == VK_DRIVER_ID_MOLTENVKat runtime (lines on currentmain:VulkanDevice.cpp:1296, 1300,Float32FlushControl.hpp:15,FragmentBarycentric.hpp:9,TessellationPipelineCache.hpp:9). It also keys some onrobustBufferAccess2. A flag onGraphics::Contextset from one of those would matchmainand would cover KosmicKrisp. KosmicKrisp is also listed on this Mac and reportsrobustBufferAccess2 = true, so it does not need the clear. -
Resources.cpp:66:std::min<VkDeviceSize>(capacity, allocationBytes)is alwayscapacity.ClassAllocationBytesnever returns less thancapacity, and slab allocations setallocationBytes = capacity(line 105). For a non-slab allocation, the mapping starts at offset 0 and covers the whole allocation (line 118). SoallocationBytesis the exact bound of the clear. Passing it would remove thestd::minand also clear the alignment padding pastcapacity. I found no way a different-sized earlier user leaves bytes in that padding, so this is a simplification, not a bug. -
Scope of the workaround. The clear covers host-visible, host-coherent pooled buffers that are mapped. It does not cover:
- pooled
Buffers in device-local memory, which have no mapping; DeviceBuffer;- a fresh non-slab allocation (line 118). Vulkan does not promise zeroed memory there. I have not tested whether MoltenVK returns stale bytes in that case;
- a read that goes past
capacity.
My probe on the earlier head showed that MoltenVK binds 100 bytes for this buffer but serves a read at byte 112. So the read is not limited to the bound range. If it is only limited by the
VkDeviceMemory, a read past the slot in a 2 MiB slab block would return a live neighbour's bytes. I have not tested that. A line indocs/dev/debt/libSceAgcDriver.mdwould record what this change leaves open, next to the other MoltenVK entries there. - pooled
AI-assisted: yes (Claude Code applied the PR, ran the tests, read the code with two independent reviews and wrote this review).
|
@maximilianfv Thanks for the approval and for checking the counts. You were right about note 1. My Rosetta wrapper never pointed the loader at the SDK's layer folder, so the layer was missing in those runs and the 0 meant nothing. I fixed the wrapper and re-ran with the loader's layer output on:
That matches your M1 Pro count. The description now says the same and names the fixture as the source (#10920). I'll leave notes 2 to 4 for a follow-up once this lands: the AI-assisted: yes (Cana) |
Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.
|
Checked the diff (Apple-only, bounded by both capacity and allocation size, both pooled paths covered, text outside |
What
Next part of splitting #8658:
agc_driver_pooled_buffer_rangefails on macOS. MoltenVK has norobustBufferAccess2, so a robust read past the end of a pooled buffer is clamped into its slot instead of returning zero. A 100-byte buffer placed in a 256-byte slot then reads the bytes an earlier user left there.On macOS, when a host-coherent mapped slot is handed to a new buffer,
Graphics/src/Resources.cppnow clears the bytes past the requested size to zero. That happens on both paths: a reused class allocation, and a new slot in a slab. Linux and Windows compile the same code as before.I compared the added lines with all 1,377 open PRs; none carries this code.
Tested
Apple M2 Pro, macOS 26.6.2, MoltenVK 1.4.2 (LunarG Vulkan SDK 1.4.363.0), x86_64 Release under Rosetta 2, with
ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation. The validation layer loaded in every run (loader output shows it inserted, with no "was not found" line). It reports oneVUID-VkDeviceCreateInfo-pProperties-04451error per run on both sides, 3 of 3 without and 3 of 3 with this PR. That error comes from the test fixture creating its own device withoutVK_KHR_portability_subset(#10920). This PR adds no newVUID.libSceAgcDriverdoes not link on macOS onmainalone, so the test runs on a local base. It ismainatb27cc8416, two commits behind this head'scbee22480, and those two only change libc'stanh. On top of that:8d601f9ec), feat(libkernel): guest memory protection and VirtualQuery on macOS hosts #10898 (119e87952), fix(libc): keep DWARF unwind tables in libc on macOS hosts #8311 (16e6ee822) and fix(agc): keep the shader preparation lifecycle mutex alive through exit #9738 (7a77240aa);Thread.cpp,Process.cppandException.cppfrom build(libkernel): compile Thread on macOS #11246, build(libkernel): compile Process on macOS #11282 and build(libkernel): compile Exception on macOS #11249;DirectMemory.cpp, libc'sGuestAllocations.cpp, and the AGC driver'sLifecycle.cpp(with itsDriverMemory.hpp),GuestMemory.cpp,GuestBufferMemory.cppandTexture.cppfrom feat: macOS host support (Apple Silicon via Rosetta 2, Vulkan on MoltenVK) #8658's branch; and anextern char** environ;declaration inFrameRecorder.cpp.On that base,
Resources.cppwasmain's file, then this PR's exact file at1ac3e3561:Resources.cppagc_driver_pooled_buffer_rangemain@Gonic0 (M5 Pro) and @maximilianfv (M1 Pro) report the same split above.
The file's text outside
__APPLE__blocks equalsmain.python3 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)libkernel, libc and the AGC driver to build there (fix(libkernel): build the file, module and rtld calls on macOS hosts #10552, feat(libkernel): process, thread and clock queries on macOS hosts #7804, feat(libkernel): guest memory protection and VirtualQuery on macOS hosts #10898)