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

[naga spv-out] bounds-check runtime-sized array access correctly. #6351

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from

Conversation

jimblandy
Copy link
Member

@jimblandy jimblandy commented Oct 1, 2024

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 all 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.

  • [naga] Extend snapshot tests for bounds checks.

    Extend the snapshot tests for bounds checking to cover the case where a runtime-sized array is indexed by a constant.

  • [naga spv-out] Let BoundsCheckResult supply the index value.

    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.

  • [naga spv-out] Abstract out OpAccessChain index production.

    Abstract the code from write_expression_pointer to handle one indexing operation out into its own function, BlockContext::write_access_chain_index.

  • [naga spv-out] Consolidate code to find index values.

    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.

  • [naga spv-out] Abstract extending a bounds check condition chain.

    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.

  • [naga spv-out] Doc fix: typo

  • [naga spv-out] Abstract out non-uniform binding array access test.

    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.

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.
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.
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.
Abstract the code from `write_expression_pointer` to handle one
indexing operation out into its own function,
`BlockContext::write_access_chain_index`.
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.
Extend the snapshot tests for bounds checking to cover the case where
a runtime-sized array is indexed by a constant.
@jimblandy jimblandy added type: bug Something isn't working area: naga back-end Outputs of naga shader conversion naga Shader Translator lang: SPIR-V Vulkan's Shading Language labels Oct 1, 2024
@jimblandy jimblandy requested a review from a team as a code owner October 1, 2024 19:14
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 gfx-rs#4441.
Copy link
Member

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

I got up to (but not including) the [naga spv-out] Consolidate code to find index values. So far, the review experience has been nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: naga back-end Outputs of naga shader conversion lang: SPIR-V Vulkan's Shading Language naga Shader Translator type: bug Something isn't working
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

SPIR-V back end skips bounds checks for AccessIndex when applied to runtime-sized arrays
2 participants