Do not neglect to apply bounds checks to indexing operations on
runtime-sized arrays, even when they are accessed via an `AccessIndex`
instruction.
Before this commit, `BlockContext::write_expression_pointer` would not
apply bounds checks to `OpAccessChain` indices provided by an
`AccessIndex` instruction, apparently with the rationale that any
out-of-bounds accesses should have been reported by constant
evaluation.
While it is true that the `index` operand of an `AccessIndex`
expression is known at compile time, and that the WGSL constant
evaluation rules require accesses that can be statically determined to
be out-of-bounds to be shader creation or pipeline creation time
errors, accesses to runtime-sized arrays don't follow this pattern:
even if the index is known, the length with which it must be compared
is not.
Fixes#4441.
Let `BoundsCheckResult::Conditional` provide both the condition to
check before carrying out the access, and the index to use for that
access. The `Conditional` variant indicates that we generated a
runtime bounds check, which implies we must have had a SPIR-V id for
the index to pass to that check, so there's no reason not to provide
that to the callers - especially if the bounds check code was able to
reduce it to a known constant.
At the moment, this is not much of a refactor, but later commits will
use `GuardedIndex` in more places, at which point this will avoid a
re-matching and assertion.
Abstract the code from `write_expression_pointer` to handle one
indexing operation out into its own function,
`BlockContext::write_access_chain_index`.
Let the SPIR-V backend use `GuardedIndex::try_resolve_to_constant`,
rather than writing out its definition in `write_restricted_index` and
`write_index_comparison`.
Call `try_resolve_to_constant` in one place, in `write_bounds_check`,
and simply pass the `GuardedIndex` into subroutines.
Reduce `write_restricted_index` and `write_index_comparison` to case
analysis and code generation.
Note that this commit does have a benign effect on SPIR-V snapshot
output for programs like this:
let one_i = 1i;
var vec0 = vec3<i32>();
vec0[one_i] = 1;
The value indexing `vec0` here is an `i32`, but after this commit, the
operand to `OpAccessChain` becomes a `u32` constant (with the same
value).
This is because `write_bounds_check` now calls
`try_resolve_to_constant` itself, rather than deferring this work to
its callees, so it may return `BoundsCheckResult::KnownInBounds` even
when the `Unchecked` policy is in force. This directs the caller,
`write_expression_pointer`, to treat the `OpAccessChain` operand as a
fresh `u32` constant, rather than simply passing through the original
`i32` expression.
Introduce a new function,
`BlockContext::extend_bounds_check_condition_chain`, which adds a new
boolean condition to the chain of bounds checks guarding an
`OpAccessChain` instruction.
Introduce a new function,
`BlockContext::is_nonuniform_binding_array_access`, which determines
whether a given array access expression means that the `OpAccessChain`
instruction must have a `NonUniform` decoration.
Roll back `wgpu`'s dependencies on `once_cell` from 1.20.1 to 1.19.0.
Version 1.20.1 of `once_cell` added a more complex conditional
dependency on `portable-atomic`, which causes `cargo metadata` to
incorrectly list `portable-atomic` as a dependency even though the
given `once_cell` features are not enabled.
The Firefox source tree uses `cargo vet` to enforce supply-chain
auditing. Since `cargo vet` depends on `cargo metadata` to tell it
what crates are going to be included in the tree, the extraneous
dependency above adds `portable-atomic` to the set of sources we must
audit. Since `portable-atomic` is roughly 50kloc, we would like to
avoid this.
Nothing in `wgpu` actually needs `once_cell` 1.20; it was upgraded by
Dependabot. So the simplest workaround for the moment is to roll back
the version.
When generating SPIR-V, avoid generating unreachable blocks following
statements like `break`, `return`, and so on that cause non-local
exits. These unreachable blocks can cause SPIR-V validation to fail.
Fixes#6220.
Make `naga:🔙:spv::BlockContext::write_block` private to
`naga:🔙:spv::block`. Introduce a new `pub(super)` function,
`write_function_body`, for `Writer::write_function` to call.
Make `BlockExit` private to `naga:🔙:spv::block`.
Move `LoopContext` from `naga:🔙:spv` into
`naga:🔙:spv::block`, and make it private.
Consolidate the explanation of why and how we wrap Naga global
variables in structs to satisfy Vulkan's requirements, and include it
in the documentation for `back::spv::GlobalVariable`.
Clarify `GlobalVariable`'s members documentation.
Bring https://github.com/gfx-rs/naga/pull/723 back from the dead.
Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
Co-authored-by: Jim Blandy <jimb@red-bean.com>
Document and refactor
`naga:🔙:spv::BlockContext::write_runtime_array_length`.
Don't try to handle finding the length of a particular element of a
`binding_array<array<T>>`. The SPIR-V backend doesn't wrap that type
correctly anyway; #6333 changes the validator to forbid such types.
Instead, assume that the elements of a `binding_array<T>` are always
structs whose final members may be a runtime-sized array.
Pull out consistency checks after the analysis of the array
expression, so that we always carry out all the checks regardless of
what path we took to produce the information.
Require `T` to be a struct in `binding_array<T, ...>`; do not permit
arrays.
In #5428, the validator was changed to accept binding array types that
the SPIR-V backend couldn't properly emit. Specifically, the validator
was changed to accept `binding_array<array<T>>`, but the SPIR-V
backend wasn't changed to wrap the binding array elements in a SPIR-V
struct type, as Vulkan requires. So the type would be accepted by the
validator, and then rejected by the backend.
This change uses `include_wgsl!(…)` in usages where an
`include_str!("….wgsl")` was used to construct
a `ShaderModuleDescriptor`'s `source, but the `label` was set to `None`.
This should (1) showcase a nice idiomatic convenience we offer in our
examples better, (2) make code more concise, and (3) get some
automatically generated labels in diagnostics where it seems it won't
hurt.
`Validator::validate_module_handles` already ensures that types refer
only to other types appearing earlier in the arena than themselves, so
this check in `Validator::validate_type` is redundant.
* Change the signature of `wgpu_core::Global::queue_submit` to return
a `(SubmissionIndex, …)` in addition to its current error type.
* Change the control flow of errors in `Queue::submit` to break to the
end of a block. This is similar to what we already do in many APIs in
`wgpu_core`.
* Hoist the scope of the local `submit_index` binding so it can be used
at the point where we need to convert current error paths to also
return the submission index.
Later, we will likely want to avoid actually retrieving a new submission
index so we can minimize the critical section of code. We'll need to
figure out a strategy for returning a valid (but not necessarily unique)
index in the case of failures that prevent successful submission.
