Repository navigation
feat(tools): run the Vulkan validation gate on Windows - #12038
Conversation
|
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.
|
|
One Windows case the gate can't see yet: an elevated process. The loader documents that " Measured with a build of
The test passes in all three, so only the loader lines tell them apart. Two ways to close it, either enough: refuse to run on Windows when elevated ( AI-assisted: yes (Claude Code) |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head 69eae14b73c9d10dd30a884adf5eea7f20feadfa and the new Windows elevated-process report. That report identifies a real false-clean path in the tool's primary guarantee.
parse_report currently rejects VUID/SYNC diagnostics and the loader's “was not found but was requested” message, but it never verifies that VK_LAYER_KHRONOS_validation was actually inserted. The official Vulkan Loader documentation says all four VK_*_LAYER_PATH discovery variables are ignored when the application runs with elevated privileges: https://github.com/KhronosGroup/Vulkan-Loader/blob/main/docs/LoaderLayerInterface.md#exception-for-elevated-privileges . The reported Windows run shows that, when the validation layer is available only through VK_LAYER_PATH, it can therefore be absent without the warning this parser relies on. The gate may then report a clean run with no validation layer active.
Please make layer activation observable and required for every selected test, for example by forcing VK_LOADER_DEBUG=layer and having parse_report require the loader's insertion message for VK_LAYER_KHRONOS_validation (with regression tests for both present and absent messages). Refusing elevated Windows runs is another safe option, but must also be covered by tests. Until this is fixed, the gate can claim validation coverage it did not execute, so I am requesting changes.
69eae14 to
d992ee4
Compare
|
Thanks, both. Fixed in
I haven't reproduced the elevated case myself. The PR description is updated. |
THEMANnt
left a comment
There was a problem hiding this comment.
Reviewed d992ee4c6e26cd1512a62ce8a42736a44860cb74 — comment; validation remains incomplete. Windows containment and validation-layer attribution pass the focused suite, but current-head hosted Build still awaits authorization.
Windows/Python 3.11: 25/25 tests passed, including real child-process exit/timeout cleanup, source/artifact guards, and rejection of a CTest success without the insertion line or with validation diagnostics. The author base runs 24 cases with 18 POSIX-only skips. Old-production API errors are not behavioral regression proof. These integration fixtures simulate Vulkan output; I did not run a physical-GPU comparison. Complete the authorized hosted check before final approval.
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewing exact head d992ee4c6e26cd1512a62ce8a42736a44860cb74 against current main 319621ba2e578bd053b0baba2877fe70520e0737.
What I ran
git diff --checkagainst currentmain: passed.git merge-tree --write-treeagainst currentmain: clean merge tree (f2376e0c60664cf5fef23081f222ea49e3af0476).- Extracted this exact head's
tools/vulkan_validation_gate.pyandtools/tests/test_vulkan_validation_gate.pyinto a temporary directory and ran the full focused Python suite on Linux: 25/25 passed. - Inspected the Windows Job Object/process-launch code, test changes, and hosted checks. Windows, relinker platform matrix, ASan, register-context, write-watch, conventions, labels, report, and eligibility passed; hosted Linux was skipped.
- I did not run the Windows-specific path locally. The Windows job result is from hosted CI.
Results
| Check | Result | Evidence |
|---|---|---|
| Patch whitespace | Pass | Local git diff --check against current main |
| Mergeability | Pass | Local merge-tree against current main |
| Focused Python tool tests | Pass | 25/25 on Linux, from the exact PR head |
| Hosted Windows / relinker / sanitizer / conventions | Pass | GitHub Actions for this head |
| Hosted Linux build | Skipped | GitHub Actions |
| Windows Job Object runtime behavior | Not independently reproduced | Windows hosted check passed; my local suite ran on Linux |
Findings
Blocking
- The branch is no longer based on current
main. Its merge base isd13b3d7c55a433c7a5040076d849dbe59fad4b53; currentmainis319621ba2e578bd053b0baba2877fe70520e0737(1,135 commits ahead of this branch).CONTRIBUTING.mdrequires one branch per topic based on currentmain. The merge preview is clean and the new layer-insertion check addresses the prior false-clean finding. Please update/rebase onto currentmainand rerun the focused Python suite and Windows validation on the resulting head before merge.
Sources
Not verified
- I did not execute the Windows Job Object code locally.
- The end-to-end RTX 5080 result is author-reported; I did not reproduce the elevated Windows case.
- The branch's hosted Windows result predates rebasing onto current
main.
To get this to a good state
Rebase/update onto current main 319621ba2e578bd053b0baba2877fe70520e0737. Then rerun the focused Python tests and the Windows validation-gate path on that head, confirming that each selected test logs actual insertion of VK_LAYER_KHRONOS_validation and that process descendants are terminated on normal completion and timeout.
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewing exact head d992ee4c6e26cd1512a62ce8a42736a44860cb74 against main.
What I ran
git diff --checkagainst currentmain: passed.git merge-tree --write-treeagainst currentmain: clean merge tree (f2376e0c60664cf5fef23081f222ea49e3af0476).- Extracted the exact PR-head tool and test files to a temporary directory and ran the focused Python test file on Linux: 25/25 passed.
- Reviewed the Windows process containment and validation-layer reporting changes. The author reports all 25 tests passing on Windows, all eight CTest groups passing, plus an end-to-end hardware run; the GitHub Build workflow was not run for this PR, while conventions and labels pass.
Results
| Check | Result | Evidence |
|---|---|---|
| Patch whitespace | Pass | Local git diff --check |
| Mergeability | Pass | Local merge-tree |
| Focused Python tool tests | Pass | 25/25 on Linux, exact PR-head files |
| Windows Python tests and CTest groups | Pass, author-reported | 25 tests and 8/8 CTest groups on Windows, exact head |
| End-to-end Vulkan gate | Pass, author-reported | RTX 5080; all eight iterations logged actual layer insertion, no new diagnostics, no surviving processes |
| Hosted Build workflow | Not run | Only conventions and labels ran in GitHub Actions |
Findings
Blocking
None.
Non-blocking
None. The earlier false-clean case is covered by requiring the loader's instance-layer insertion message per selected test. The Windows path assigns a suspended process to a Job Object before resuming it; the reported Windows descendant-cleanup negative control and end-to-end run provide useful coverage.
Sources
Not verified
- I did not execute the Windows Job Object code locally.
- The Windows suite and hardware run are author-reported; only the 25-test Linux run was independently reproduced here.
- GitHub's Build workflow did not run on this PR.
To get this to a good state
No blocking issue remains in the reviewed change. The Windows-specific behavior has direct author-run coverage and the complete Python suite passes locally on Linux.
✅ AMD GPU test: passedPR head
Unstable on main (3, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (13, not counted against this PR)Validated but not compared (6, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 769 tests)Skipped (60)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Could a collaborator start the Build workflow for this PR when there's a slot? The change only touches Head |
|
I ran the current head |
|
Re-checked after the red Build: the linux job failed in |
What
tools/vulkan_validation_gate.pyrefused to run on Windows because it had no way to contain a command's process tree. On Windows, each configure, build and CTest command now starts suspended (CREATE_SUSPENDED), joins a new job object, and only then has its thread resumed, so none of its descendants can start outside the job. When the command exits or times out, the job is terminated, askillpgdoes for the process group on POSIX. The POSIX path is unchanged.CTest runs also get
VK_LOADER_DEBUG=layer, and a selected test now passes only if its output contains the loader'sInsert instance layer "VK_LAYER_KHRONOS_validation"line. Before, a run without the layer could report clean: the loader ignoresVK_LAYER_PATHin an elevated process (exception for elevated privileges) and then prints no missing-layer message.report.jsonrecordslayer_insertedfor each test.The synthetic fixture tests were skipped on Windows; they now run there as well. Two Windows fixes in the tests:
CMakeLists.txttakes the Python path with forward slashes, because backslashes are escapes in CMake strings.The fixture program prints the loader's insertion line unless a test turns it off. New tests in
ReportTestsandLayerTestscover output with and without the line.Each fixture test builds two CMake fixtures and takes about 2.5 s on Windows. To keep every CTest group under 30 s on the Windows runners, the two fixture groups are split into eight: new
TimeoutTests,ValidationTests,LayerTests,OutputTests,SetupTestsandCommandTestsclasses, registered inCMakeLists.txt. No existing test body changes.docs/dev/BUILD.mdnow describes the containment on both platforms instead of the Windows refusal, and the insertion-line requirement.Basis: Job Objects, AssignProcessToJobObject, Thread32First.
Tested
Windows 11 Home (26300), Python 3.13.9, WinLibs GCC 15.2.0, CMake 4.3.1, Ninja.
Current head
b516ccdc03:main096c45570fmerged in (only conflict: the removed Windows refusal test next to the new relative-executable test), then the #11809 timeout tests moved intoTimeoutTests.tools/tests/test_vulkan_validation_gate.py, all nine classes: 32 tests pass, none skipped. With a relinker-only configure (-DANYPS5_RELINKER_ONLY=ON -DBUILD_TESTING=ON),ctest -R '^vulkan_validation_gate_' --parallel 4passes 9 of 9 groups; the slowest takes 9.9 s.check_main_sync,configure_seed,configure_seed_round_tripsanddebt_splitrun 1.25 to 2.5 times faster on this machine than in the latest Windows CI job. At 2.5 times, the slowest group takes about 25 s there.c893d2c747(before the split) merged intomain4f41f4c375: relinker-only build,ctest --output-on-failure --timeout 120 --parallel 4, 689 of 689 pass.vulkan_validation_gate_integrationtook 13.6 s there with the timeout tests in it, so they now run as their own group.tools/check_conventions.py --base 4f41f4c375passed.Earlier, on
d992ee4c6e(maind13b3d7c55). The containment code is unchanged since9f4735ddd8, where the containment negative control and the Linuxrun()check were run.not insertedremoved from the failure condition, both new tests fail.run()with a plainPopenthat kills only the direct child.test_command_cleanup_removes_descendant_after_parent_exit_and_timeoutthen fails in both subtests, because the grandchild outlives the command.ReportTestspass.run(), called directly, kills a grandchild after both a timeout and a normal exit. The fixture tests were skipped there because CMake and Ninja are not installed.--baselineismain801d5b9bc1;--candidateis fix(shader): load and store 16-bit data of converted packed images #11580 merged onto thatmain(a16d5cc761).--target agc_driver_image_converted_unorm_tests --target agc_driver_image_packed16_load_tests --tests '^agc_driver_image_(converted_unorm|packed16)' --gpu NVIDIA --repeat 4 --runtime core/libs/libs/unpatched, plus--cmake-argforgcc,g++and the Python path.complete, exit 0, 5 tests with 4 iterations per build, and no new diagnostics.layer_insertedis true for every test in all 8 iterations. Every test log selected the RTX 5080, and no process was left running. The run took 8.6 min, including both builds.tools/pr_overlap.py --branch feat/validation-gate-windowsreported no overlap with other open PRs; the seven conflicting PRs it could not compare don't touch the gate.Not run: the fixture tests on Linux or macOS (left to CI), the hosted Build workflow, an elevated process (the loader behaviour is from the report in this PR's comments and the loader documentation), and the end-to-end hardware run on the current head.
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)