[Mesa-dev] [PATCH 5/6] glsl/linker: simplify xfb_offset vs xfb_stride overflow check

[Timothy Arceri]

On 3/2/19 7:21 am, Andres Gomez wrote:
> On Sat, 2019-02-02 at 10:10 +1100, Timothy Arceri wrote:
>> On 2/2/19 5:05 am, Andres Gomez wrote:
>>> Current implementation uses a complicated calculation which relies in
>>> an implicit conversion to check the integral part of 2 division
>>> results.
>>>
>>> However, the calculation actually checks that the xfb_offset is
>>> smaller or a multiplier of the xfb_stride. For example, while this is
>>> expected to fail, it actually succeeds:
>>>
>>>     "
>>>
>>>       ...
>>>
>>>       layout(xfb_buffer = 2, xfb_stride = 12) out block3 {
>>>         layout(xfb_offset = 0) vec3 c;
>>>         layout(xfb_offset = 12) vec3 d; // ERROR, requires stride of 24
>>
>> Why does this require a stride of 24?
>>
>> vec3 c uses bytes 0-11. So there is no issue with vec3 d having an
>> offset of 12. Its been a long time since I worked on this but I think
>> this change is wrong. I see no reason this should fail compilation.
> 
> Mmmm ... maybe I should add the reference from the GLSL specs but the
> patch doesn't solve exactly the overflow logic but just a specific
> corner case, that's why I didn't include it in the first place.
> 
> FWIW, the spec in GLSL is not completely clear and you have to read it
> twice and check the examples to understand that the limitation of an
> "xfb_offset" overflowing a "xfb_stride" is not the value declared for
> the "xfb_offset" qualifier but that value plus the size of the variable
> to which the qualifier is applied.
> 
> It is clearer in the OpenGL spec, though:
> 
>  From page 390 (page 412 of the PDF) of the OpenGL 4.6 (Core Profile) spec:
> 
>    " If the set of output variables to record in transform feedback
>      mode is specified using layout qualifiers, a program will fail to
>      link if:"
> 
> ...
> 
>    "   * any binding point has a stride declared using the xfb_stride
>          layout qualifier and the sum of the offset and size of any
>          variable associated with that binding point exceeds the value
>          of this stride;"
> 
> Anyway, let me know if the following chunk is enough and you want me to
> add it into the commit log:
> 
>  From page 76 (page 80 of the PDF) of the GLSL 4.60 v.5 spec:
> 
>    " The xfb_stride qualifier specifies how many bytes are consumed by
>      each captured vertex. It applies to the transform feedback buffer
>      for that declaration, whether it is inherited or explicitly
>      declared. It can be applied to variables, blocks, block members, or
>      just the qualifier out. If the buffer is capturing any outputs with
>      double-precision components, the stride must be a multiple of 8,
>      otherwise it must be a multiple of 4, or a compile-time or
>      link-time error results. It is a compile-time or link-time error
>      to have any xfb_offset that overflows xfb_stride, whether stated
>      on declarations before or after the xfb_stride, or in different
>      compilation units. While xfb_stride can be declared multiple times
>      for the same buffer, it is a compile-time or link-time error to
>      have different values specified for the stride for the same buffer.
> 
>      For example:"
> 
> ...
> 
>    " layout(xfb_buffer = 2, xfb_stride = 32) out block3 {
>        layout(xfb_offset = 12) vec3 c;
>        layout(xfb_offset = 24) vec3 d; // ERROR, requires stride of 36
>        layout(xfb_offset = 0) vec3 g; // okay, increasing order not required
>      };"
> 
> 
>>
>>
>>>       };
>>>
>>>       ...
>>>
>>>     "
>>>
>>> Fixes: 2fab85aaea5 ("glsl: add xfb_stride link time validation")
>>> Cc: Timothy Arceri <tarceri at itsqueeze.com>
>>> Signed-off-by: Andres Gomez <agomez at igalia.com>
>>> ---
>>>    src/compiler/glsl/link_varyings.cpp | 3 +--
>>>    1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
>>> index 6cebc5b3c5a..ab66ceb0d00 100644
>>> --- a/src/compiler/glsl/link_varyings.cpp
>>> +++ b/src/compiler/glsl/link_varyings.cpp
>>> @@ -1213,8 +1213,7 @@ tfeedback_decl::store(struct gl_context *ctx, struct gl_shader_program *prog,
>>>             return false;
>>>          }
>>>    
>>> -      if ((this->offset / 4) / info->Buffers[buffer].Stride !=
>>> -          (xfb_offset - 1) / info->Buffers[buffer].Stride) {
>>> +      if (xfb_offset > info->Buffers[buffer].Stride) {

After reading the spec I think you are right.

Reviewed-by: Timothy Arceri <tarceri at itsqueeze.com>


>>>             linker_error(prog, "xfb_offset (%d) overflows xfb_stride (%d) for "
>>>                          "buffer (%d)", xfb_offset * 4,
>>>                          info->Buffers[buffer].Stride * 4, buffer);
>>>