[Mesa-dev] Mesa (master): mesa: add back glGetnUniform*v() overflow error reporting

[Ian Romanick]

On 03/15/2012 05:09 PM, nobled wrote:
> On Thu, Mar 15, 2012 at 5:27 PM, Ian Romanick<idr at freedesktop.org>  wrote:
>> On 03/15/2012 01:18 PM, Kenneth Graunke wrote:
>>>
>>> On 03/15/2012 11:26 AM, Ian Romanick wrote:
>>>>
>>>> On 03/13/2012 11:35 AM, Dylan Noblesmith wrote:
>>>>>
>>>>> Module: Mesa
>>>>> Branch: master
>>>>> Commit: b536ac6b2bc54ad9bb6e58fbd307055e419a288f
>>>>> URL:
>>>>>
>>>>> http://cgit.freedesktop.org/mesa/mesa/commit/?id=b536ac6b2bc54ad9bb6e58fbd307055e419a288f
>>>>>
>>>>>
>>>>>
>>>>> Author: Dylan Noblesmith<nobled at dreamwidth.org>
>>>>> Date: Thu Dec 22 21:05:38 2011 +0000
>>>>>
>>>>> mesa: add back glGetnUniform*v() overflow error reporting
>>>>>
>>>>> The error was removed in:
>>>>>
>>>>> commit 719909698c67c287a393d2380278e7b7495ae018
>>>>> Author: Ian Romanick<ian.d.romanick at intel.com>
>>>>> Date: Tue Oct 18 16:01:49 2011 -0700
>>>>>
>>>>> mesa: Rewrite the way uniforms are tracked and handled
>>>>>
>>>>> The GL_ARB_robustness spec doesn't say the implementation
>>>>> should truncate the output, so just return after setting
>>>>> the required error like it did before the above commit.
>>>>
>>>>
>>>> This patch is wrong. Please revert. The ARB_robustness spec specifically
>>>> says:
>>>>
>>>> "The commands
>>>>
>>>> void GetUniformfv(uint program, int location, float *params);
>>>> void GetnUniformfvARB(uint program, int location, sizei bufSize,
>>>> float *params);
>>>> void GetUniformiv(uint program, int location, int *params);
>>>> void GetnUniformivARB(uint program, int location, sizei bufSize,
>>>> int *params);
>>>> void GetUniformuiv(uint program, int location, uint *params);
>>>> void GetnUniformuivARB(uint program, int location, sizei bufSize,
>>>> uint *params);
>>>> void GetUniformdv(uint program, int location, uint *params);
>>>> void GetnUniformdvARB(uint program, int location, sizei bufSize,
>>>> uint *params);
>>>>
>>>> return the value or values of the uniform at location location of
>>>> the default uniform block for program object program in the array
>>>> params. GetnUniformfvARB, GetnUniformivARB, GetnUniformuivARB and
>>>> GetnUniformdvARB do not write more than<bufSize>  bytes into<params>."
>>>>
>>>> The "do not write more than<bufSize>  bytes" is the important bit.
>
> And zero is not more than<bufSize>, and we're writing zero bytes here, so....
>
> I should have quoted more extensively though, you're right. Specifically:
>
>
> "Errors
>
>      [...] GetnUniformfvARB,
>      GetnUniformivARB, GetnUniformuivARB and GetnUniformdvARB share all the
>      errors of their unsized buffer query counterparts with the addition
>      that INVALID_OPERATION is generated if the buffer size required to
>      fill all the requested data is less than<bufSize>."

Wow.  I read the last bit of that and almost cried.  I've submitted a 
spec bug.  Obviously the above should read "...INVALID_OPERATION is 
generated if the buffer size required to fill all the requested data is 
greater than <bufSize>."  The text near the Getn functions should also 
be worded differently.  Ugh.

> and
>
> "Issues
>
> [...]
>
>      5.  If a new query with an explicit buffer size is called and the buffer
>          size is not sufficient to return the data, what happens?
>
>          RESOLVED:  Return an INVALID_OPERATION error and ignore the query."
>
> Sorry for being too vague in the commit log; I should've said the spec
> *explicitly forbids* the implementation from doing anything at all
> when<bufSize>  is not sufficient.

Okay, that makes sense.  The rationale in the spec also makes sense.  Do 
we have a piglit test for this behavior?

>>>> In the future, please wait for at least one Reviewed-by when changing
>>>> API behavior, and quote spec language in the code to justify such
>>>> changes.
>>>
>>>
>>> In fairness, those patches had sat on the list for a long time, which
>>> seems to invoke the "silence is consent" rule. We should have reviewed
>>> it earlier.
>>
>> Yeah... I'm not sure how I missed these.  I also noticed that there was
>> review on the list from Brian, but that didn't end up as a Reviewed-by in
>> the commit message.  So, I'll take the last part back. :)