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

[Matteo Bruni]

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.

> @@ -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.