Bug 29540

The new piglit test glsl-getattriblocation test returns attrib_loc=0 for the old compiler and attrib_loc=1 for the new compiler.

This causes the glDrawArrays/Elements() validation test to fail at api_validate.c:124 so nothing is drawn.

There's two issues here.

1. By returning attrib_loc=1 instead of 0 will we have one less user-defined vertex attribute available to users with the new compiler?  The query of GL_MAX_VERTEX_ATTRIBS_ARB still returns 16.

2. Mesa's glDrawArrays/Elements() validation check is incorrect.  I've added two other piglit tests (glsl-bindattriblocation and glsl-novertexdata) that test this logic.  They pass with NVIDIA's driver but fail with Mesa.  The attached patch, however, fixes the problem for Mesa (for the old drivers at least but it doesn't work with gallium yet).

Created attachment 37848 [details] [review]
Patch to fix glDrawArrays/Elements validation

If there's not objections to this, I'll commit later.

(In reply to comment #1)

> 1. By returning attrib_loc=1 instead of 0 will we have one less user-defined
> vertex attribute available to users with the new compiler?  The query of
> GL_MAX_VERTEX_ATTRIBS_ARB still returns 16.

Attribute 0 is special.  It is bound to gl_Vertex by default.  Applications can specifically bind to 0 using glBindAttribLocation, but the linker doesn't automatically bind to it.  It's possible that the spec says we should bind something to 0 if gl_Vertex isn't used, which is the case in this test.

(In reply to comment #2)
> Created an attachment (id=37848) [details]
> Patch to fix glDrawArrays/Elements validation
> 
> If there's not objections to this, I'll commit later.

At least until OpenGL 3.2, IIRC, attribute 0 is still required for drawing to happen.

(In reply to comment #3)
> (In reply to comment #1)
> 
> > 1. By returning attrib_loc=1 instead of 0 will we have one less user-defined
> > vertex attribute available to users with the new compiler?  The query of
> > GL_MAX_VERTEX_ATTRIBS_ARB still returns 16.
> 
> Attribute 0 is special.  It is bound to gl_Vertex by default.  Applications can
> specifically bind to 0 using glBindAttribLocation, but the linker doesn't
> automatically bind to it.  It's possible that the spec says we should bind
> something to 0 if gl_Vertex isn't used, which is the case in this test.

Calling glVertexAttrib*(index=0, value) is equivalent to glVertex*(value) but I don't think it's the same with arrays.  That is, I don't think that generic attribute array 0 is the same as the conventional GL_VERTEX_ARRAY.

NVIDIA's driver returns attrib_loc=0 for glsl-getattriblocation too.

This was always pretty confusing in the specs.

Created attachment 37850 [details] [review]
Assign attrib location 0 if gl_Vertex is not used

This patch makes the test case pass.  I'm testing for regressions now.  If there are none, I'll push it to the glsl2 branch.

(In reply to comment #5)
> (In reply to comment #3)
> > (In reply to comment #1)
> > 
> > > 1. By returning attrib_loc=1 instead of 0 will we have one less user-defined
> > > vertex attribute available to users with the new compiler?  The query of
> > > GL_MAX_VERTEX_ATTRIBS_ARB still returns 16.
> > 
> > Attribute 0 is special.  It is bound to gl_Vertex by default.  Applications can
> > specifically bind to 0 using glBindAttribLocation, but the linker doesn't
> > automatically bind to it.  It's possible that the spec says we should bind
> > something to 0 if gl_Vertex isn't used, which is the case in this test.
> 
> Calling glVertexAttrib*(index=0, value) is equivalent to glVertex*(value) but I
> don't think it's the same with arrays.  That is, I don't think that generic
> attribute array 0 is the same as the conventional GL_VERTEX_ARRAY.
> 
> NVIDIA's driver returns attrib_loc=0 for glsl-getattriblocation too.
> 
> This was always pretty confusing in the specs.

It's a giant hassle. :)  The OpenGL 2.1 spec says:

    "LinkProgram will also fail if the vertex shaders used in the
    program object contain assignments (not removed during pre-processing)
    to an attribute variable bound to generic attribute zero and to the
    conventional vertex position (gl Vertex)."

That patch that I just attached only assigns location 0 if gl_Vertex is not used.  With this change the new compiler matches Nvidia's behavior for this test.  I *think* this is correct.

(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > (In reply to comment #1)
> > > 
> > > > 1. By returning attrib_loc=1 instead of 0 will we have one less user-defined
> > > > vertex attribute available to users with the new compiler?  The query of
> > > > GL_MAX_VERTEX_ATTRIBS_ARB still returns 16.
> > > 
> > > Attribute 0 is special.  It is bound to gl_Vertex by default.  Applications can
> > > specifically bind to 0 using glBindAttribLocation, but the linker doesn't
> > > automatically bind to it.  It's possible that the spec says we should bind
> > > something to 0 if gl_Vertex isn't used, which is the case in this test.
> > 
> > Calling glVertexAttrib*(index=0, value) is equivalent to glVertex*(value) but I
> > don't think it's the same with arrays.  That is, I don't think that generic
> > attribute array 0 is the same as the conventional GL_VERTEX_ARRAY.
> > 
> > NVIDIA's driver returns attrib_loc=0 for glsl-getattriblocation too.
> > 
> > This was always pretty confusing in the specs.
> 
> It's a giant hassle. :)  The OpenGL 2.1 spec says:
> 
>     "LinkProgram will also fail if the vertex shaders used in the
>     program object contain assignments (not removed during pre-processing)
>     to an attribute variable bound to generic attribute zero and to the
>     conventional vertex position (gl Vertex)."
> 
> That patch that I just attached only assigns location 0 if gl_Vertex is not
> used.  With this change the new compiler matches Nvidia's behavior for this
> test.  I *think* this is correct.

Sounds great.  Thanks.  If the new piglit tests pass we should be good.

Fixed by the following commit.  This is slightly different from that previous patch (which caused some regressions when the only access to gl_Vertex was via ftransform).

commit c33e78f62bed762d8e5987e111a6e0424dc26c76
Author: Ian Romanick <ian.d.romanick@intel.com>
Date:   Fri Aug 13 12:30:41 2010 -0700

    linker: Assign attrib location 0 if gl_Vertex is not used
    
    If gl_Vertex is not used in the shader, then attribute location 0 is
    available for use.
    
    Fixes piglit test case glsl-getattriblocation (bugzilla #29540).