Skip to content

fix(vulkan): create shader modules after stage ownership is committed - #128

Merged
soltros merged 2 commits into
mainfrom
fix/stage-module-ownership
Nov 11, 2026
Merged

soltros merged 2 commits into
mainfrom
fix/stage-module-ownership

Conversation

@soltros

@soltros soltros commented Nov 11, 2026 •

Copy link
Copy Markdown
Owner

Translation creates a Vulkan shader module while its Stage is still local. Copying SPIR-V/constants or growing p.stages can then throw, and #123's failure handler cannot find the module because that stage never reached the Program. Commit the stage first and create its module immediately afterward.

Reorder all seven graphics/compute/task/mesh/fallback stage paths. Metadata and SPIR-V collection retain their existing behavior; only module creation moves after p.stages.push_back. Once a handle exists, the Program already owns its slot, so existing partial-stage cleanup can release it if later translation work fails. Successful shader inputs, stage ordering and pipeline behavior are unchanged. No public renderer API or generated shader headers change.

Addresses another allocation-failure ownership path under #18, which remains open pending gameplay/RSS validation. This is distinct from #125's cache-reader guard.

The new authored fixture compiles real stage-assembly statements and the real translation failure/completion handlers. It fails on unchanged source due to an orphaned module. With the fix, 66 allocation-fault/success cases pass across seven stage paths and both mesh modes, including cleanup of an earlier owned stage, retained successful ownership and exactly-once destruction. The cases also pass with ASan/UBSan and ASAN_OPTIONS=detect_leaks=0 (LeakSanitizer is unavailable under host tracing). The prior 16,000-case failed-translation fixture passes. These are representative metadata copies and real C++ allocation faults with allocation-free fake Vulkan handles, not the complete translator or real driver; no game assets, shader binaries or actual OOM were used.

Published tree matches local HEAD 758bd950dd34eeb9be78545aaf6fe65301254f6d. Full local suite: 284 passed, 12 known host/tool-dependent failures, 56 skipped. Hosted validation completed successfully: Windows 330 passed/0 failed/22 skipped; Linux 341 passed/0 failed/11 skipped. Windows Builder, Linux AppImage/backend/player build, lint, focused SPIR-V and native F2I jobs all passed. This PR now integrates main c003c84, including the independently validated #124/#125/#96. #127's tested content exactly matched that main tree and its temporary validation PR is closed. The queue now records those merged fixes and the next audit targets. Validation is complete.

Fixture portability follow-up: the initial head's native Windows/Linux packaging passed, but both source suites rejected an unused VK_NULL_HANDLE constant under Clang warnings-as-errors. Removed the unused fixture declaration; warning checks remain enabled. All 66 ownership cases still pass locally normally and with ASan/UBSan (leak scanning disabled as documented). Passing hosted checks ran on a379e8a; the combined full local suite finished: 284 passed, the same 12 host/tool-dependent failure paths, and 56 skipped.

@soltros soltros mentioned this pull request Nov 11, 2026
3 tasks done
@soltros
soltros marked this pull request as ready for review November 11, 2026 19:53
@soltros
soltros merged commit 228d2c4 into main Nov 11, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant