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

[Ilia Mirkin]

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? Pointless assignments are...
pointless. (BTW, 'assignation' is something else... English is fun.)

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?

  -ilia