Skip to content

test(agc): spawn the lazy resolved ABI reference child without a shell - #10650

Open
maximilianfv wants to merge 2 commits into
boykopovar:mainfrom
maximilianfv:fix/agc-macos-lazy-abi-spawn-child
Open

maximilianfv wants to merge 2 commits into
boykopovar:mainfrom
maximilianfv:fix/agc-macos-lazy-abi-spawn-child

Conversation

@maximilianfv

@maximilianfv maximilianfv commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

What

core/libs/prx/libSceAgcDriver/tests/execution/LazyResolvedAbi.cpp starts its --eager-reference child with std::system, which runs the command through /bin/sh -c. On macOS /bin/sh is a SIP-protected platform binary, so dyld removes every DYLD_* variable when it starts it. The child then loses DYLD_FALLBACK_LIBRARY_PATH, which is how the tests find the LunarG SDK's libvulkan.1.dylib (the DYLD_LIBRARY_PATH setup from #1 is stripped the same way). It fails with Vulkan loader: Failed loading libvulkan.1.dylib. Without ANYPS5_REQUIRE_VULKAN the child skips with "no usable Vulkan device" (exit 77, VulkanTestDevice.hpp:25-26, LazyResolvedAbi.cpp:681); with it, OpenVulkanTestDevice rethrows (VulkanTestDevice.hpp:24) and the child's main returns 1 (LazyResolvedAbi.cpp:723). Either way the parent stops with lazy resolved ABI: the eager reference process failed. This makes agc_driver_lazy_resolved_abi and agc_driver_lazy_resolved_abi_switch_set fail on every macOS setup that points dyld at the Vulkan loader through a DYLD_* variable; a loader installed on dyld's default search path (for example /usr/local/lib) is unaffected.

On hosts other than Windows, the child now starts with posix_spawnp(argv[0], {argv[0], "--eager-reference"}, environ) and waitpid. That passes the parent's environment on unchanged without going through a shell, the same way core/relinker/relinker/tests/RelinkerProcess.hpp already uses posix_spawn(..., environ). The test still requires exit status 0. Following oneandonlydean's review, the wait (WaitExit) retries waitpid while it fails with EINTR, so the test never fails, and its cleanup never deletes the reference and cache paths, while the child is still running. A nonzero exit, a signalled exit and a failed spawn still count as failures. A self-check (CheckWaitExit), run before the eager child starts, covers exit 0, exit 3, a child killed by SIGKILL, a failed spawn, and one interrupted wait. In that last case the forked child blocks on a pipe until a SIGALRM handler, installed without SA_RESTART and aimed at the waiting thread with pthread_kill, writes to it, so the child cannot exit before the signal arrives. Windows keeps std::system. The change is limited to this test. The other self-spawning tests that use std::system (ShaderDiskCache.cpp, AsyncLog.cpp) are left alone because their children pass without loading Vulkan.

Prior art: commit 5546b23 in #8658 by @iamthehimansh (2026-10-23, closed) found the same cause in this test. It fixes it differently: it keeps std::system and, on macOS only, adds DYLD_LIBRARY_PATH="..." to the front of the shell command, so it passes on only that one variable (not DYLD_FALLBACK_LIBRARY_PATH). The fix was never split out of #8658, and neither #10362 nor #10419 carries it. #7201 and #7332 do not change this test.

Tested

