[Mesa-dev] [PATCH 14/16] main: Added entry point for glCreateRenderbuffers

[Martin Peres]

On 27/03/15 07:13, Ilia Mirkin wrote:
> On Fri, Mar 27, 2015 at 1:06 AM, Vinson Lee <vlee at freedesktop.org> wrote:
>> On Mon, Feb 16, 2015 at 6:14 AM, Martin Peres
>> <martin.peres at linux.intel.com> wrote:
>>> @@ -1404,14 +1405,36 @@ _mesa_GenRenderbuffers(GLsizei n, GLuint *renderbuffers)
>>>      for (i = 0; i < n; i++) {
>>>         GLuint name = first + i;
>>>         renderbuffers[i] = name;
>>> -      /* insert dummy placeholder into hash table */
>>> +
>>> +      if (dsa) {
>>> +         obj = _mesa_new_renderbuffer(ctx, name);
>>> +      } else {
>>> +         obj = &DummyRenderbuffer;
>>> +      }
>>> +      /* insert the object into the hash table */
>>>         mtx_lock(&ctx->Shared->Mutex);
>>> -      _mesa_HashInsert(ctx->Shared->RenderBuffers, name, &DummyRenderbuffer);
>>> +      _mesa_HashInsert(ctx->Shared->RenderBuffers, name, obj);
>>>         mtx_unlock(&ctx->Shared->Mutex);
>>>      }
>>>   }
>>>
>> This patch introduced a new Coverity unused value defect.
>>
>> returned_pointer: Assigning value from allocate_renderbuffer(ctx,
>> name, func) to obj here, but that stored value is overwritten before
>> it can be used.
> Yeah, I mentioned this to Martin privately. Actually the patch above
> is fine. However I'm guessing that on rebasing, it became this:
>
>        if (dsa) {
>           obj = allocate_renderbuffer(ctx, name, func);
>        } else {
>           obj = &DummyRenderbuffer;
>
>           /* insert the object into the hash table */
>           mtx_lock(&ctx->Shared->Mutex);
>           _mesa_HashInsert(ctx->Shared->RenderBuffers, name, obj);
>           mtx_unlock(&ctx->Shared->Mutex);
>        }
>
> Which seems quite wrong -- we should move the closing } above the hash
> insertion, I would imagine.

Sorry for the noise. This is not a bug as allocate_renderbuffer inserts 
the hash in the table.
Illia is right, the code used to look like what he said but I changed it 
and forgot to remove the (now-useless) assignation.

I have a patch ready that would silence this warning, should I post it?

>
>    -ilia