[Mesa-dev] [PATCH] nir/spirv: fix crashes when dereferencing a pointer for an OpVariable

[Samuel Iglesias Gonsálvez]

On 09/07/2017 07:15 PM, Jason Ekstrand wrote:
> On Tue, Aug 29, 2017 at 3:04 AM, Samuel Iglesias Gonsálvez
> <siglesias at igalia.com <mailto:siglesias at igalia.com>> wrote:
>
>     When creating a vtn_pointer for an OpVariable, the block_index and
>     offsets fields are null because there is not ssa to take the data
>     from.
>
>     However, we can dereference that pointer when processing an
>     SpvOp*AccessChain opcodes through vtn_ssa_offset_pointer_dereference()
>     when the OpVariable when the StorageClass is Uniform or StorageBuffer.
>
>     Inside vtn_ssa_offset_pointer_dereference() we have the code to
>     initialize block_index and offset if they are null, but it is called
>     after checking if the pointer has then non-null.
>
>
> This seems fishy.  The code you're moving only triggers for
> OpPtrAccessChain.  In order to run into an issue, they would have to
> be doing an OpPtrAccessChain an an array of resources.  This shouldn't
> be allowed by the spec.  I'll have to look at the actual SPIR-V to be
> sure, but I think this is probably a CTS bug.
>  

For example, one of the tests failing is:
dEQP-VK.spirv_assembly.instruction.compute.indexing.opptraccesschain_u32

A snippet of its SPIR-V is:

[...]
OpDecorate %_ptr_buffer_Input ArrayStride 65536
[...]
%Input = OpTypeStruct %_arr__arr_mat4v4float_uint_32_uint_32
%_ptr_buffer_Input = OpTypePointer StorageBuffer %Input
%dataInput = OpVariable %_ptr_buffer_Input StorageBuffer
[...]
%54 = OpPtrAccessChain %_ptr_buffer_float %dataInput %idx_1 %idx_0 %i0
%i1 %i2 %i3

%dataInput points to a struct of an AoA of matrices. However, according
to SPV_KHR_variable_pointers [0]:

  *

    Each *OpPtrAccessChain* must have a /Base/ whose type is decorated
    with *ArrayStride*.

So I think this is fine, or am I understanding it wrongly?

Sam

[0]
https://www.khronos.org/registry/spir-v/extensions/KHR/SPV_KHR_variable_pointers.html

>     Reordering that code fixes crashes in:
>
>        dEQP-VK.spirv_assembly.instruction.*.indexing.*
>
>     Signed-off-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com
>     <mailto:siglesias at igalia.com>>
>     ---
>      src/compiler/spirv/vtn_variables.c | 29 +++++++++++++++--------------
>      1 file changed, 15 insertions(+), 14 deletions(-)
>
>     diff --git a/src/compiler/spirv/vtn_variables.c
>     b/src/compiler/spirv/vtn_variables.c
>     index 4f6acd2e07..baf1edde4c 100644
>     --- a/src/compiler/spirv/vtn_variables.c
>     +++ b/src/compiler/spirv/vtn_variables.c
>     @@ -146,20 +146,6 @@ vtn_ssa_offset_pointer_dereference(struct
>     vtn_builder *b,
>         struct vtn_type *type = base->type;
>
>         unsigned idx = 0;
>     -   if (deref_chain->ptr_as_array) {
>     -      /* We need ptr_type for the stride */
>     -      assert(base->ptr_type);
>     -      /* This must be a pointer to an actual element somewhere */
>     -      assert(block_index && offset);
>     -      /* We need at least one element in the chain */
>     -      assert(deref_chain->length >= 1);
>     -
>     -      nir_ssa_def *elem_offset =
>     -         vtn_access_link_as_ssa(b, deref_chain->link[idx],
>     -                                base->ptr_type->stride);
>     -      offset = nir_iadd(&b->nb, offset, elem_offset);
>     -      idx++;
>     -   }
>
>         if (!block_index) {
>            assert(base->var);
>     @@ -182,6 +168,21 @@ vtn_ssa_offset_pointer_dereference(struct
>     vtn_builder *b,
>         }
>         assert(offset);
>
>     +   if (deref_chain->ptr_as_array) {
>     +      /* We need ptr_type for the stride */
>     +      assert(base->ptr_type);
>     +      /* This must be a pointer to an actual element somewhere */
>     +      assert(block_index && offset);
>     +      /* We need at least one element in the chain */
>     +      assert(deref_chain->length >= 1);
>     +
>     +      nir_ssa_def *elem_offset =
>     +         vtn_access_link_as_ssa(b, deref_chain->link[idx],
>     +                                base->ptr_type->stride);
>     +      offset = nir_iadd(&b->nb, offset, elem_offset);
>     +      idx++;
>     +   }
>     +
>         for (; idx < deref_chain->length; idx++) {
>            switch (glsl_get_base_type(type->type)) {
>            case GLSL_TYPE_UINT:
>     --
>     2.14.1
>
>     _______________________________________________
>     mesa-dev mailing list
>     mesa-dev at lists.freedesktop.org <mailto:mesa-dev at lists.freedesktop.org>
>     https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>     <https://lists.freedesktop.org/mailman/listinfo/mesa-dev>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170911/cd6504a5/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170911/cd6504a5/attachment-0001.sig>