Apple M1 Pro, macOS 27.0.1, x86_64 build under Rosetta 2, Vulkan through MoltenVK from the LunarG Vulkan SDK 1.4.363. main alone does not build on macOS, so I built in a tree made of mugurc's macOS series (ci/macos-series-2, 27e968974) merged with main at 522e32e3b6, plus these local changes, all identical in both runs: the macOS build parts of #10419, #10540, #10562, #10641, #10642, and two of my unopened fixes (the image table reuse chain-pool check, and an OpMemoryBarrier before 32-bit image atomics in SpirvImageEmitter.cpp). Only this PR's change was toggled. Both runs used ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ctest --test-dir build -R '^agc_driver_lazy_resolved_abi' -E 'audio_in|voice' -V.

  • Cause: with DYLD_FALLBACK_LIBRARY_PATH exported, sh -c 'echo $DYLD_FALLBACK_LIBRARY_PATH' prints an empty line. A small C probe shows that posix_spawn passes the variable to the child and system() drops it. A wrapper that exports the variable again inside the child makes the unchanged test pass.
  • Without this change: both tests fail (0/2). The child cannot load libvulkan.1.dylib, and the parent stops with the eager reference process failed.
  • With it: both tests pass (2/2): 6 shaders drew, 5 of them ABI-resolved; ... the lazy run wrote none of the 43 shader cache files eager wrote again.
  • Validation: neither log contains was not found but was requested by env var VK_INSTANCE_LAYERS. The run without the change has 0 Validation Error lines. The run with it has 4, all VUID-VkImageViewCreateInfo-imageViewFormatReinterpretation-04466 (vkCreateImageView, portability subset, imageViewFormatReinterpretation is VK_FALSE on MoltenVK). Each test logs one in the eager child's draw phase and one in the parent's lazy draw phase. Without the change, neither phase runs: the child fails while loading Vulkan, and the parent stops at the check of the child's exit status. So these errors come from image view code that this test never reached before, and this change touches no device or image code.

EINTR follow-up (1e4cc24bb5), on mugurc's macOS series (ci/macos-series-2, 27e968974) merged with main at 2742004ed2, same command, 3 runs each with the previous head 569e4d36da and with this head: both tests pass 2/2 in all six runs, including the new self-check. Every run has the same 4 VUID-VkImageViewCreateInfo-imageViewFormatReinterpretation-04466 errors and no layer-not-found line. Not checked: whether the 50 ms signal can land before the main thread reaches waitpid. The check then still passes, but without exercising the retry.

Not tested on Linux or Windows. Windows keeps the same code path, and on Linux posix_spawnp with environ gives the child the same environment std::system did.

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 under Silent stubs in their library's technical debt file
  • 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
  • Fixes: none
  • Unblocks: none
  • GPUs tested: Apple M1 Pro, MoltenVK 1.4.2 (Vulkan SDK 1.4.363), macOS 27.0.1
  • AI-assisted: yes (Claude Code found the cause, made the change and wrote this description; I am responsible for every line)

🤖 Generated with Claude Code

The lazy resolved ABI test starts its --eager-reference child through
std::system, that is through /bin/sh. On macOS /bin/sh is a protected
platform binary, so dyld drops every DYLD_* variable when it starts and
the child inherits an environment without the path to the Vulkan
loader; it then skips for lack of a Vulkan device and the parent fails
with "the eager reference process failed". On hosts other than
Windows, start the child with posix_spawnp and the parent's
environment, and wait for it.

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

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 569e4d36da693f0ce8b949cb2c3e6df4e640f868. On POSIX, the test now launches itself directly with posix_spawnp and passes the current environ, avoiding the shell that strips DYLD_* variables under SIP; it waits for the child and requires a normal zero exit. Windows retains its prior std::system path. Exact-head diff-check, conventions, and merge-tree checks pass; hosted eligibility, conventions, intake, labels, and report pass, while product builds are skipped. I did not run the macOS GPU test locally; the validation results are the author’s controlled two-run report. No blocking issue found.

@github-actions github-actions Bot added the agc libSceAgc* label Oct 24, 2026

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the complete diff at 569e4d3. On non-Windows, posix_spawnp launches the same executable directly with the existing environment and waitpid propagates the child exit code; Windows retains its existing path. This matches the repository’s existing POSIX spawn helper. The macOS cause and red/green results in the PR body are detailed and consistent with the change. Local verification: git diff --check, check_conventions.py, and the current-main merge tree pass. I could not run the macOS Vulkan test locally; hosted eligibility, conventions, intake, labels, and report checks pass, while product builds are skipped by policy. The author reports four MoltenVK image-view VUIDs in the newly reached draw phases; those lines concern untouched image-view code and should be tracked separately if no existing issue covers them. The limitation is disclosed in the PR body.

@ycbuzi

ycbuzi commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

Windows run of 569e4d36da merged onto main 66e6772ca2 (8d54338d25, clean). Windows 11 Home 10.0.26200, WinLibs MinGW-w64 GCC 15.2.0 (ucrt-posix-seh r7), Release, BUILD_TESTING=ON, NVIDIA GeForce RTX 4070 SUPER. agc_driver_lazy_resolved_abi and agc_driver_lazy_resolved_abi_switch_set pass both runs of ctest --repeat until-fail:2, so spawning the reference child without a shell works with the Windows process path too.

