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

Fix unaligned slice::from_raw_parts in gles push contant handling. #6341

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

Imberflur
Copy link
Contributor

@Imberflur Imberflur commented Sep 29, 2024

Connections
Fixes #6327

Description

Fixes calling slice::from_raw_parts with an unaligned pointer by using bytemuck::pod_read_unaligned instead (which copies the value to the stack). I assume this is fine for values of the sizes used in push constants. If it seems useful to try to avoid this, then the push constants need to be stored aligned instead (actually looking at this, it would probably be easy to make CommandBuffer::add_push_constant_data include some padding to align the data).

This adds a new dependency on bytemuck in wgpu-hal. I don't know whether adding this is acceptable (e.g. for firefox).

Testing

I updated to a recent rustc nightly (rustc 1.83.0-nightly (2bd1e894e 2024-09-26)) and used rustup override set nightly followed by cargo xtask test on a linux system where the gles backend is present.

Before the fix I saw panics from new debug assertions in slice::from_raw_parts with these tests:

  • wgpu_test::regression::issue_3349::multi_stage_data_binding
  • wgpu_test::shader::struct_layout::push_constant_input

After the fix these test no longer panic.

Checklist

  • Run cargo fmt.
  • 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.

Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

thank you! Agreed, the copy should be fine for push constants

This adds a new dependency on bytemuck in wgpu-hal. I don't know whether adding this is acceptable (e.g. for firefox).

Should be fine, it's already used in wgpu-core if spv-in is enabled, although that feature is probably not enabled in Firefox?
cc: @ErichDonGubler to confirm that this won't be a problem

Cargo.toml Show resolved Hide resolved
@ErichDonGubler
Copy link
Member

ErichDonGubler commented Oct 3, 2024

@Wumpf: Doesn't seem problematic to me. The biggest obstacle to most new dependencies is the work to vet the soundness of the crate, and bytemuck has been audited by the Chromium team/Google in a way that's shared with Firefox/Mozilla:

https://github.com/google/rust-crate-audits/blob/a0fe5aac7671f88eb32eea18dd0df1c95d0f1312/audits.toml#L1228-L1233

CC @jimblandy, who is slated to handle a re-vendor from WGPU into Firefox next.

@ErichDonGubler ErichDonGubler enabled auto-merge (squash) October 3, 2024 20:24
@ErichDonGubler ErichDonGubler merged commit c5a4b4e into gfx-rs:trunk Oct 3, 2024
27 checks passed
@Imberflur Imberflur deleted the fix-get-data2 branch October 3, 2024 22:25
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.

Missing precondition for slice::from_raw_parts in gles backend
3 participants