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

[Martin Peres]

On 27/03/15 17:40, Ilia Mirkin wrote:
> On Fri, Mar 27, 2015 at 4:19 AM, Martin Peres
> <martin.peres at linux.intel.com> wrote:
>> 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?
> Yes. If a patch improves code (e.g. by removing an unnecessary and/or
> confusing assignment), why not?

True, this patch is unlikely to conflict with any other patch since it 
is just one-line long.

> Pointless assignments are...
> pointless. (BTW, 'assignation' is something else... English is fun.)

Ahah, right.

> Also, it doesn't seem like allocate_renderbuffer locks the hash before
> inserting into it... that seems like a potential bug too, right?
> Alternatively if the locking is unnecessary, remove it in the other
> place?
I had planed on looking in this issue but apparently somehow forgot. 
Anyway, you
are right. It is a bug. Have a look at the series I'll be posting in 
reply to your email
for a full explanation.

Thanks for your comments and reminding me to look into this issue.

Martin