Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Naga snapshot tests should not include binary files #6332

Open
jimblandy opened this issue Sep 26, 2024 · 3 comments
Open

Naga snapshot tests should not include binary files #6332

jimblandy opened this issue Sep 26, 2024 · 3 comments
Labels
area: tests Improvements or issues with our test suite naga Shader Translator

Comments

@jimblandy
Copy link
Member

jimblandy commented Sep 26, 2024

The Naga snapshot tests should be changed to take SPIR-V input as assembly language, not binary files. Alternatively, CI should disassemble the binary SPIR-V files and assert that they match accompanying source files.

The xz utils backdoor concealed the portion of its code that should have aroused suspicion as test input for the decompression library. Binary data discourages review in a way that textual data does not. (As evidence for this, recent PRs from perfectly trustworthy contributors to wgpu have included .spv input files that didn't match the disassembly present alongside them in the PR. These problems were caught in PR review, but the author themselves hadn't noticed.)

In wgpu, the naga/tests/in/spv/ directory contains various .spv binary files. There are a few approaches:

  • We could have snapshot tests take .spvasm files as input instead. We could add a dev dependency on a SPIR-V assembler, or require the spirv-as command to be present on the path. We would need to assess the impact on test run time.
  • We could have CI disassembly each .spv file and verify that the result matches the accompanying .spvasm. file. Thus, any PR that affected a .spv file would also inlude a diff to its .spvasm file.
@jimblandy jimblandy added area: tests Improvements or issues with our test suite naga Shader Translator labels Sep 26, 2024
@cwfitzgerald
Copy link
Member

I don't see easy bindings to spirv-as, but shelling out likely isn't too costly, and it's not unreasonable to expect the vulkan sdk installed and in-path.

@evilpie
Copy link

evilpie commented Sep 27, 2024

Rust does have the advantage compared to C code (including xz) that the build system is a lot more sane and it's harder to hide some nefarious instructions in there. It would be nice if wgpu didn't use build.rs at all, but the current usage is at least very easy to audit.

@Imberflur
Copy link
Contributor

Some thoughts on this:

  • Not having to coordinate these files would make it easier for contributors adding new tests.
  • A potential downside with shelling out to spirv-as is if there are inconsistencies in the generated spirv binary from contributors having different versions installed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: tests Improvements or issues with our test suite naga Shader Translator
Projects
None yet
Development

No branches or pull requests

4 participants