From: 'Anton Johansson' via <qemu-devel@nongnu.org>
To: ltaylorsimpson@gmail.com
Cc: qemu-devel@nongnu.org, ale@rev.ng, bcain@quicinc.com
Subject: Re: [PATCH 3/4] target/hexagon: idef-parser fix leak of init_list
Date: Wed, 8 May 2024 16:24:01 +0200 [thread overview]
Message-ID: <jsusre6vphnnyoengz74djo2dvaoiz3auruhkmcc2pygv73w3u@hk7zhipgpbz6> (raw)
In-Reply-To: <071701daa0c2$5988d450$0c9a7cf0$@gmail.com>
On 07/05/24, ltaylorsimpson@gmail.com wrote:
>
>
> > -----Original Message-----
> > From: 'Anton Johansson' <anjo@rev.ng>
> > Sent: Tuesday, May 7, 2024 4:47 AM
> > To: ltaylorsimpson@gmail.com
> > Cc: qemu-devel@nongnu.org; ale@rev.ng; bcain@quicinc.com
> > Subject: Re: [PATCH 3/4] target/hexagon: idef-parser fix leak of init_list
> >
> > On 06/05/24, ltaylorsimpson@gmail.com wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Anton Johansson <anjo@rev.ng>
> > > > Sent: Monday, May 6, 2024 1:31 PM
> > > > To: qemu-devel@nongnu.org
> > > > Cc: ale@rev.ng; ltaylorsimpson@gmail.com; bcain@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 <anjo@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
next prev parent reply other threads:[~2024-05-08 14:24 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-06 18:31 [PATCH 0/4] target/hexagon: Minor idef-parser cleanup Anton Johansson via
2024-05-06 18:31 ` [PATCH 1/4] target/hexagon: idef-parser remove unused defines Anton Johansson via
2024-05-06 18:58 ` ltaylorsimpson
2024-05-06 18:31 ` [PATCH 2/4] target/hexagon: idef-parser remove undefined functions Anton Johansson via
2024-05-06 18:58 ` ltaylorsimpson
2024-05-06 18:31 ` [PATCH 3/4] target/hexagon: idef-parser fix leak of init_list Anton Johansson via
2024-05-06 18:58 ` ltaylorsimpson
2024-05-07 9:47 ` 'Anton Johansson' via
2024-05-07 21:05 ` ltaylorsimpson
2024-05-08 14:24 ` 'Anton Johansson' via [this message]
2024-05-06 18:31 ` [PATCH 4/4] target/hexagon: idef-parser simplify predicate init Anton Johansson via
2024-05-06 18:57 ` ltaylorsimpson
2024-05-07 11:18 ` 'Anton Johansson' via
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=jsusre6vphnnyoengz74djo2dvaoiz3auruhkmcc2pygv73w3u@hk7zhipgpbz6 \
--to=qemu-devel@nongnu.org \
--cc=ale@rev.ng \
--cc=anjo@rev.ng \
--cc=bcain@quicinc.com \
--cc=ltaylorsimpson@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).