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

[Cupertino Miranda <cupertino.miranda@oracle.com>]

Hi David,

Thanks for the review.
Will apply the changes.
Nice catch and optimization with the vlen size.

Cupertino

David Faust writes:

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