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