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

[Timothy Arceri]

On 12/03/17 12:17, Andres Gomez wrote:
> On Sun, 2017-03-12 at 10:07 +1100, Timothy Arceri wrote:
>>
>> 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.
>
> Yes, glGetActiveUniformBlockiv, as I answered before:
>
> "And if there is any, we make use of them:
> https://cgit.freedesktop.org/piglit/tree/tests/shaders/shader_runner.c#n2662
>
> That's when the SIGSEV happens."

Pointing me to shader runner is not very helpful for explaining where 
exactly in Mesa the segfault is happening.

>
>> If we want to fix this bug we should really be adding a piglit test to
>> go with it.
>
> I recently added a test for that:
> https://cgit.freedesktop.org/piglit/tree/tests/spec/arb_shading_language_420pack/linker/different-bindings-uniform-blocks-instanced.shader_test

This is not a test for the segfault. It depends on a bug in shader 
runner (not checking the link status), if that is fixed we would no 
longer be testing the problem.

>
> That test will SIGSEV because of the described reason once the patch 3
> in this series lands, which fixes the actual problem for which that
> test was created:
> https://patchwork.freedesktop.org/patch/140481/
>
> AFAIK, there is no other way of triggering this crash.

You need to write a test that uses that api rather than using shader 
runner. If I understand correctly all you need to do is call 
glGetActiveUniformBlockiv() on any shader which doesn't link as in your 
scenario above. It may even crash on a shader that does compile 
correctly but contains not uniform blocks, or even on a shaders when the 
uniform blocks are optimised away but the app still tries to use the 
function.

In other words passing in an index to glGetActiveUniformBlockiv() when 
there are no uniformblocks would probably cause a crash, although I 
can't be sure because you haven't told me where it's crashing yet ;-)

>
>> The sensible thing to do would be to NULL check the uniform block array
>> in this function.
>
> I think the best solution is what it is understood already now in the
> code, which is that the index of active uniform blocks is always
> coherent with the block array size.
>
> Setting the index to 0, which is coherent and doesn't make any harm, is
> the easiest and most straight forward solution.


But you are just working around the problem and again the spec doesn't 
say it must be 0. If the app passes an index to 
glGetActiveUniformBlockiv() when there are no uniform blocks it will 
still crash, this patch doesn't really fix the problem it just hides the 
instance you are seeing.

>
> Anyway, we can NULL check but, then, I don't know exactly which is the
> behavior that you expect from the glGetActiveUniformBlockiv call (?).
>

I can't find a good refenerce in the 4.5 spec so you might need to do 
some digging yourself, but IMO this is something that should be defined 
even if we must file a bug report. The docs on the other hand say: 
"GL_INVALID_VALUE is generated if uniformBlockIndex is greater than or 
equal to the value of GL_ACTIVE_UNIFORM_BLOCKS or is not the index of an 
active uniform block in program."

"is not the index of an active uniform block in program" being the 
relevant part.