[Mesa-dev] [PATCH 1/3] glsl: on UBO/SSBOs link error, the number of active blocks remains 0

[Timothy Arceri]

On 12/03/17 00:29, Andres Gomez wrote:
> On Sat, 2017-03-11 at 23:24 +1100, Timothy Arceri wrote:
>>
>> On 10/03/17 18:53, Andres Gomez wrote:
>>>
>>> According with that text it would be OK to report a different number
>>> than 0 when asking for the active UBOs to the failed link program but
>>> it still will be not OK that, when trying to use that active UBOs, we
>>> would get a SIGSEV.
>>
>> That is is because of an application bug (not checking that link was
>> successful) not a Mesa bug.
>
> I disagree, the one crashing is Mesa.

Then I'll repeat my initial question. Where exactly does it crash? What 
OpenGL call? Is it glGetActiveUniformBlockiv? If so that looks like a bug.

If we want to fix this bug we should really be adding a piglit test to 
go with it.

The sensible thing to do would be to NULL check the uniform block array 
in this function.

>
>>>>>
>>> In other words, I think this is still a bug. The solution could be
>>> different but, with current implementation, the most straight forward
>>> is the patch I provided, setting the active UBOs to 0, which is
>>> coherent with the rest of the internal state saved for the program.
>>
>> IMO you are trying to fix something the spec doesn't say needs fixing. I
>> would be surprised if this was the only value that is reported as non
>> zero on failure. If we really wanted to reset things to zero on a link
>> failure we should have a helper that does it for all values on a link
>> failure, not one off cases like this.
>>
>>>
>>> Last, this patch is solving the "regression" that was caused by Commit
>>> f1293b2f9bc3.
>>
>> This is not a regression, it just happens to now return the value that
>> it would be if the link didn't fail, which as far as I can tell is
>> allowed by the spec.
>
> Again, the regression is the crash, not returning 0 or another value.
>
> There is another possibility of fix which would be to actually set the
> proper array of active uniforms so it wouldn't crash when trying to use
> them.
>
> As I commented before, setting to 0 is the most straight forward
> solution and, as you also say, the spec doesn't take a stand on that.
>
> Therefore, why wouldn't we actually do this change and avoid that
> crash?
>