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

[Jason Ekstrand]

On Mon, Sep 11, 2017 at 1:42 AM, Samuel Iglesias Gonsálvez <
siglesias at igalia.com> wrote:

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

Did a little looking at this test in particular and it's definitely bogus.


> A snippet of its SPIR-V is:
>
> [...]
> OpDecorate %_ptr_buffer_Input ArrayStride 65536
> [...]
> %Input = OpTypeStruct %_arr__arr_mat4v4float_uint_32_uint_32
>

This is a struct containing an array of arrays.  The struct is decorated
with Block which is reasonable


> %_ptr_buffer_Input = OpTypePointer StorageBuffer %Input
>

This is a pointer to that struct.  In the full SPIR-V, it's decorated with
an array stride which makes no sense whatsoever because you can't just jump
from one block to the next with a stride.  It's possible that the test
author intended to have the block contain an implicit array of structs but
if they wanted that then they should have done a struct of array of struct
of array of array of matrices.


> %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*.
>
>
That is the rule that *should* be catching this but they got around it by
slapping a bogus ArrayStride decoration onto it.  I'm not really sure what
SPIR-V rule is supposed to catch this.  One of the design flaws of SPIR-V
is that we have valid usage rules based on decorations like you quoted
above but we have no way of saying that
%_arr__arr_mat4v4float_uint_32_uint_32 can't have an ArrayStride
decoration.  In fact, it can have such a decoration and might have to if
it's also embedded in an array somewhere.  I'll file SPIR-V and CTS bugs.
I really wish we had caught this before the test got merged...

--Jason


>
>    -
>
>
> 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
> <https://www.khronos.org/registry/spir-v/specs/1.2/SPIRV.html#OpPtrAccessChain>
>
>
> Reordering that code fixes crashes in:
>>
>>    dEQP-VK.spirv_assembly.instruction.*.indexing.*
>>
>> Signed-off-by: Samuel Iglesias Gonsálvez <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
>> 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/2fd9ef81/attachment.html>