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

[core, hal, types] Clarify wgpu_hal's bounds check promises. #6201

Merged
merged 5 commits into from
Sep 3, 2024

Conversation

jimblandy
Copy link
Member

@jimblandy jimblandy commented Sep 2, 2024

  • [core, hal, types] Clarify wgpu_hal's bounds check promises.

    In wgpu_hal:

    • Document that wgpu_hal guarantees that shaders will not access buffer
      contents beyond the bindgroups' bound regions, rounded up to some
      adapter-specific alignment. Introduce the term "accessible region" for
      the portion of the buffer that shaders can actually get at.

    • Document that all bets are off if you disable bounds checks with
      ShaderModuleDescriptor::runtime_checks.

    • Provide this alignment in wgpu_hal::Alignments. Update all backends
      appropriately.

    • In the Vulkan backend, use Naga to inject bounds checks on buffer accesses
      unless robustBufferAccess2 is available; robustBufferAccess is not
      sufficient. Retrieve VK_EXT_robustness2's properties, as needed to discover
      the alignment above.

    In wgpu_core:

    • Use buffer bindings' accessible regions to determine which parts of the buffer
      need to be initialized.

    In wgpu_types:

    • Document some of the possible effects of using
      ShaderBoundsChecks::unchecked.
  • [core]: Let Device::create_buffer_binding get limits from self.

    Rather than passing self.limits to Device::create_buffer_binding
    as an argument, let it simply refer to self.limits itself.

  • [core] Remove unnecessary pub(crate) from Device methods.

    Remove the pub(crate) visibility marking from various associated
    functions of Device that are defined in, and not used outside of,
    the wgpu_core::device::resource module.

  • [core] Simplify self types in device::resource.

    Change various functions that have no need to create an owning
    reference to the Device to accept &self instead of &Arc<Self>.

    Change ParentDevice::same_device to accept &Device as the point of
    comparison, not &Arc<Device>. Call sites will use Deref conversion,
    so no callers need to be changed.

  • [core] Rename map_buffer to resolve_buffer, etc.

    In Device::create_bind_group, name the functions that convert
    resource ids to Arcs resolve_foo, not map_foo:

    • The types that hold Arcs are usually called ResolvedBlah.

    • The name map_buffer is misleading.

Fixes #1813.

Checklist

  • Run cargo fmt.
  • Run cargo clippy.
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@jimblandy jimblandy requested a review from a team as a code owner September 2, 2024 23:44
@jimblandy jimblandy force-pushed the fix-1813 branch 2 times, most recently from 6616d2d to b7d4d93 Compare September 2, 2024 23:56
In `Device::create_bind_group`, name the functions that convert
resource ids to Arcs `resolve_foo`, not `map_foo`:

- The types that hold Arcs are usually called `ResolvedBlah`.

- The name `map_buffer` is misleading.
Change various functions that have no need to create an owning
reference to the `Device` to accept `&self` instead of `&Arc<Self>`.

Change `ParentDevice::same_device` to accept `&Device` as the point of
comparison, not `&Arc<Device>`. Call sites will use Deref conversion,
so no callers need to be changed.
Remove the `pub(crate)` visibility marking from various associated
functions of `Device` that are defined in, and not used outside of,
the `wgpu_core::device::resource` module.
Rather than passing `self.limits` to `Device::create_buffer_binding`
as an argument, let it simply refer to `self.limits` itself.
In `wgpu_hal`:

- Document that `wgpu_hal` guarantees that shaders will not access buffer
  contents beyond the bindgroups' bound regions, rounded up to some
  adapter-specific alignment. Introduce the term "accessible region" for
  the portion of the buffer that shaders can actually get at.

- Document that all bets are off if you disable bounds checks with
  `ShaderModuleDescriptor::runtime_checks`.

- Provide this alignment in `wgpu_hal::Alignments`. Update all backends
  appropriately.

- In the Vulkan backend, use Naga to inject bounds checks on buffer accesses
  unless `robustBufferAccess2` is available; `robustBufferAccess` is not
  sufficient. Retrieve `VK_EXT_robustness2`'s properties, as needed to discover
  the alignment above.

In `wgpu_core`:

- Use buffer bindings' accessible regions to determine which parts of the buffer
  need to be initialized.

In `wgpu_types`:

- Document some of the possible effects of using
  `ShaderBoundsChecks::unchecked`.

Fixes gfx-rs#1813.
Copy link
Contributor

@nical nical left a comment

Choose a reason for hiding this comment

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

Beautiful.

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.

Looks great!

@ErichDonGubler ErichDonGubler merged commit ee35b0e into gfx-rs:trunk Sep 3, 2024
25 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.

Robust Buffer Access conflicts with lazy initialization
4 participants