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

[Timothy Arceri]

On 21/10/17 00:35, Ilia Mirkin wrote:
> 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.

Are you sure this is really required?


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

As I keep pointing out your patch doesn't handle component aliasing. 
Dropping the [4] would mean we not longer check for this.


> 
> Cheers,
> 
>    -ilia
>