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

[Ilia Mirkin]

On Fri, Oct 20, 2017 at 12:26 PM, Timothy Arceri <tarceri at itsqueeze.com> wrote:
>
>
> On 21/10/17 03:00, Ilia Mirkin wrote:
>>
>> On Fri, Oct 20, 2017 at 11:55 AM, Timothy Arceri <tarceri at itsqueeze.com>
>> wrote:
>>>
>>>
>>>
>>> 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?
>>
>>
>> https://github.com/KhronosGroup/OpenGL-API/issues/13
>>
>>>> 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.
>>
>>
>> I'm 50% sure that something else checks that already.
>
>
> This is where it's done. Without it you won't catch:
>
>> layout (location=0, component=0) int a;
>> layout (location=0, component=0) int b;

OK, well it seems like a fundamentally different check. Either way,
both cases have to be accommodated. If this is the only scan through
the inputs/outputs, then I guess it's OK to keep the components
separate and then have a pass afterwards which compares the types
across the components and makes sure that they're compatible.

  -ilia