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

[nobled]

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

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.


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