Re: [PATCH 2/3] bpf: remove huge memory waste with string allocation.

[David Faust <david.faust@oracle.com>]

Hi Cupertino,

On 4/11/24 04:11, Cupertino Miranda wrote:
> Code was allocating way too much space for the string.

A little bit more description would not hurt. Perhaps you could say
something like:
"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): Corrected
> 	string allocation.

nit: present tense, i.e. "Correct" rather than "Corrected"

> ---
>  gcc/config/bpf/core-builtins.cc | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/gcc/config/bpf/core-builtins.cc b/gcc/config/bpf/core-builtins.cc
> index e03e986e2c1..ead1777d465 100644
> --- a/gcc/config/bpf/core-builtins.cc
> +++ b/gcc/config/bpf/core-builtins.cc
> @@ -872,10 +872,11 @@ process_enum_value (struct cr_builtins *data)
>  	{
>  	  if (TREE_VALUE (l) == expr)
>  	    {
> -	      char *tmp = (char *) ggc_alloc_atomic ((index / 10) + 1);
> +	      /* Array size is 21 = ceil(log_10(2^64)) + 1 to hold string
> +		 representations of 64 bit integers.  */
> +	      char tmp[21];
>  	      sprintf (tmp, "%d", index);

It looks like `index' is an `unsigned int', so this sprintf should use
%u rather %d, no?

Also, it occurs to me that the `vlen' of BTF types is only 16 bits, so
BTF has no way currently to represent enums with more than 65535
enumerators. It may be worth adding a sanity check to bail out (error)
here if we're going to claim an index higher than that. And if that is
validated before the printf, the buffer can be 6 bytes ("65535\0").

> -	      ret.str = (const char *) tmp;
> -
> +	      ret.str = CONST_CAST (char *, ggc_strdup(tmp));
>  	      break;
>  	    }
>  	  index++;