[Mesa-dev] [PATCH v2] glsl: use non-null context when cloning variable

[Tapani Pälli]

On 06/29/2015 08:34 AM, Kenneth Graunke wrote:
> On Monday, June 29, 2015 01:13:30 AM Ilia Mirkin wrote:
>> ProgramResourceList might not yet have been initialized. In that case,
>> parent the var to the program.
>>
>> Fixes: c2ff3485b3d (glsl: clone inputs and outputs during linking)
>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>> Cc: mesa-stable at lists.freedesktop.org
>> ---
>>
>> v1 -> v2: parent to prog only if the resource list doesn't exist
>>
>> Perhaps it's not worth it to clone in the first place if the resource
>> list isn't there?
>>
>>   src/glsl/linker.cpp | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>> index 5da9cad..c8cd858 100644
>> --- a/src/glsl/linker.cpp
>> +++ b/src/glsl/linker.cpp
>> @@ -2639,7 +2639,9 @@ add_interface_variables(struct gl_shader_program *shProg,
>>
>>         /* Clone ir_variable data so that backend is able to free memory. */
>>         if (!add_program_resource(shProg, programInterface,
>> -                                var->clone(shProg->ProgramResourceList, NULL),
>> +                                var->clone(shProg->ProgramResourceList ?
>> +                                           (void *)shProg->ProgramResourceList :
>> +                                           (void *)shProg, NULL),
>>                                   build_stageref(shProg, var->name) | mask))
>>            return false;
>>      }
>>
>
> There's a mistake in the original patch - add_program_resource is what
> allocates ProgramResourceList in the first place, making it non-NULL.
> But before it can do that, we use it as a memory context for cloning.
>
> Tapani, any thoughts on that?  You know this code much better than I do :)

Right, this was not so simple after all. I think we might even need a 
separate context here to handle this gracefully.

> I've gone ahead and reverted those two patches, which should fix this
> leak.  Tapani found some other bugs in those patches, and I believe
> is looking into fixing them.  We may as well revert them in the
> meantime.

Yes, revert makes sense. I will first move the resource list creation to 
happen only after driver link hook, then we can worry about 
allocating/cloning in a proper manner and freeing the IR.

// Tapani