Not run: a title. AI-assisted: yes (Claude Code)

@oneandonlydean

Copy link
Copy Markdown
Collaborator

@maximilianfv In core/libs/prx/libSceAgcDriver/tests/execution/LazyResolvedAbi.cpp:378, RunEagerReference calls waitpid once. If a handled signal interrupts it, it returns -1 with errno == EINTR while the eager child is still running and unreaped. The caller at line 628 then throws, and the clean lambda (lines 600-608, run at 650) deletes the reference and cache paths that child may still be writing.

Could you retry waitpid while it fails with EINTR, until the child is reaped? Please keep nonzero and signalled exits as failures, and make sure cleanup never runs while the child is alive. A case that injects one EINTR before a successful wait, next to exit-0, nonzero, signal and spawn-failure cases, would cover it.

waitpid can return -1 with EINTR when a handled signal arrives while the
eager reference child is still running. The test would then fail, and
its cleanup would remove the reference and cache paths the child may
still be writing. Retry the wait until the child is reaped; a nonzero
exit, a signalled exit and a failed spawn still report a failure.

A self-check covers exit 0, a nonzero exit, a killed child, a failed
spawn, and a wait interrupted by a signal sent to the waiting thread,
whose handler is the only thing that lets that child exit.

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

Copy link
Copy Markdown
Contributor Author

@oneandonlydean Done in 1e4cc24bb5.

  • WaitExit retries waitpid while it fails with EINTR and returns only once the child is reaped, or after a failure other than EINTR, which means there is no child left to wait for. RunEagerReference returns only through it, so the clean lambda can't run while the eager child is alive. A nonzero exit returns its code, a signalled exit and a failed spawn return -1, and the caller still requires 0.
  • CheckWaitExit runs before the eager child starts and covers:
    • exit 0, exit 3, and a child killed with SIGKILL;
    • a spawn of a nonexistent path;
    • one interrupted wait. The forked child blocks on a pipe that only a SIGALRM handler writes to. The handler is installed without SA_RESTART, and a helper thread sends the signal to the waiting thread with pthread_kill after 50 ms. So the child can't exit before the signal arrives, and the test checks that the handler ran exactly once.

On the M1 Pro (MoltenVK), on mugurc's macOS series (ci/macos-series-2, 27e968974) merged with main at 2742004ed2, both tests pass in 3 of 3 runs with the old head and 3 of 3 with this one. Every run has the same 4 imageViewFormatReinterpretation-04466 errors. One limit: if the main thread takes more than 50 ms to reach waitpid, the check still passes but doesn't exercise the retry. Not run on Linux or Windows; the Windows path is unchanged.

@oneandonlydean

Copy link
Copy Markdown
Collaborator

@maximilianfv The WaitExit loop now addresses the interrupted-wait ownership bug and keeps the exit-status failures intact. The remaining part of my ask is the regression check: as you noted, the 50 ms signal can arrive before waitpid, so Interruptions == 1 and code == 0 can both pass with a one-shot wait that never saw EINTR.

Please make the check require an observed or injected EINTR before the successful reap, and show that removing the retry fails it while the corrected helper passes. Keep the exit-0, nonzero, signal and spawn-failure controls. There is no need to change the posix_spawnp/environment approach again.

@ycbuzi

ycbuzi commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

A Windows rerun of the new head 1e4cc24bb5, merged onto main 9656d366be with no conflicts: WinLibs GCC 15.2.0 r7 (MinGW-w64 UCRT), Release, one GPU at a time with ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1, ctest -V --repeat until-fail:2.

  • agc_driver_lazy_resolved_abi and agc_driver_lazy_resolved_abi_switch_set pass, both runs, on the RTX 4070 SUPER, the GTX 1650 SUPER and the Radeon iGPU (1002:164e). Each log's Selected GPU line names the device.
  • The test builds on MinGW with no warnings, so the POSIX-only parts (spawn.h, sys/wait.h, posix_spawnp, the release pipe) are guarded correctly. On Windows RunEagerReference still goes through std::system, so this run covers that path, not the new posix_spawnp one.

AI-assisted: yes (Claude Code)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc*

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants