diff mbox series

[v3] bpf: remove huge memory waste with string allocation.

Message ID 20240418205821.366065-1-cupertino.miranda@oracle.com
State New
Headers
Series [v3] bpf: remove huge memory waste with string allocation. |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Testing passed

Commit Message

Cupertino Miranda April 18, 2024, 8:58 p.m. UTC 
  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(-)

Comments

David Faust April 18, 2024, 9:29 p.m. UTC | #1 
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<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;
>  }
Cupertino Miranda April 19, 2024, 10:18 a.m. UTC | #2 
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;
>>  }
diff mbox series

Patch

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;
 }