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

[Timothy Arceri]

On 10/03/17 18:53, Andres Gomez wrote:
> On Fri, 2017-03-10 at 14:28 +1100, Timothy Arceri wrote:
>>
>> On 10/03/17 08:46, Andres Gomez wrote:
>>> On Fri, 2017-03-10 at 08:32 +1100, Timothy Arceri wrote:
>>>> On 23/02/17 19:55, Andres Gomez wrote:
>>>>> Commit f1293b2f9bc3 split apart buffer block arrays but introduced
>>>>> also the possibility of a recount of active
>>>>> blocks (NumUniformBlocks/NumShaderStorageBlocks) which would be
>>>>> incoherent with the actual amount of active
>>>>> blocks (UniformBlocks/ShaderStorageBlocks) in the program.
>>>>>
>>>>> This could cause a segmentation fault if trying to use the index of a
>>>>> block in a link failed program.
>>>>
>>>> Where exactly does this segfault happen?
>>>> interstage_cross_validate_uniform_blocks() should exit linking because
>>>> we returned false.
>>>
>>> It does exit, the segfault is not happening when running this code but
>>> when using the link failed program later, as commented.
>>>
>>> I caught this by using piglit's shader runner. In the "init_test"
>>> function the last action is to call "setup_ubos":
>>> https://cgit.freedesktop.org/piglit/tree/tests/shaders/shader_runner.c#n3704
>>>
>>> This is done regardless the link status gotten previously. At
>>> "setup_ubos" we ask first for the active UBOs:
>>> https://cgit.freedesktop.org/piglit/tree/tests/shaders/shader_runner.c#n2652
>>>
>>> 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.
>>>
>>> Of course, you could argue what are you doing using a link failed
>>> program ☺ but the fact is that it should not return a value for the
>>> active UBOs other than 0 and, most importantly, it shouldn't crash.
>>>
>>> I hope this clarifies your doubt.
>>>
>>
>> I'm not sure this is a bug. The section 7.3. (PROGRAM OBJECTS) of the
>> OpenGL 4.5 spec says:
>>
>> "If a program is  not linked successfully, the link may have failed for
>> a number of reasons, including cases where the program required more
>> resources than supported by the implementation. Implementations are
>> permitted, but not required, to record lists of resources that would
>> have been considered active had the program linked successfully."
>
> 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.

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

>
> I set you to review because you are the author of that commit.
>