On 07/05/24, ltaylorsimp...@gmail.com wrote:
>
>
> > -----Original Message-----
> > From: 'Anton Johansson' <a...@rev.ng>
> > Sent: Tuesday, May 7, 2024 4:47 AM
> > To: ltaylorsimp...@gmail.com
> > Cc: qemu-devel@nongnu.org; a...@rev.ng; bc...@quicinc.com
> > Subject: Re: [PATCH 3/4] target/hexagon: idef-parser fix leak of init_list
> >
> > On 06/05/24, ltaylorsimp...@gmail.com wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Anton Johansson <a...@rev.ng>
> > > > Sent: Monday, May 6, 2024 1:31 PM
> > > > To: qemu-devel@nongnu.org
> > > > Cc: a...@rev.ng; ltaylorsimp...@gmail.com; bc...@quicinc.com
> > > > Subject: [PATCH 3/4] target/hexagon: idef-parser fix leak of
> > > > init_list
> > > >
> > > > gen_inst_init_args() is called for instructions using a predicate as
> > > > an
> > > rvalue.
> > > > Upon first call, the list of arguments which might need
> > > > initialization
> > > init_list is
> > > > freed to indicate that they have been processed. For instructions
> > > > without
> > > an
> > > > rvalue predicate,
> > > > gen_inst_init_args() isn't called and init_list will never be freed.
> > > >
> > > > Free init_list from free_instruction() if it hasn't already been freed.
> > > >
> > > > Signed-off-by: Anton Johansson <a...@rev.ng>
> > > > ---
> > > > target/hexagon/idef-parser/parser-helpers.c | 7 +++++++
> > > > 1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/target/hexagon/idef-parser/parser-helpers.c
> > > > b/target/hexagon/idef-parser/parser-helpers.c
> > > > index 95f2b43076..bae01c2bb8 100644
> > > > --- a/target/hexagon/idef-parser/parser-helpers.c
> > > > +++ b/target/hexagon/idef-parser/parser-helpers.c
> > > > @@ -2121,6 +2121,13 @@ void free_instruction(Context *c)
> > > > g_string_free(g_array_index(c->inst.strings, GString*, i),
> > > > TRUE);
> > > > }
> > > > g_array_free(c->inst.strings, TRUE);
> > > > + /*
> > > > + * Free list of arguments that might need initialization, if
> > > > + they
> > > haven't
> > > > + * already been free'd.
> > > > + */
> > > > + if (c->inst.init_list) {
> > > > + g_array_free(c->inst.init_list, TRUE);
> > > > + }
> > > > /* Free INAME token value */
> > > > g_string_free(c->inst.name, TRUE);
> > > > /* Free variables and registers */
> > >
> > > Why not do this in gen_inst_init_args just before this?
> > > /* Free argument init list once we have initialized everything */
> > > g_array_free(c->inst.init_list, TRUE);
> > > c->inst.init_list = NULL;
> >
> > Thanks for the reviews Taylor! I'm not sure I understand what you mean
> > here, we already free init_list in gen_inst_init_args, since
> > gen_inst_init_args
> > won't be called for all instructions we need to also free from a common
> > place.
> >
> > //Anton
>
> It just seems more natural to free the elements of the array at the same
> place you free the array itself. If there are valid reasons for doing it
> elsewhere, I'm OK with that.
>
> Taylor
>
>
Ah I see what you mean. The array and its elements are either freed in
gen_inst_init_args or free_instruction so they do occur together. The
"freeing of variables and registers" comment only refers to declared
TCGvs and has nothing to do with the arguments.
Comment is a bit outdated so I've updated it.
//Anton
