On 8/9/19 1:01 PM, Matteo Bruni wrote:
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.
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.