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

[Andres Gomez]

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) {
> >            linker_error(prog, "xfb_offset (%d) overflows xfb_stride (%d) for "
> >                         "buffer (%d)", xfb_offset * 4,
> >                         info->Buffers[buffer].Stride * 4, buffer);
> > 
-- 
Br,

Andres