Skip to content

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.

@ErichDonGubler ErichDonGubler marked this pull request as draft November 7, 2024 13:24
@ErichDonGubler ErichDonGubler force-pushed the idx-buf-bounds-check branch 2 times, most recently from 2baee80 to e45e770 Compare November 12, 2024 13:57
@ErichDonGubler ErichDonGubler force-pushed the idx-buf-bounds-check branch 2 times, most recently from fe2a15b to 5fc583d Compare December 17, 2024 19:52
@ErichDonGubler ErichDonGubler force-pushed the idx-buf-bounds-check branch 2 times, most recently from a8572a5 to b66eb2a Compare January 28, 2025 15:47
@ErichDonGubler ErichDonGubler force-pushed the idx-buf-bounds-check branch 2 times, most recently from febc8f8 to d1c33cc Compare February 28, 2025 03:21
@ErichDonGubler ErichDonGubler force-pushed the idx-buf-bounds-check branch 2 times, most recently from 9194281 to 2681370 Compare April 18, 2025 12:47
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.
@ErichDonGubler
Copy link
Member Author

Made obsolete by #7911 and its ilk.

@github-project-automation github-project-automation bot moved this from In Progress to Done in WebGPU for Firefox Nov 25, 2025
@ErichDonGubler ErichDonGubler deleted the idx-buf-bounds-check branch November 25, 2025 01:54
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: Done

Development

Successfully merging this pull request may close these issues.

3 participants