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