[PATCH 6/6] d3dcompiler: Parse initializer lists using a variable-size array.

[Zebediah Figura]

On 8/9/19 1:01 PM, Matteo Bruni wrote:
> On Fri, Aug 9, 2019 at 4:53 PM Zebediah Figura <zfigura at codeweavers.com> wrote:
>>
>> Signed-off-by: Zebediah Figura <zfigura at codeweavers.com>
>> ---
>>   dlls/d3dcompiler_43/d3dcompiler_private.h |   8 +-
>>   dlls/d3dcompiler_43/hlsl.y                | 117 +++++++++++-----------
>>   2 files changed, 67 insertions(+), 58 deletions(-)
> 
>>       LIST_FOR_EACH_ENTRY(field, type->e.elements, struct hlsl_struct_field, entry)
>>       {
>> -        if (!cur_node)
>> +        struct hlsl_ir_node *node = initializer->args[i];
>> +
>> +        if (i++ >= initializer->args_count)
>>           {
>>               d3dcompiler_free(initializer);
>>               return;
>> @@ -528,19 +528,12 @@ static void struct_var_initializer(struct list *list, struct hlsl_ir_var *var, s
>>           }
>>           else
>>               FIXME("Initializing with \"mismatched\" fields is not supported yet.\n");
>> -        cur_node = list_next(initializer, cur_node);
>> -        node = LIST_ENTRY(cur_node, struct hlsl_ir_node, entry);
>>       }
>>
>>       /* Free initializer elements in excess. */
>> -    while (cur_node)
>> -    {
>> -        struct list *next = list_next(initializer, cur_node);
>> -        free_instr(node);
>> -        cur_node = next;
>> -        node = LIST_ENTRY(cur_node, struct hlsl_ir_node, entry);
>> -    }
>> -    d3dcompiler_free(initializer);
>> +    for (; i < initializer->args_count; ++i)
>> +        free_instr(initializer->args[i]);
>> +    d3dcompiler_free(initializer->args);
>>   }
> 
> This is a bit suspicious. In the hunk above there's still a
> d3dcompiler_free(initializer) but here you're removing it. I think
> removing it is correct but that probably means the one above should
> also go.

Yep, I just missed the one above. Thanks for catching it.

> 
>> @@ -624,7 +616,9 @@ static struct list *declare_vars(struct hlsl_type *basic_type, DWORD modifiers,
>>                   {
>>                       hlsl_report_message(v->loc.file, v->loc.line, v->loc.col, HLSL_LEVEL_ERROR,
>>                               "'%s' initializer does not match", v->name);
>> -                    free_instr_list(v->initializer);
>> +                    for (i = 0; i < v->initializer.args_count; ++i)
>> +                        free_instr(v->initializer.args[i]);
>> +                    d3dcompiler_free(v->initializer.args);
>>                       d3dcompiler_free(v);
>>                       continue;
>>                   }
> 
> Not exactly critical, but maybe introducing a small function for
> freeing the initializer would be nice.
> 
>>                           | initializer_expr_list ',' initializer_expr
>>                               {
>>                                   $$ = $1;
>> -                                list_add_tail($$, &$3->entry);
>> +                                $$.args = d3dcompiler_realloc($$.args, ($$.args_count + 1) * sizeof(*$$.args));
>> +                                $$.args[$$.args_count++] = $3;
>>                               }
> 
> This isn't really something new with your patch, just to start a
> conversation on the question: is this a memory allocation we should
> check? Which are those?
> 
> I think ideally we should check all of them, although that's quite
> impractical. For this one specifically I think it's reasonable to
> check it given that it's a realloc for a (small, but potentially not
> tiny) array.
> Then there's the question of handling the failure without crashing
> while completing parsing. IIRC I tried to do that, in places :/
> I'd say don't go to crazy lengths to make this (and other similar
> spots) entirely correct, but when it's not too much of a nuisance it's
> nice to have. Fixing those is not a priority of course.
> 

Yeah, that's fair; I just noticed there were places that did both and 
immediately went with the easier option. But it's not particularly 
difficult to watch for allocation errors, so I'll do that where practical.