From: David Faust <david.faust@oracle.com>
To: Cupertino Miranda <cupertino.miranda@oracle.com>
Cc: gcc-patches@gcc.gnu.org, jose.marchesi@oracle.com,
elena.zannoni@oracle.com
Subject: Re: [PATCH v2] bpf: remove huge memory waste with string allocation.
Date: Wed, 17 Apr 2024 12:21:59 -0700 [thread overview]
Message-ID: <67dd0e90-4091-4a48-a348-fc68b3a7f130@oracle.com> (raw)
In-Reply-To: <20240417184441.339037-1-cupertino.miranda@oracle.com>
On 4/17/24 11:44, Cupertino Miranda wrote:
> The BPF backend was allocating an unnecessarily large string when
> constructing CO-RE relocations for enum types.
>
> gcc/ChangeLog:
> * config/bpf/core-builtins.cc (process_enum_value): Correct
> string allocation.
> ---
> gcc/config/bpf/core-builtins.cc | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/gcc/config/bpf/core-builtins.cc b/gcc/config/bpf/core-builtins.cc
> index e03e986e2c1..beb039ea6e0 100644
> --- a/gcc/config/bpf/core-builtins.cc
> +++ b/gcc/config/bpf/core-builtins.cc
> @@ -870,12 +870,14 @@ process_enum_value (struct cr_builtins *data)
> unsigned int index = 0;
> for (tree l = TYPE_VALUES (type); l; l = TREE_CHAIN (l))
> {
> + gcc_assert (index < (1 << 16));
Since the index here is computed from the TREE node and not BTF, is it
somehow already enforced that this code won't be run if the enum can't
be represented in BTF?
If not, I don't think the assert is a good idea. The TREE node could be
an enum too large for BTF, but not too large for gcc. So the assert
will cause an ICE for something that is a representation limit (in BTF
and by extension .BTF.ext i.e. CO-RE) rather than a compiler bug.
IMO an error or warning makes more sense here, something like:
if (index >= (1 << 16))
{
bpf_error ("enumerator in CO-RE relocation cannot be represented");
break;
}
WDYT?
> if (TREE_VALUE (l) == expr)
> {
> - char *tmp = (char *) ggc_alloc_atomic ((index / 10) + 1);
> - sprintf (tmp, "%d", index);
> - ret.str = (const char *) tmp;
> -
> + /* Index can only be a value up to 2^16. Should always fit
> + in 6 chars. */
> + char tmp[6];
> + sprintf (tmp, "%u", index);
> + ret.str = CONST_CAST (char *, ggc_strdup(tmp));
> break;
> }
> index++;
prev parent reply other threads:[~2024-04-17 19:22 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-17 18:44 Cupertino Miranda
2024-04-17 19:21 ` David Faust [this message]
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=67dd0e90-4091-4a48-a348-fc68b3a7f130@oracle.com \
--to=david.faust@oracle.com \
--cc=cupertino.miranda@oracle.com \
--cc=elena.zannoni@oracle.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jose.marchesi@oracle.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).