Repository navigation
fix(agc): check the deferred destruction resolver only against live devices - #11705
adriadam10 merged 2 commits into
Conversation
…evices The deferred destruction registry kept the first device's vkGetDeviceProcAddr after that device was destroyed. VulkanDevice unloads the Vulkan loader with the device, and on macOS the next load can map it at another address, so every later device in the process failed with "deferred destruction found two different device resolvers". The check now applies only while another device is still registered; the new device's resolver replaces the old one.
|
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.
|
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head be7db251ad005c9ead50177d190b9f04a17743f9 against current main ddd72f488b6070c3864c8782cc5691ca3e30e680. Changes requested for one reproduced resolver-routing regression.
[P2] Preserve the resolver for a previously returned device-proc wrapper across reload. deferringDeviceProc routes passthrough calls through the single process-wide registry().resolver. This change replaces that resolver whenever the registry has no deferred devices. After DeferDestructionToIdle(first, mockProc) and DestroyDeferredObjects(first), setting up second with reloadedProc changes what the already-returned firstProc routes to: firstProc(first, "vkQueueWaitIdle") now returns reloadedQueueWaitIdle, not mockQueueWaitIdle. I reproduced this by compiling the exact PR implementation/test with that additional assertion; it fails. DestroyDeferredObjects only removes deferred tracking; production VulkanDevice calls it before vkDestroyDevice, so the old device may still be live during this interval. The existing test explicitly expects a previously returned wrapper to pass through after DestroyDeferredObjects, but does not cover a subsequent resolver replacement.
Please preserve resolver routing for that old wrapper/device across the reload (or establish that this transition is unsupported and remove the stale-wrapper behavior contract), and add a regression for the sequence above. The requested loader-reload case and the two-live-device rejection should both remain covered.
Current-main conventions and git diff --check pass. Hosted policy checks pass; product test jobs are skipped by path policy. This review's additional focused resolver regression was compiled and executed locally; it did not run the full AGC suite.
…rough The previous commit replaced the process-wide resolver when a device was set up after the last deferred device went, so a wrapper returned for the earlier device, which may still be alive until vkDestroyDevice, passed through to the later device's loader. The registry now keeps the resolver of every device it set up and passes each device's calls through its own one; the most recent resolver is used only for devices it never set up. Two live devices with different resolvers are still refused.
|
@MarcZX10 thanks, you're right. Fixed in 846e298.
The GPU suite numbers in the description are from be7db25. I haven't re-run them on this head yet. For a device that is still live, both heads pick the same resolver. Written with AI assistance (Claude Code). I ran the builds and tests above on my machine. |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 846e2989232e60ebb4c1720c7bdb8d68b095b928.
The updated per-device resolver map fixes the earlier-wrapper regression: a wrapper returned for the first device retains that device's resolver after a later loader reload, while the new device uses its own resolver. The live-device resolver mismatch guard remains, and the regression test now checks both routing directions plus rejection of two live devices with different resolvers.
I applied the exact PR changes since the current-main merge base in a disposable review tree, built agc_deferred_destruction_tests, and ran ctest -R '^agc_deferred_destruction$': 1/1 passed. The normal host CPU guard was bypassed only in that scratch tree for this CPU-only test, then restored; tracked source is clean. The PR correctly distinguishes GPU-suite numbers from its previous head; its current-head focused tests pass on Linux and macOS per the author. Hosted eligibility, conventions and label checks pass; product jobs are skipped by path policy.
No blocking issue found in the updated diff.
|
GPU re-check of Machine: Apple M2 Max, macOS 27.2, x86_64 Release build under Rosetta 2.
GPUs tested: Apple M2 Max (MoltenVK 1.4.2 and MoltenVK main + #2704) Written with AI assistance (Claude Code). I ran the builds and tests above on my machine. |
|
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), Khronos Validation Layers nightly 1.4.366 (b2b44c0d37), WinLibs GCC 15.2.0 (x86_64-ucrt-posix-seh, r6), Release, Same focused run on both:
The tree also carried the open PRs #11873, #11943, #11721, #11708, #11703, #11702, #11701, #11948, 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. |
|
Checked the per-device resolver map against the diff: a wrapper returned for an earlier device keeps its own resolver, a later device uses its own, and two live devices with different resolvers still throw. The new case in |
What
Deferred destruction (#10062, MoltenVK only) keeps the
vkGetDeviceProcAddrof the first device in a process-wide registry and throwsdeferred destruction found two different device resolverswhen a later device brings a different pointer. The registry never forgets it: it is still there after its device is gone.VulkanDeviceunloads the Vulkan loader withSDL_UnloadObjectwhen the device goes, and on macOS the nextSDL_LoadObjectcan maplibvulkan.1.dylibat another address. In an x86_64 probe, threedlopen/dlsym("vkGetDeviceProcAddr")/dlcloserounds returned0x10937b240,0x1094ea240and0x10937b240. So every test that opens a secondVulkanDevicein the same process fails at device setup. imellag saw the same message on 9 tests in #11551, changing from run to run.The registry now keeps the resolver of every device it set up and passes each device's calls through its own one, so a wrapper returned for an earlier device keeps reaching that device's loader after a later device is set up. The most recent resolver is used only for devices the registry never set up. A device set up after the last deferred device went may bring a different resolver; two live devices with different resolvers still throw.
agc_deferred_destructiongets a case that sets up a device, destroys its deferred objects, and sets up the next one with a different resolver, as after a loader reload. It checks that the next device is accepted and passes through to its own resolver, that the earlier device's wrapper still passes through to the earlier resolver (MarcZX10's review), and that a second live device with the first resolver is still refused.Tested
macOS 27.2, Apple M2 Max, x86_64 Release under Rosetta 2.
mainalone doesn't build on macOS yet, so the GPU runs usemain27ad7244d2plus the open macOS build PRs (#10366, #7631, #10659, #10660, #11249, #11358, #10669, #10552, #7804, #10540, #10676, #11447, #11437) and #11554, which loadslibvulkan.1.dyliband makes this reachable.ANYPS5_NO_SHADER_CACHE=1,ctest -R '^agc' -j1 --timeout 30, without the flip, display andlds_atomics64tests (1027 tests).deferred destruction found two different device resolvers(2 of those as 30 s timeouts);agc_driver_indirect_arguments_pending_producerloadslibvulkan.so.1on macOS,agc_driver_stencil_ref_export_unusedhits a MoltenVK MSL compile error,agc_driver_rect_list_indirectpasses on MoltenVK main with MSL: Preserve negative zero in precise multiplication KhronosGroup/SPIRV-Cross#2704 (signed zero). No test fails with the change that passes without it.agc_deferred_destruction(the test andDeferredDestruction.cppbuilt alone, macOS x86_64 Apple clang and Linux Ubuntu GCC 15.2, same results): onmain'sDeferredDestruction.cppthe new case fails with "a device set up after the last one went could not use a resolver from a reloaded Vulkan loader"; on the first head of this PR (be7db25) it fails with "an earlier device's wrapper passed through to a later device's resolver"; at the current head the whole test passes.VulkanDevicedoesn't make (it destroys the device right afterDestroyDeferredObjects).agc_driver_address_passes,clamped_global_fill,compute_table_calls,depth_copy_draw,host_import_teardown,indexed_copy) withVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation,ANYPS5_REQUIRE_VULKAN=1,ctest -V: all pass, the layer loads, noVUID-errors. Without the change they stop at device setup, so there is no run to compare against.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)🤖 Generated with Claude Code