[Mesa-dev] [PATCH 3/3] [Bug 38970] [bisected]piglit glx/glx-pixmap-multi failed

[Nicolai Hähnle]

On 18.10.2016 19:23, Ian Romanick wrote:
> On 09/29/2016 01:55 PM, Anutex wrote:
>> I tried to debug this issue with changing the condition to check only bad magic and Error.
>> And the test passed.
>>
>> Though i am not sure what is the correct behaviour if we are in this condition.
>> May be we should make some  other condition if the Hash Table have the bucket data.
>> ---
>>  src/glx/dri2_glx.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c
>> index af388d9..a1fd9ff 100644
>> --- a/src/glx/dri2_glx.c
>> +++ b/src/glx/dri2_glx.c
>> @@ -411,12 +411,13 @@ dri2CreateDrawable(struct glx_screen *base, XID xDrawable,
>>        return NULL;
>>     }
>>
>> -   if (__glxHashInsert(pdp->dri2Hash, xDrawable, pdraw)) {
>> +   if (__glxHashInsert(pdp->dri2Hash, xDrawable, pdraw) == -1) {
>
> I'm not 100% sure the existing code is wrong.  __glxHashInsert returns
> -1 for an error, and it returns 1 if the key is already in the hash
> table.  In that case we'll leak the memory for the new pdraw, right?
> That also seems bad.
>
> It seems like instead the code should look up xDrawable in the hash
> table and return the value that's already there.  Maybe.  I haven't
> looked at this code in years, so I may be forgetting some subtlety.

dri2DestroyDrawable destroys the pdraw though. It also removes the 
xDrawable entry in the hash table without checking whether it points at 
pdraw or not, so on the surface that looks pretty bogus if we create a 
GLXDrawable twice.

_However_, the real question is what the hash is used for in the first 
place. It looks to me like the hash is actually pretty pointless in the 
pixmap case. And it just so happens that the GLX spec forbids creating a 
GLXDrawable from a Window twice, but it doesn't forbid creating a 
GLXDrawable from a Pixmap twice.

Then again, my GLX knowledge is basically zero, so what do I know :)

Nicolai

>
>>        (*psc->core->destroyDrawable) (pdraw->driDrawable);
>>        DRI2DestroyDrawable(psc->base.dpy, xDrawable);
>>        free(pdraw);
>>        return None;
>>     }
>> +	
>>
>
> Spurious whitespace change.
>
>>     /*
>>      * Make sure server has the same swap interval we do for the new
>>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>