[Mesa-dev] [PATCH] glsl/linker: produce error when invalid explicit locations are used

[Iago Toral]

Thanks for the review Nicolai! However, I sent a v2 yesterday that
handles some cases better:

https://lists.freedesktop.org/archives/mesa-dev/2017-October/173062.htm
l

Specifically, this v2 computes the location slot for patch variables
correctly and fixes a couple of regressions in piglit that I had missed
when I sent the v1. Does your review stand for the v2?

Iago


On Tue, 2017-10-17 at 21:22 +0200, Nicolai Hähnle wrote:
> On 16.10.2017 14:06, Iago Toral Quiroga wrote:
> > We only need to add a check to validate output locations here. For
> > inputs with invalid locations we will fail to link when we can't
> > find a matching output in the same (invalid) location.
> > 
> > Fixes:
> > KHR-GL45.enhanced_layouts.varying_location_limit
> 
> Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>
> 
> 
> > ---
> >   src/compiler/glsl/link_varyings.cpp | 12 +++++++++++-
> >   src/compiler/glsl/link_varyings.h   |  3 ++-
> >   src/compiler/glsl/linker.cpp        |  2 +-
> >   3 files changed, 14 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/compiler/glsl/link_varyings.cpp
> > b/src/compiler/glsl/link_varyings.cpp
> > index 29842ecacd..bdb70edf47 100644
> > --- a/src/compiler/glsl/link_varyings.cpp
> > +++ b/src/compiler/glsl/link_varyings.cpp
> > @@ -381,7 +381,8 @@ cross_validate_front_and_back_color(struct
> > gl_shader_program *prog,
> >    * Validate that outputs from one stage match inputs of another
> >    */
> >   void
> > -cross_validate_outputs_to_inputs(struct gl_shader_program *prog,
> > +cross_validate_outputs_to_inputs(struct gl_context *ctx,
> > +                                 struct gl_shader_program *prog,
> >                                    gl_linked_shader *producer,
> >                                    gl_linked_shader *consumer)
> >   {
> > @@ -410,6 +411,15 @@ cross_validate_outputs_to_inputs(struct
> > gl_shader_program *prog,
> >            unsigned slot_limit = idx + num_elements;
> >            unsigned last_comp;
> >   
> > +         unsigned slot_max =
> > +            ctx->Const.Program[producer-
> > >Stage].MaxOutputComponents / 4;
> > +         if (slot_limit > slot_max) {
> > +            linker_error(prog,
> > +                         "Invalid location %u in %s shader\n",
> > +                         idx,
> > _mesa_shader_stage_to_string(producer->Stage));
> > +            return;
> > +         }
> > +
> >            if (type->without_array()->is_record()) {
> >               /* The component qualifier can't be used on structs
> > so just treat
> >                * all component slots as used.
> > diff --git a/src/compiler/glsl/link_varyings.h
> > b/src/compiler/glsl/link_varyings.h
> > index 4e1f6d2e42..081b04ea38 100644
> > --- a/src/compiler/glsl/link_varyings.h
> > +++ b/src/compiler/glsl/link_varyings.h
> > @@ -300,7 +300,8 @@ link_varyings(struct gl_shader_program *prog,
> > unsigned first, unsigned last,
> >                 struct gl_context *ctx, void *mem_ctx);
> >   
> >   void
> > -cross_validate_outputs_to_inputs(struct gl_shader_program *prog,
> > +cross_validate_outputs_to_inputs(struct gl_context *ctx,
> > +                                 struct gl_shader_program *prog,
> >                                    gl_linked_shader *producer,
> >                                    gl_linked_shader *consumer);
> >   
> > diff --git a/src/compiler/glsl/linker.cpp
> > b/src/compiler/glsl/linker.cpp
> > index 03eb05bf63..3798309678 100644
> > --- a/src/compiler/glsl/linker.cpp
> > +++ b/src/compiler/glsl/linker.cpp
> > @@ -4929,7 +4929,7 @@ link_shaders(struct gl_context *ctx, struct
> > gl_shader_program *prog)
> >         if (!prog->data->LinkStatus)
> >            goto done;
> >   
> > -      cross_validate_outputs_to_inputs(prog,
> > +      cross_validate_outputs_to_inputs(ctx, prog,
> >                                          prog-
> > >_LinkedShaders[prev],
> >                                          prog->_LinkedShaders[i]);
> >         if (!prog->data->LinkStatus)
> > 
> 
>