[Mesa-dev] [PATCH 3/3] glsl/linker: validate explicit locations for SSO programs

[Ilia Mirkin]

On Fri, Oct 20, 2017 at 6:46 AM, Iago Toral Quiroga <itoral at igalia.com> wrote:
> ---
>  src/compiler/glsl/link_varyings.cpp | 53 +++++++++++++++++++++++++++++++++++++
>  src/compiler/glsl/link_varyings.h   |  4 +++
>  src/compiler/glsl/linker.cpp        |  6 +++++
>  3 files changed, 63 insertions(+)
>
> diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
> index dffb4f98df..d7e407184d 100644
> --- a/src/compiler/glsl/link_varyings.cpp
> +++ b/src/compiler/glsl/link_varyings.cpp
> @@ -599,6 +599,59 @@ validate_explicit_variable_location(struct gl_context *ctx,
>  }
>
>  /**
> + * Validate explicit locations for SSO programs. For non-SSO programs this
> + * is alreadty done in cross_validate_outputs_to_inputs().
> + */
> +void
> +validate_sso_explicit_locations(struct gl_context *ctx,
> +                                struct gl_shader_program *prog)
> +{
> +   assert(prog->SeparateShader);
> +   struct explicit_location_info explicit_locations_in[MAX_VARYINGS_INCL_PATCH][4];
> +   struct explicit_location_info explicit_locations_out[MAX_VARYINGS_INCL_PATCH][4];

These comments may point to issues in earlier patches / overall logic,
but I'll still make them here. I don't have time to do a full review
of all the patches, unfortunately. Thanks for addressing my concerns
with your earlier patchset.

This should be MAX_VARYINGS. The patch varyings are meant to be
aliased against the non-patch varyings, and their indices must be
assigned as such, otherwise you won't get the overlaps you're supposed
to.

Furthermore, I haven't checked how your code works, but I found it was
easier to not have the [4]. None of the checks need to be
per-component (and in fact, you're supposed to raise an error when
components disagree on type-related things, except for VS inputs which
you skip here anyways). Just store the first value in the info, and
then if anything comes in counter to that, return an error. You can
see what I did in https://patchwork.freedesktop.org/patch/175959/. I
spent quite some time to make sure that the patch was correct, so if
you see any functional differences to what you've done, I'd recommend
considering why there are differences -- there shouldn't be any.

Cheers,

  -ilia