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

[Andres Gomez]

On Sun, 2017-03-12 at 16:34 +1100, Timothy Arceri wrote:
> 
> 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.

Talking by memory is a big mistake. I've been misleading you, although
not much. I'll try to explain the whole case and I will answer
afterwards your in-line comments. Sorry about any previous noise.

The most important difference is that the SIGSEV is not happening
inside glGetActiveUniformBlockiv. This API behaves (IMHO), correctly
returning an error:

Mesa: User error: GL_INVALID_VALUE in glGetActiveUniformBlockiv(bufferindex 0)


The crash happens 7 lines below, when calling glUniformBlockBinding.

The exact place where it crashes is in the _mesa_UniformBlockBinding
function, at:
https://cgit.freedesktop.org/mesa/mesa/tree/src/mesa/main/uniforms.c#n1040

The crash shouldn't happen because some lines above we check that the
requested uniform block index is valid.

So, briefing, glGetActiveUniformBlockiv doesn't crash. When passed a
uniform block index out of range it returns GL_INVALID_VALUE.

glUniformBlockBinding doesn't crash either, when testing the API
directly. If you pass an uniform block index out of range it will
return with something like:
Mesa: User error: GL_INVALID_VALUE in glUniformBlockBinding(block index 1 >= 1)


So, what is this series for and what I was trying to test/fix?

As explained in the cover letter of this series, there are two
scenarios related with UBOs and the binding qualifier currently
misbehaving. The first one is a program like:

   "# VS

    layout(binding = 1) Block {
      vec4 color;
    } uni_block1;

    ...

    # FS

    layout(binding = 2) Block {
      vec4 color;
    } uni_block2;

    ..."

A program like this should fail on linking. This scenario is fixed by
the patch 2 of this series and a set of tests for this and other
similar cases was introduced by:
https://cgit.freedesktop.org/piglit/commit/?id=5c296c728edcf259b5ba4d8a68e91d1afcd8f171

Co-laterally, a program like this is showing the SIGSEV behavior that
is dealt with in the patch 1 of the series.

The reason for this is because, when failing at linking stage, we end
with an internal state that is inconsistent between the
NumUniformBlocks variable and the UniformBlocks array size.

This inconsistency happens only after Commit f1293b2f9bc3 landed in
master and, AFAIS, it can only be triggered with a liking failed
program like the above: when the binding layout qualifier is
inconsistent between 2 different compilation units.

Patch 3 of this series can be left out of the explanation for this
specific problem.

> 
> > 
> > > 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 ;-)

Let's replace glGetActiveUniformBlockiv with glUniformBlockBinding.

As I was explaining before, testing that won't trigger any crash. If
you test on a successfully linked program with an UBO index out of
range, it will return an error accordingly. This is tested at:
https://cgit.freedesktop.org/piglit/tree/tests/spec/arb_uniform_buffer_object/uniformblockbinding.c

If you test on a linking failed program it will return an error
accordingly. This is not tested explicitly but I think trying to test
this is an overkill (we can generate a large amount of link failed
programs because of many different reasons).

Additionally, the SIGSEV is not happening just because of using the API
against a link failed program. If we test a link failed program with an
active UBO index out of range it always behaves properly, returning
GL_INVALID_VALUE. The problem is because trying a "claimed by
glGetProgramiv" active UBO index value in range in a link failed
program.

The only scenario in which it will SIGSEV is when the linking failure
happens with a program like the described above because the internally
stored value of the active UBOs is inconsistent with the size of the
array of the active UBOs. Requesting the active UBOs with
glGetProgramiv returns this inconsistent value. Afterwards,
glGetActiveUniformBlockiv returns GL_INVALID_VALUE and
glUniformBlockBinding crashes.

The most straight forward and easiest solution from my POV is to fix
that inconsistency that shouldn't ever happen in the first place.

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

I think I already explained this above. The proposed solution, from my
POV, is correct and it doesn't hide the problem, it fixes it.

Also, since the spec doesn't take a stand, setting the internal value
to 0 in consistency with the internally stored array of active UBOs is
the best way to go, IMHO.

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

As explained above, GL_INVALID_VALUE is the error used already now. I
think that's the behavior we want and the one we will have with this
patch.

I hope I have been able to clarify this matter now.

-- 
Br,

Andres