Re: [PATCH] tools/lib/perf: Fix -Werror=alloc-size-larger-than in cpumap.c

[Likhitha Korrapati <likhitha@linux.ibm.com>]

Hi Ian,

On 5/21/25 21:15, Ian Rogers wrote:
> On Wed, May 21, 2025 at 6:03 AM Likhitha Korrapati
> <likhitha@linux.ibm.com> wrote:
>>
>> Hi Arnaldo,
>>
>> On 5/14/25 02:43, Arnaldo Carvalho de Melo wrote:
>>> On Fri, May 02, 2025 at 01:14:32PM +0530, Mukesh Kumar Chaurasiya wrote:
>>>> On Fri, Apr 25, 2025 at 02:46:43PM -0300, Arnaldo Carvalho de Melo wrote:
>>>>> Maybe that max() call in perf_cpu_map__intersect() somehow makes the
>>>>> compiler happy.
>>>
>>>>> And in perf_cpu_map__alloc() all calls seems to validate it.
>>>
>>>>> Like:
>>>
>>>>> +++ b/tools/lib/perf/cpumap.c
>>>>> @@ -411,7 +411,7 @@ int perf_cpu_map__merge(struct perf_cpu_map **orig, struct perf_cpu_map *other)
>>>>>           }
>>>>>
>>>>>           tmp_len = __perf_cpu_map__nr(*orig) + __perf_cpu_map__nr(other);
>>>>> -       tmp_cpus = malloc(tmp_len * sizeof(struct perf_cpu));
>>>>> +       tmp_cpus = calloc(tmp_len, sizeof(struct perf_cpu));
>>>>>           if (!tmp_cpus)
>>>>>                   return -ENOMEM;
>>>
>>>>> ⬢ [acme@toolbx perf-tools-next]$
>>>
>>>>> And better, do the max size that the compiler is trying to help us
>>>>> catch?
>>>
>>>> Isn't it better to use perf_cpu_map__nr. That should fix this problem.
>>>
>>> Maybe, have you tried it?
>>
>> I have tried this method and it works.
>>
>> --- a/tools/lib/perf/cpumap.c
>> +++ b/tools/lib/perf/cpumap.c
>> @@ -410,7 +410,7 @@ int perf_cpu_map__merge(struct perf_cpu_map **orig,
>> struct perf_cpu_map *other)
>>                   return 0;
>>           }
>>
>> -       tmp_len = max(__perf_cpu_map__nr(*orig), __perf_cpu_map__nr(other));
>> +       tmp_len = perf_cpu_map__nr(*orig) +  perf_cpu_map__nr(other);
>>           tmp_cpus = malloc(tmp_len * sizeof(struct perf_cpu));
>>           if (!tmp_cpus)
>>                   return -ENOMEM;
>>
>> I will send a V2 with this change if this looks good.
> 
> How is this different from the existing code:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/lib/perf/cpumap.c?h=perf-tools-next#n423
> ```
>          tmp_len = __perf_cpu_map__nr(*orig) + __perf_cpu_map__nr(other);
>          tmp_cpus = malloc(tmp_len * sizeof(struct perf_cpu));
>          if (!tmp_cpus)
>                  return -ENOMEM;
> ```
> 
> Thanks,
> Ian

I gave the wrong diff. Here is the corrected diff.

--- a/tools/lib/perf/cpumap.c
+++ b/tools/lib/perf/cpumap.c
@@ -410,7 +410,7 @@ int perf_cpu_map__merge(struct perf_cpu_map **orig, 
struct perf_cpu_map *other)
                 return 0;
         }

-       tmp_len = __perf_cpu_map__nr(*orig) + __perf_cpu_map__nr(other);
+       tmp_len = perf_cpu_map__nr(*orig) + perf_cpu_map__nr(other);
         tmp_cpus = malloc(tmp_len * sizeof(struct perf_cpu));
         if (!tmp_cpus)
                 return -ENOMEM;


I am using perf_cpu_map__nr instead of __perf_cpu_map__nr.

Thanks,
Likhitha.

> 
>> Thanks
>> Likhitha.
>>
>>>
>>>> One question I have, in perf_cpu_map__nr, the function is returning
>>>> 1 in case *cpus is NULL. Is it ok to do that? wouldn't it cause problems?
>>>
>>> Indeed this better be documented, as by just looking at:
>>>
>>> int perf_cpu_map__nr(const struct perf_cpu_map *cpus)
>>> {
>>>           return cpus ? __perf_cpu_map__nr(cpus) : 1;
>>> }
>>>
>>> It really doesn't make much sense to say that a NULL map has one entry.
>>>
>>> But the next functions are:
>>>
>>> bool perf_cpu_map__has_any_cpu_or_is_empty(const struct perf_cpu_map *map)
>>> {
>>>           return map ? __perf_cpu_map__cpu(map, 0).cpu == -1 : true;
>>> }
>>>
>>> bool perf_cpu_map__is_any_cpu_or_is_empty(const struct perf_cpu_map *map)
>>> {
>>>           if (!map)
>>>                   return true;
>>>
>>>           return __perf_cpu_map__nr(map) == 1 && __perf_cpu_map__cpu(map, 0).cpu == -1;
>>> }
>>>
>>> bool perf_cpu_map__is_empty(const struct perf_cpu_map *map)
>>> {
>>>           return map == NULL;
>>> }
>>>
>>> So it seems that a NULL cpu map means "any/all CPU) and a map with just
>>> one entry would have as its content "-1" that would mean "any/all CPU".
>>>
>>> Ian did work on trying to simplify/clarify this, so maybe he can chime
>>> in :-)
>>>
>>> - Arnaldo
>>
>