Re: [PATCH 3/4] target/hexagon: idef-parser fix leak of init_list
On 07/05/24, ltaylorsimp...@gmail.com wrote:
>
>
> > -Original Message-
> > From: 'Anton Johansson'
> > 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
> > > > 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
> > > > ---
> > > > 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
RE: [PATCH 3/4] target/hexagon: idef-parser fix leak of init_list
> -Original Message-
> From: 'Anton Johansson'
> 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
> > > 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
> > > ---
> > > 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
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
> > 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
> > ---
> > 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
RE: [PATCH 3/4] target/hexagon: idef-parser fix leak of init_list
> -Original Message-
> From: Anton Johansson
> 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
> ---
> 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;
Taylor
