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

[Andres Gomez]

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.

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.

Last, this patch is solving the "regression" that was caused by Commit
f1293b2f9bc3.

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

-- 
Br,

Andres