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

Move buffer slice OOB check from panic in wgpu to validation error in wgpu-core #6493

Draft
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

ErichDonGubler
Copy link
Member

@ErichDonGubler ErichDonGubler commented Nov 4, 2024

Connections

Exercised by webgpu:api,validation,encoding,cmds,render,setIndexBuffer:offset_and_size_oob:*, which was disovered to be failing in Firefox in this webgpu-backlog1 run on a Windows debug build in try:bed05e732fb557b6173872c508612399b6bc365e.

Description

wgpu implemented OOB checks on BufferSlice creation as a panic. However, it should be implemented as a validation check in wgpu-core. So fix it. 😀

Testing

  • Exercised by webgpu:api,validation,encoding,cmds,render,setIndexBuffer:offset_and_size_oob:* in Firefox.

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@ErichDonGubler ErichDonGubler added type: bug Something isn't working area: validation Issues related to validation, diagnostics, and error handling labels Nov 4, 2024
@ErichDonGubler ErichDonGubler self-assigned this Nov 4, 2024
@ErichDonGubler ErichDonGubler requested a review from a team as a code owner November 4, 2024 19:21
@ErichDonGubler ErichDonGubler changed the title Idx buf bounds check Move buffer slice OOB check from panic in wgpu to validation error in wgpu-core Nov 4, 2024
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

The validation we are removing (which was recently added in #6432) applies in more cases than RenderPass::set_index_buffer. It also applied to:

  • set_index_buffer on the render bundle encoder
  • set_vertex_buffer on the render pass encoder & render bundle encoder
  • map_async on the buffer

Looking at the spec as well, having it in these places seems correct.

@Wumpf
Copy link
Member

Wumpf commented Nov 6, 2024

To build upon that: slice specifically is a rust artifact and the validation done there is a rusty one just like slice on a vec validates.
Validating to consume a given BufferSlice in wgpu-core is another story.

On that note: the fields of BufferSlice should probably be just pub (which then again very much puts the spot light on the validation on wgpu-core which is needed for WebGPU compliance anyways). I remember another ticket somewhere that was struggling with this. BufferSlice itself is also just a rust artifact - in the WebGPU api this is handled with loose size & offset fields

@ErichDonGubler
Copy link
Member Author

@teoxoy: Ah, right, I need to ensure that there's coverage for those methods, too. Will do that.

  • Do it

@ErichDonGubler
Copy link
Member Author

@Wumpf (CC @teoxoy): This is actually an interesting tension between typical Rust programming and the experience dictated by the WebGPU spec. The spec. specifies that this sort of error is a validation error; so, still safe, but we don't get a panic at the site of usage. I think this is for wgpu devs. to decide on, and honestly...maybe I should just punt the removal of the panicking for another issue/PR.

Exercised by
`webgpu:api,validation,encoding,cmds,render,setIndexBuffer:offset_and_size_oob:*`,
which was disovered to be failing in Firefox in [this `webgpu-backlog1`
run on a Windows debug build in
`try:bed05e732fb557b6173872c508612399b6bc365e`](https://treeherder.mozilla.org/jobs?repo=try&tier=1%2C2%2C3&author=egubler%40mozilla.com&selectedTaskRun=enQluY6MT36HfWG8-NnjAw.0).
This does not, in fact, remove any bounds checks in practice. It is now
a validation error, implemented by the previous commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: validation Issues related to validation, diagnostics, and error handling type: bug Something isn't working
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants