[Mesa-dev] [PATCH] glsl: Fix memory leak with known glsl_type instances

[Emil Velikov]

On 27 February 2018 at 05:43, Tapani Pälli <tapani.palli at intel.com> wrote:
>
>
> On 02/26/2018 07:55 PM, Emil Velikov wrote:
>>
>> On 15 February 2018 at 09:12, Tapani Pälli <tapani.palli at intel.com> wrote:
>>>
>>> From: Simon Hausmann <simon.hausmann at qt.io>
>>>
>>> When looking up known glsl_type instances in the various hash tables, we
>>> end up leaking the key instances used for the lookup, as the glsl_type
>>> constructor allocates memory on the global mem_ctx. This patch changes
>>> glsl_type to manage its own memory, which fixes the leak and also allows
>>> getting rid of the global mem_ctx and its mutex.
>>>
>>> v2: remove lambda usage (Tapani)
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104884
>>> Signed-off-by: Simon Hausmann <simon.hausmann at qt.io>
>>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>>> ---
>>>   src/compiler/glsl_types.cpp | 83
>>> ++++++++++++++++++++-------------------------
>>>   src/compiler/glsl_types.h   | 44 +++---------------------
>>>   2 files changed, 41 insertions(+), 86 deletions(-)
>>>
>> Great overall result, there's a small question below.
>>
>> Can you include the mesa-stable tag and/or the commit that introduced
>> this shared memory pool.
>> ... Gut feeling says it was before the file started using ralloc.
>
>
> Sure, can add this.
>
>>
>>> @@ -970,7 +961,7 @@ glsl_type::get_array_instance(const glsl_type *base,
>>> unsigned array_size)
>>>         const glsl_type *t = new glsl_type(base, array_size);
>>>
>>>         entry = _mesa_hash_table_insert(array_types,
>>> -                                      ralloc_strdup(mem_ctx, key),
>>> +                                      strdup(key),
>>>                                         (void *) t);
>>>      }
>>>
>>
>> Are you sure we need the strdup here? AFAICT nothing clears it so it
>> will be leaked.
>>
>
> Yes I believe we do need it, see comment about base type name some rows
> before. It is being freed during _mesa_glsl_release_types() by the
> hash_free_type_function when array_types hash is iterated.
>
Right - could swear I saw _mesa_hash_table_insert duplicating the key.
That's not the case, so everything will be torn as you mentioned.

Thanks Tapani!

With a stable tag. the patch is
Reviewed-by: Emil Velikov <emil.velikov at collabora.com>

-Emil