Repository navigation
fix(agc): keep a shader library part's modules until the part is destroyed - #3173
Conversation
…royed Pipeline destroyed its shader modules right after LinkPipelineFromLibraries returned, while the pre-rasterization and fragment shader library parts built from them stay cached and are relinked later with VK_PIPELINE_CREATE_LINK_TIME_OPTIMIZATION_BIT_EXT. On AMD's Windows driver (RX 6700 XT, Adrenalin 25.10.45.05) that link faults inside amdvlk64.dll at varying offsets, reading freed heap memory, in 22 agc tests on a cold driver cache; all of them pass with APS5_NO_PIPELINE_LTO=1. Running the optimized link synchronously or without a pipeline cache did not help, keeping the modules alive did. When the optimized link is enabled, the library store takes over the modules of each newly built shader part and destroys them in ClearPipelineLibraries after the pending links have finished. Modules of a pipeline whose parts were all cached are destroyed as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dextroid17
left a comment
There was a problem hiding this comment.
A workaround is only reviewable when it says what it is working around, and this one says it exactly: destroying a shader module after pipeline creation is valid Vulkan; the deferred VK_PIPELINE_CREATE_LINK_TIME_OPTIMIZATION_BIT_EXT link that the library parts stay cached for faults inside amdvlk64.dll at varying offsets, reading freed heap memory; and so it is a driver defect rather than a use-after-free on our side. Writing that down — driver, module, symptom, and the fact that the specification allows what we do — is what turns a workaround into a decision someone can revisit when the driver changes.
The ownership transfer is the right mechanism. Having the library store take over the modules of each newly built shader part and destroy them in ClearPipelineLibraries after the pending links have finished ties the lifetime to the thing that actually needs it rather than to the pipeline that created it. Doing that for every part built while the optimized link is enabled, rather than only for the ones that faulted, is the kind of difference that prevents a second bug report later.
One thing worth stating explicitly, since the fault belongs to one driver: the workaround is unconditional. There is no vendor or device check anywhere on this path — APS5_NO_PIPELINE_LTO=1 is the only way out of it. That is the right default for a fault seen on a single vendored driver, because the alternative is a title that keeps crashing on a driver nobody wrote a guard for; but it does mean the entry should say so plainly, so whoever meets the same fault elsewhere knows where to look first.
Written with AI assistance.
|
Windows title test on current main After passing the earlier audio and resummarize stops, the game faults at A private probe outside Git narrows this to the initial fragment-shader library creation, before that creation returns: This appears to be a separate initial library-compilation failure, beyond the retained-module lifetime handled here. I am reducing it further; I have not rerun the full AGC suite or established a driver-wide cause. Game data and generated captures stay local. AI-assisted investigation and write-up; these are results from actual local Windows runs. |
…kground linking #4152 dropped the device-wide module list that #3173 added, so Pipeline destroys the shader modules right after the library parts are created, also for the parts prebuilt at registration. AMD's Windows driver still reads them during the background link-time-optimized link and faults inside amdvlk64.dll. Hand the modules of each new part to the device's library store again and destroy them in ClearPipelineLibraries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Carrying @poonasor's #3149, which the intake check closed when it was opened. The change is theirs (
8acd0113, cherry-picked onto currentmainwith poonasor kept as the author; it applied without conflicts).What
Pipelinedestroys its shader modules right afterLinkPipelineFromLibrariesreturns, but the pre-rasterization and fragment shader library parts built from them stay cached and are relinked later by the backgroundVK_PIPELINE_CREATE_LINK_TIME_OPTIMIZATION_BIT_EXTlink (#2713). On AMD's Windows driver that link faults insideamdvlk64.dll, at varying offsets, reading freed heap memory. Destroying a module after pipeline creation is valid Vulkan, so this looks like a driver defect; the change works around it.When the optimized link is enabled, the library store takes over the modules of each newly built shader part and destroys them in
ClearPipelineLibraries, after the pending links have finished. Modules of a pipeline whose parts all came from the cache are destroyed as before, and nothing changes withAPS5_NO_PIPELINE_LTO=1. Cost: one retainedVkShaderModuleper stage of each unique shader part, for the device's lifetime.This is the AMD half of #2777: #2869 fixed the destroyed layout, which did not change the AMD failures (as noted there and in #2755). Addresses the segfaults in #2944 and #2755.
How the cause was narrowed down (RX 6700 XT, on
57f96fd2+51078717,agc_driver_vertex_fetchandagc_driver_provoking_vertex):LinkPipelineFromLibrariesjob, insidevkCreateGraphicsPipelines.APS5_NO_PIPELINE_LTO=1: pass.VkPipelineCache: still faults 3/3.VK_LOADER_LAYERS_DISABLE=~implicit~): still faults.Tested
@poonasor's run:
Windows 11 Pro 23H2 (22631), WinLibs GCC 15.2.0 r7, Ninja, Release,
BUILD_TESTING=ON. AMD Radeon RX 6700 XT (Navi 22, 0x73df), Adrenalin 25.10.45.05 (driver 32.0.21045.5002), the only Vulkan device.%LOCALAPPDATA%\AMD\VkCacheemptied before eachagc_run,ctest -R "^agc_" -j1:main379d8b4eamdvlk64.dll, plussdwa_float_selectorsandsdwa_ldexp_f16main379d8b4e+ this PRpixel_interlock,sdwa_float_selectors,sdwa_ldexp_f16, no SEGFAULTThe 22:
agc_pipeline_cache,agc_pipeline_library, andagc_driver_mesh,conservative_rasterization,depth_bias_per_face,dual_source_blend,exec_host_lanes,ngg_vertex_wave_info,packed_unorm_color_target,pixel_fixed_point_position,pixel_helper_lanes,pixel_interlock,pixel_lds,pixel_lds_lane_slots,pixel_uint16_export,pixel_valid_mask,pixel_wave_reduction,prepared_exports,provoking_vertex,scalar_swappc_fetch,srgb8_color_target,vertex_fetch.Full parallel
ctestwith this PR (warm driver cache): 724 of 732 passed, 5 skipped (video_outflip tests,guest_video_out_wqhd_detection_no_vulkan), the same 3 failed.The 3 remaining failures are wrong results, not faults, and are not addressed here: the two
sdwaf16 ones are the values already in #2944;pixel_interlockno longer faults and fails withpixel interlock wave64 BDA fault: pixel 0 counted 1 of 0 overlapping read-modify-writes, which it also does withAPS5_NO_PIPELINE_LTO=1.Title: Dreaming Sarah (PPSA02929), which faulted in
amdvlk64.dll+0x21eef34at its first graphics submission on this machine (discussion #3018), relinked against this build runs for 90 s with no fault in the log. I did not check the rendered output.Not tested on NVIDIA, Intel or Linux.
python tools/check_conventions.py --base origin/mainpasses.Title confirmation from @poonasor (comment on #2944): Dreaming Sarah (PPSA02929), which faulted in
amdvlk64.dllat its first graphics submission on the RX 6700 XT, reaches gameplay with this change and renders correctly at about 60 FPS, with audio and controls working and no fault in the log.My run on RDNA 3.5: Radeon 860M (
0x1114), AMD driver 32.0.13058.8001, Windows 11 (26300), WinLibs GCC 15.2.0 r7, Release,%LOCALAPPDATA%\AMD\VkCacheandbuild/tests/shader_cacheemptied first,ctest -R agc_ -j1(also on #2944):main181420famdvlk64.dllmain181420f + this changeFork CI on this branch (run): Windows and the three relinker jobs pass. Linux fails
agc_driver_image_packed16_loadandagc_driver_flat_apertures, which also fail with an unrelated change on the samemain(run), andagc_unit_shadow("the partial publish did not copy exactly the partly covered unit", the open #1429), which creates no pipelines.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt.mdfiles or images added to the repository (attach them to this PR)