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

hal: limit binding sizes to i32 #2363

Merged
merged 1 commit into from
Jan 6, 2022
Merged

hal: limit binding sizes to i32 #2363

merged 1 commit into from
Jan 6, 2022

Conversation

kvark
Copy link
Member

@kvark kvark commented Jan 6, 2022

Connections
Closes #2361

Description
Driver compiler toolchain produce an offset as i32 internally, we can't expose bindings larger than that until we start doing 64-bit indexing.

Testing
Untested

@kvark kvark added the PR: needs back-porting PR with a fix that needs to land on crates label Jan 6, 2022
@kvark kvark enabled auto-merge (rebase) January 6, 2022 17:12
@kvark kvark merged commit 4bfa272 into gfx-rs:master Jan 6, 2022
/// Interestingly, the index itself can't reach that high, because the minimum
/// element size is 4 bytes, but the compiler toolchain still computes the
/// offset at some intermediate point, internally, as i32.
pub const MAX_I32_BINDING_SIZE: u32 = 1 << 31;
Copy link
Contributor

Choose a reason for hiding this comment

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

I may miss something, but 1 << 31 does not fit in an i32, as the maximum value for it is (1 << 31) - 1.

I think that using i32::MAX as u32 instead could be less error-prone.

Copy link
Member Author

Choose a reason for hiding this comment

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

The size doesn't need to fit into i32. It's the byte offsets that are internally converted into i32, and they are strictly less than the full binding size.

@kvark
Copy link
Member Author

kvark commented Jan 10, 2022

published in wgpu-hal-0.12.2

@kvark kvark removed the PR: needs back-porting PR with a fix that needs to land on crates label Jan 10, 2022
@kvark kvark added PR: needs back-porting PR with a fix that needs to land on crates and removed PR: needs back-porting PR with a fix that needs to land on crates labels Jan 21, 2022
cwfitzgerald pushed a commit that referenced this pull request Oct 25, 2023
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.

Incorrect max_storage_buffer_binding_size with silent failure
2 participants