[Mesa-dev] [PATCH 1/5] mesa: Fix errors values returned by glShaderBinary()

[Eduardo Lima Mitev]

On 06/09/2015 07:41 PM, Ian Romanick wrote:
> On 06/04/2015 06:38 PM, Ben Widawsky wrote:
>> On Wed, Mar 11, 2015 at 10:01:24AM +0100, Eduardo Lima Mitev wrote:
>>> Page 68, section 7.2 'Shader Binaries" of the of the OpenGL ES 3.1,
>>> and page 88 of the OpenGL 4.5 specs state:
>>>
>>>     "An INVALID_VALUE error is generated if count or length is negative.
>>>      An INVALID_ENUM error is generated if binaryformat is not a supported
>>>      format returned in SHADER_BINARY_FORMATS."
>>>
>>> Currently, an INVALID_OPERATION error is returned for all cases.
>>>
>>> Fixes 1 dEQP test:
>>> * dEQP-GLES3.functional.negative_api.shader.shader_binary
>>> ---
>>>  src/mesa/main/shaderapi.c | 15 ++++++++++++++-
>>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
>>> index 5731d58..49a9c80 100644
>>> --- a/src/mesa/main/shaderapi.c
>>> +++ b/src/mesa/main/shaderapi.c
>>> @@ -1711,7 +1711,20 @@ _mesa_ShaderBinary(GLint n, const GLuint* shaders, GLenum binaryformat,
>>>     (void) binaryformat;
>>>     (void) binary;
>>>     (void) length;
>>> -   _mesa_error(ctx, GL_INVALID_OPERATION, "glShaderBinary");
>>> +
>>> +   /* Page 68, section 7.2 'Shader Binaries" of the of the OpenGL ES 3.1, and
>>> +    * page 88 of the OpenGL 4.5 specs state:
>>> +    *
>>> +    *     "An INVALID_VALUE error is generated if count or length is negative.
>>> +    *      An INVALID_ENUM error is generated if binaryformat is not a supported
>>> +    *      format returned in SHADER_BINARY_FORMATS."
>>> +    */
>>> +   if (n < 0 || length < 0) {
>>> +      _mesa_error(ctx, GL_INVALID_VALUE, "glShaderBinary(count or length < 0)");
>>> +      return;
>>> +   }
>>> +
>>> +   _mesa_error(ctx, GL_INVALID_ENUM, "glShaderBinary(format)");
>>>  }
>>>  
> 
> There are some generic errors that apply to all GL functions.  These are
> listed in section 2.3.1 (Errors).  The full text from section 7.2 (from
> the GL 4.4 spec) says:
> 
>     An INVALID_VALUE error is generated if count or length is negative.
> 
>     An INVALID_ENUM error is generated if binaryformat is not a
>     supported format returned in SHADER_BINARY_FORMATS.
> 
>     An INVALID_VALUE error is generated if the data pointed to by binary
>     does not match the specified binaryformat.
> 
>     An INVALID_VALUE error is generated if any of the handles in
>     shaders is not the name of either a program or shader object.
> 
>     An INVALID_OPERATION error is generated if any of the handles in
>     shader is the name of a program object.
> 
>     An INVALID_OPERATION error is generated if more than one of the
>     handles in shaders refers to the same type of shader object.
> 
>     Additional errors corresponding to specific binary formats may be
>     generated as specified by the extensions defining those formats.
> 
> If we're going to add code to generate two of the specified error codes,
> we probably ought to add code to generate the other as well.
> 

I agree with this in general, but in this particular case since loading
binary shaders is not supported by mesa, we will never reach past the
2nd error check on that list:

An INVALID_ENUM error is generated if binaryformat is not a
supported format returned in SHADER_BINARY_FORMATS.

So it is pointless to validate further.

The original patch adds the only check that, per the spec, goes earlier.

cheers,
Eduardo