On Fri, Aug 9, 2019 at 4:53 PM Zebediah Figura <zfigura(a)codeweavers.com> wrote:
Signed-off-by: Zebediah Figura <zfigura(a)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.