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

[David Faust <david.faust@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++;