diff mbox series

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

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

Checks

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

Commit Message

Cupertino Miranda April 17, 2024, 6:44 p.m. UTC 
  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(-)

Comments

David Faust April 17, 2024, 7:21 p.m. UTC | #1 
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++;
diff mbox series

Patch

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));
 	  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++;