Re: [PATCH v3] bpf: remove huge memory waste with string allocation.
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 *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;
>> }
Re: [PATCH v3] bpf: remove huge memory waste with string allocation.
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?
Otherwise, OK.
Thanks!
>
> 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 *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;
> }
[PATCH v3] bpf: remove huge memory waste with string allocation.
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
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.
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 *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;
}
--
2.30.2
