From: Cupertino Miranda <cupertino.miranda@oracle.com>
To: David Faust <david.faust@oracle.com>
Cc: gcc-patches@gcc.gnu.org, jose.marchesi@oracle.com,
elena.zannoni@oracle.com
Subject: Re: [PATCH v3] bpf: remove huge memory waste with string allocation.
Date: Fri, 19 Apr 2024 11:18:04 +0100 [thread overview]
Message-ID: <87ttjxirkz.fsf@oracle.com> (raw)
In-Reply-To: <ee1553b7-88a2-4166-871f-2e33d81064a2@oracle.com>
David Faust writes:
> Hi Cupertino,
>
> On 4/18/24 13:58, Cupertino Miranda wrote:
>> Hi David, everyone,
>>
>> Following Davids last review I decided to properly detect error cases,
>> as suggested.
>> The error however should be reported earlier in compilation in
>> pack_enum_valud function, where all the errors are reported.
>>
>> Thanks for the quick and detailed reviews.
>>
>> Regards,
>> Cupertino
>
> Thanks for taking the time on this.
> This version is nice, just one little comment:
>
>>
>> The BPF backend was allocating an unnecessarily large string when
>> constructing CO-RE relocations for enum types.
>> This patch further verifies if an enumerator is valid for CO-RE
>> representability and returns an error in those cases.
>
> The second sentence is a little awkward and seems to imply the error is
> returned when the enumerator is valid :)
> Perhaps "...verifies that an enumerator is valid for CO-RE, and returns
> an error if it is not" or similar would be more clear?
Thanks for all the suggestions.
>
> Otherwise, OK.
> Thanks!
Pushed!
>
>
>>
>> gcc/ChangeLog:
>> * config/bpf/core-builtins.cc (get_index_for_enum_value): Create
>> function.
>> (pack_enum_value): Check for enumerator and error out.
>> (process_enum_value): Correct string allocation.
>> ---
>> gcc/config/bpf/core-builtins.cc | 57 ++++++++++++++++++++++-----------
>> 1 file changed, 38 insertions(+), 19 deletions(-)
>>
>> diff --git a/gcc/config/bpf/core-builtins.cc b/gcc/config/bpf/core-builtins.cc
>> index e03e986e2c1..829acea98f7 100644
>> --- a/gcc/config/bpf/core-builtins.cc
>> +++ b/gcc/config/bpf/core-builtins.cc
>> @@ -795,6 +795,23 @@ process_field_expr (struct cr_builtins *data)
>> static GTY(()) hash_map<tree, tree> *bpf_enum_mappings;
>> tree enum_value_type = NULL_TREE;
>>
>> +static int
>> +get_index_for_enum_value (tree type, tree expr)
>> +{
>> + gcc_assert (TREE_CODE (expr) == CONST_DECL
>> + && TREE_CODE (type) == ENUMERAL_TYPE);
>> +
>> + unsigned int index = 0;
>> + for (tree l = TYPE_VALUES (type); l; l = TREE_CHAIN (l))
>> + {
>> + gcc_assert (index < (1 << 16));
>> + if (TREE_VALUE (l) == expr)
>> + return index;
>> + index++;
>> + }
>> + return -1;
>> +}
>> +
>> /* Pack helper for the __builtin_preserve_enum_value. */
>>
>> static struct cr_local
>> @@ -846,6 +863,16 @@ pack_enum_value_fail:
>> ret.reloc_data.default_value = integer_one_node;
>> }
>>
>> + if (ret.fail == false )
>> + {
>> + int index = get_index_for_enum_value (type, tmp);
>> + if (index == -1 || index >= (1 << 16))
>> + {
>> + bpf_error ("enum value in CO-RE builtin cannot be represented");
>> + ret.fail = true;
>> + }
>> + }
>> +
>> ret.reloc_data.type = type;
>> ret.reloc_data.kind = kind;
>> return ret;
>> @@ -864,25 +891,17 @@ process_enum_value (struct cr_builtins *data)
>>
>> struct cr_final ret = { NULL, type, data->kind };
>>
>> - if (TREE_CODE (expr) == CONST_DECL
>> - && TREE_CODE (type) == ENUMERAL_TYPE)
>> - {
>> - unsigned int index = 0;
>> - for (tree l = TYPE_VALUES (type); l; l = TREE_CHAIN (l))
>> - {
>> - if (TREE_VALUE (l) == expr)
>> - {
>> - char *tmp = (char *) ggc_alloc_atomic ((index / 10) + 1);
>> - sprintf (tmp, "%d", index);
>> - ret.str = (const char *) tmp;
>> -
>> - break;
>> - }
>> - index++;
>> - }
>> - }
>> - else
>> - gcc_unreachable ();
>> + gcc_assert (TREE_CODE (expr) == CONST_DECL
>> + && TREE_CODE (type) == ENUMERAL_TYPE);
>> +
>> + int index = get_index_for_enum_value (type, expr);
>> + gcc_assert (index != -1 && index < (1 << 16));
>> +
>> + /* 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));
>>
>> return ret;
>> }
prev parent reply other threads:[~2024-04-19 10:18 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-18 20:58 Cupertino Miranda
2024-04-18 21:29 ` David Faust
2024-04-19 10:18 ` Cupertino Miranda [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=87ttjxirkz.fsf@oracle.com \
--to=cupertino.miranda@oracle.com \
--cc=david.faust@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).