* Re: [PATCH] tools/lib/perf: Fix -Werror=alloc-size-larger-than in cpumap.c
2025-04-25 17:46 ` Arnaldo Carvalho de Melo
@ 2025-04-29 5:11 ` Likhitha Korrapati
2025-05-02 7:44 ` Mukesh Kumar Chaurasiya
2025-05-02 9:05 ` Likhitha Korrapati
2 siblings, 0 replies; 17+ messages in thread
From: Likhitha Korrapati @ 2025-04-29 5:11 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Athira Rajeev
Cc: Namhyung Kim, Jiri Olsa, Adrian Hunter, Ian Rogers,
open list:PERFORMANCE EVENTS SUBSYSTEM, linuxppc-dev,
Venkat Rao Bagalkote, Madhavan Srinivasan
Hi Arnaldo,
On 4/25/25 23:16, Arnaldo Carvalho de Melo wrote:
> On Fri, Apr 25, 2025 at 08:19:02PM +0530, Athira Rajeev wrote:
>>> On 14 Apr 2025, at 7:08 AM, Madhavan Srinivasan <maddy@linux.ibm.com> wrote:
>>> On 4/7/25 5:38 PM, Venkat Rao Bagalkote wrote:
>>>> On 07/04/25 12:10 am, Athira Rajeev wrote:
>>>>>> On 6 Apr 2025, at 10:04 PM, Likhitha Korrapati <likhitha@linux.ibm.com> wrote:
>
>>>>>> perf build break observed when using gcc 13-3 (FC39 ppc64le)
>>>>>> with the following error.
>
>>>>>> cpumap.c: In function 'perf_cpu_map__merge':
>>>>>> cpumap.c:414:20: error: argument 1 range [18446744069414584320, 18446744073709551614] exceeds maximum object size 9223372036854775807 [-Werror=alloc-size-larger-than=]
>>>>>> 414 | tmp_cpus = malloc(tmp_len * sizeof(struct perf_cpu));
>>>>>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>> In file included from cpumap.c:4:
>>>>>> /usr/include/stdlib.h:672:14: note: in a call to allocation function 'malloc' declared here
>>>>>> 672 | extern void *malloc (size_t __size) __THROW __attribute_malloc__
>>>>>> | ^~~~~~
>>>>>> cc1: all warnings being treated as errors
>
>>>>>> Error happens to be only in gcc13-3 and not in latest gcc 14.
>>>>>> Even though git-bisect pointed bad commit as:
>>>>>> 'commit f5b07010c13c ("libperf: Don't remove -g when EXTRA_CFLAGS are used")',
>>>>>> issue is with tmp_len being "int". It holds number of cpus and making
>>>>>> it "unsigned int" fixes the issues.
>
>>>>>> After the fix:
>
>>>>>> CC util/pmu-flex.o
>>>>>> CC util/expr-flex.o
>>>>>> LD util/perf-util-in.o
>>>>>> LD perf-util-in.o
>>>>>> AR libperf-util.a
>>>>>> LINK perf
>>>>>> GEN python/perf.cpython-312-powerpc64le-linux-gnu.so
>
>>>>>> Signed-off-by: Likhitha Korrapati <likhitha@linux.ibm.com>
>>>>> Looks good to me
>
>>>>> Reviewed-by: Athira Rajeev <atrajeev@linux.ibm.com>
>
>>>> Tested this patch on perf-tools-next repo, and this patch fixes the issue.
>
>>>> Tested-by: Venkat Rao Bagalkote <venkat88@linux.ibm.com>
>
>>> Arnaldo, Namhyung,
>
>>> can you consider pulling this fix? since it is breaking the build in gcc13-3 or
>>> if you have any comments do let us know.
>
> This isn't the only place in that file where this pattern exists:
>
> ⬢ [acme@toolbx perf-tools-next]$ grep malloc tools/lib/perf/cpumap.c
> cpus = malloc(sizeof(*cpus) + sizeof(struct perf_cpu) * nr_cpus);
> tmp_cpus = malloc(tmp_len * sizeof(struct perf_cpu));
> tmp_cpus = malloc(tmp_len * sizeof(struct perf_cpu));
> ⬢ [acme@toolbx perf-tools-next]$
>
>
> struct perf_cpu_map *perf_cpu_map__alloc(int nr_cpus)
> {
> RC_STRUCT(perf_cpu_map) *cpus;
> struct perf_cpu_map *result;
>
> if (nr_cpus == 0)
> return NULL;
>
> cpus = malloc(sizeof(*cpus) + sizeof(struct perf_cpu) * nr_cpus);
>
>
> int perf_cpu_map__merge(struct perf_cpu_map **orig, struct perf_cpu_map *other)
> {
> struct perf_cpu *tmp_cpus;
> int tmp_len;
> int i, j, k;
> struct perf_cpu_map *merged;
>
> if (perf_cpu_map__is_subset(*orig, other))
> return 0;
> if (perf_cpu_map__is_subset(other, *orig)) {
> perf_cpu_map__put(*orig);
> *orig = perf_cpu_map__get(other);
> return 0;
> }
>
> tmp_len = __perf_cpu_map__nr(*orig) + __perf_cpu_map__nr(other);
> tmp_cpus = malloc(tmp_len * sizeof(struct perf_cpu));
>
>
> struct perf_cpu_map *perf_cpu_map__intersect(struct perf_cpu_map *orig,
> struct perf_cpu_map *other)
> {
> struct perf_cpu *tmp_cpus;
> int tmp_len;
> int i, j, k;
> struct perf_cpu_map *merged = NULL;
>
> if (perf_cpu_map__is_subset(other, orig))
> return perf_cpu_map__get(orig);
> if (perf_cpu_map__is_subset(orig, other))
> return perf_cpu_map__get(other);
>
> tmp_len = max(__perf_cpu_map__nr(orig), __perf_cpu_map__nr(other));
> tmp_cpus = malloc(tmp_len * sizeof(struct perf_cpu));
>
> I'm trying to figure out why its only in perf_cpu_map__merge() that this
> triggers :-\
>
> 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.
>
> But wouldn't turning this into a calloc() be better?
I have tried using calloc() instead of malloc() and the issue still
exists even using calloc().
cpumap.c: In function ‘perf_cpu_map__merge’:
cpumap.c:414:20: error: argument 1 range [18446744071562067968,
18446744073709551615] exceeds maximum object size 9223372036854775807
[-Werror=alloc-size-larger-than=]
414 | tmp_cpus = calloc(tmp_len , sizeof(struct perf_cpu));
>
> Like:
>
> diff --git a/tools/lib/perf/cpumap.c b/tools/lib/perf/cpumap.c
> index 4454a5987570cfbc..99d21618a252ac0e 100644
> --- a/tools/lib/perf/cpumap.c
> +++ 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?
>
> - Arnaldo
I have tried using max and it worked. I am doing testing with this
change and will be posting a V2.
Thanks
Likhitha
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] tools/lib/perf: Fix -Werror=alloc-size-larger-than in cpumap.c
2025-04-25 17:46 ` Arnaldo Carvalho de Melo
2025-04-29 5:11 ` Likhitha Korrapati
@ 2025-05-02 7:44 ` Mukesh Kumar Chaurasiya
2025-05-13 21:13 ` Arnaldo Carvalho de Melo
2025-05-02 9:05 ` Likhitha Korrapati
2 siblings, 1 reply; 17+ messages in thread
From: Mukesh Kumar Chaurasiya @ 2025-05-02 7:44 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Athira Rajeev, Namhyung Kim, Jiri Olsa, Adrian Hunter, Ian Rogers,
open list:PERFORMANCE EVENTS SUBSYSTEM, Likhitha Korrapati,
linuxppc-dev, Venkat Rao Bagalkote, Madhavan Srinivasan
On Fri, Apr 25, 2025 at 02:46:43PM -0300, Arnaldo Carvalho de Melo wrote:
> On Fri, Apr 25, 2025 at 08:19:02PM +0530, Athira Rajeev wrote:
> > > On 14 Apr 2025, at 7:08 AM, Madhavan Srinivasan <maddy@linux.ibm.com> wrote:
> > > On 4/7/25 5:38 PM, Venkat Rao Bagalkote wrote:
> > >> On 07/04/25 12:10 am, Athira Rajeev wrote:
> > >>>> On 6 Apr 2025, at 10:04 PM, Likhitha Korrapati <likhitha@linux.ibm.com> wrote:
>
> > >>>> perf build break observed when using gcc 13-3 (FC39 ppc64le)
> > >>>> with the following error.
>
> > >>>> cpumap.c: In function 'perf_cpu_map__merge':
> > >>>> cpumap.c:414:20: error: argument 1 range [18446744069414584320, 18446744073709551614] exceeds maximum object size 9223372036854775807 [-Werror=alloc-size-larger-than=]
> > >>>> 414 | tmp_cpus = malloc(tmp_len * sizeof(struct perf_cpu));
> > >>>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >>>> In file included from cpumap.c:4:
> > >>>> /usr/include/stdlib.h:672:14: note: in a call to allocation function 'malloc' declared here
> > >>>> 672 | extern void *malloc (size_t __size) __THROW __attribute_malloc__
> > >>>> | ^~~~~~
> > >>>> cc1: all warnings being treated as errors
>
> > >>>> Error happens to be only in gcc13-3 and not in latest gcc 14.
> > >>>> Even though git-bisect pointed bad commit as:
> > >>>> 'commit f5b07010c13c ("libperf: Don't remove -g when EXTRA_CFLAGS are used")',
> > >>>> issue is with tmp_len being "int". It holds number of cpus and making
> > >>>> it "unsigned int" fixes the issues.
>
> > >>>> After the fix:
>
> > >>>> CC util/pmu-flex.o
> > >>>> CC util/expr-flex.o
> > >>>> LD util/perf-util-in.o
> > >>>> LD perf-util-in.o
> > >>>> AR libperf-util.a
> > >>>> LINK perf
> > >>>> GEN python/perf.cpython-312-powerpc64le-linux-gnu.so
>
> > >>>> Signed-off-by: Likhitha Korrapati <likhitha@linux.ibm.com>
> > >>> Looks good to me
>
> > >>> Reviewed-by: Athira Rajeev <atrajeev@linux.ibm.com>
>
> > >> Tested this patch on perf-tools-next repo, and this patch fixes the issue.
>
> > >> Tested-by: Venkat Rao Bagalkote <venkat88@linux.ibm.com>
>
> > > Arnaldo, Namhyung,
>
> > > can you consider pulling this fix? since it is breaking the build in gcc13-3 or
> > > if you have any comments do let us know.
>
> This isn't the only place in that file where this pattern exists:
>
> ⬢ [acme@toolbx perf-tools-next]$ grep malloc tools/lib/perf/cpumap.c
> cpus = malloc(sizeof(*cpus) + sizeof(struct perf_cpu) * nr_cpus);
> tmp_cpus = malloc(tmp_len * sizeof(struct perf_cpu));
> tmp_cpus = malloc(tmp_len * sizeof(struct perf_cpu));
> ⬢ [acme@toolbx perf-tools-next]$
>
>
> struct perf_cpu_map *perf_cpu_map__alloc(int nr_cpus)
> {
> RC_STRUCT(perf_cpu_map) *cpus;
> struct perf_cpu_map *result;
>
> if (nr_cpus == 0)
> return NULL;
>
> cpus = malloc(sizeof(*cpus) + sizeof(struct perf_cpu) * nr_cpus);
>
>
> int perf_cpu_map__merge(struct perf_cpu_map **orig, struct perf_cpu_map *other)
> {
> struct perf_cpu *tmp_cpus;
> int tmp_len;
> int i, j, k;
> struct perf_cpu_map *merged;
>
> if (perf_cpu_map__is_subset(*orig, other))
> return 0;
> if (perf_cpu_map__is_subset(other, *orig)) {
> perf_cpu_map__put(*orig);
> *orig = perf_cpu_map__get(other);
> return 0;
> }
>
> tmp_len = __perf_cpu_map__nr(*orig) + __perf_cpu_map__nr(other);
> tmp_cpus = malloc(tmp_len * sizeof(struct perf_cpu));
>
>
> struct perf_cpu_map *perf_cpu_map__intersect(struct perf_cpu_map *orig,
> struct perf_cpu_map *other)
> {
> struct perf_cpu *tmp_cpus;
> int tmp_len;
> int i, j, k;
> struct perf_cpu_map *merged = NULL;
>
> if (perf_cpu_map__is_subset(other, orig))
> return perf_cpu_map__get(orig);
> if (perf_cpu_map__is_subset(orig, other))
> return perf_cpu_map__get(other);
>
> tmp_len = max(__perf_cpu_map__nr(orig), __perf_cpu_map__nr(other));
> tmp_cpus = malloc(tmp_len * sizeof(struct perf_cpu));
>
> I'm trying to figure out why its only in perf_cpu_map__merge() that this
> triggers :-\
>
> 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.
>
> But wouldn't turning this into a calloc() be better?
>
> Like:
>
> diff --git a/tools/lib/perf/cpumap.c b/tools/lib/perf/cpumap.c
> index 4454a5987570cfbc..99d21618a252ac0e 100644
> --- a/tools/lib/perf/cpumap.c
> +++ 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?
>
> - Arnaldo
Isn't it better to use perf_cpu_map__nr. That should fix this problem.
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?
Regards,
Mukesh
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] tools/lib/perf: Fix -Werror=alloc-size-larger-than in cpumap.c
2025-05-02 7:44 ` Mukesh Kumar Chaurasiya
@ 2025-05-13 21:13 ` Arnaldo Carvalho de Melo
2025-05-13 22:12 ` Ian Rogers
2025-05-21 13:03 ` Likhitha Korrapati
0 siblings, 2 replies; 17+ messages in thread
From: Arnaldo Carvalho de Melo @ 2025-05-13 21:13 UTC (permalink / raw)
To: Mukesh Kumar Chaurasiya
Cc: Athira Rajeev, Namhyung Kim, Jiri Olsa, Adrian Hunter, Ian Rogers,
open list:PERFORMANCE EVENTS SUBSYSTEM, Likhitha Korrapati,
linuxppc-dev, Venkat Rao Bagalkote, Madhavan Srinivasan
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?
> 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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] tools/lib/perf: Fix -Werror=alloc-size-larger-than in cpumap.c
2025-05-13 21:13 ` Arnaldo Carvalho de Melo
@ 2025-05-13 22:12 ` Ian Rogers
2025-05-13 22:36 ` Ian Rogers
2025-05-21 13:03 ` Likhitha Korrapati
1 sibling, 1 reply; 17+ messages in thread
From: Ian Rogers @ 2025-05-13 22:12 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Mukesh Kumar Chaurasiya, Athira Rajeev, Namhyung Kim, Jiri Olsa,
Adrian Hunter, open list:PERFORMANCE EVENTS SUBSYSTEM,
Likhitha Korrapati, linuxppc-dev, Venkat Rao Bagalkote,
Madhavan Srinivasan
On Tue, May 13, 2025 at 2:13 PM Arnaldo Carvalho de Melo
<acme@kernel.org> 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?
>
> > 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 :-)
So I've tried to improve the naming but not vary the implementation
greatly - initially I was in the code fixing reference count checking
issues. There is an important distinction between "all" meaning a
range of CPUs like 0-15 on a 16 core/hyperthread system, and "any"
meaning the special "-1" value. It is possible to have a perf_cpu_map
to both be "all" and "any", iterating an empty perf_cpu_map has
strangely also meant the "any" and so the code isn't specific but
relies on these odd properties.
Anyway, I'm not sure on the implication of this with
malloc/calloc/unsigned... It would seem reasonable to me for
__perf_cpu_map__nr to return an unsigned number and to propagate that
to fix the new GCC issue.
Thanks,
Ian
> - Arnaldo
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] tools/lib/perf: Fix -Werror=alloc-size-larger-than in cpumap.c
2025-05-13 22:12 ` Ian Rogers
@ 2025-05-13 22:36 ` Ian Rogers
0 siblings, 0 replies; 17+ messages in thread
From: Ian Rogers @ 2025-05-13 22:36 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Mukesh Kumar Chaurasiya, Athira Rajeev, Namhyung Kim, Jiri Olsa,
Adrian Hunter, open list:PERFORMANCE EVENTS SUBSYSTEM,
Likhitha Korrapati, linuxppc-dev, Venkat Rao Bagalkote,
Madhavan Srinivasan
On Tue, May 13, 2025 at 3:12 PM Ian Rogers <irogers@google.com> wrote:
>
> On Tue, May 13, 2025 at 2:13 PM Arnaldo Carvalho de Melo
> <acme@kernel.org> 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?
> >
> > > 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 :-)
>
> So I've tried to improve the naming but not vary the implementation
> greatly - initially I was in the code fixing reference count checking
> issues. There is an important distinction between "all" meaning a
> range of CPUs like 0-15 on a 16 core/hyperthread system, and "any"
> meaning the special "-1" value. It is possible to have a perf_cpu_map
> to both be "all" and "any", iterating an empty perf_cpu_map has
> strangely also meant the "any" and so the code isn't specific but
> relies on these odd properties.
I just remembered, there was also a discussion with Adrian IIRC. I
wanted "all" to mean all possible CPUs on a system, and "online" to be
either the same or a subset of that, in case CPUs were offline. There
was a different view that the distinction between "all" and "online"
wasn't useful. So there is some confusion in the code in this regard
and some errors may exist when CPU cores are offline. There's
definitely work to make the API better.
Thanks,
Ian
> Anyway, I'm not sure on the implication of this with
> malloc/calloc/unsigned... It would seem reasonable to me for
> __perf_cpu_map__nr to return an unsigned number and to propagate that
> to fix the new GCC issue.
>
> Thanks,
> Ian
>
>
> > - Arnaldo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] tools/lib/perf: Fix -Werror=alloc-size-larger-than in cpumap.c
2025-05-13 21:13 ` Arnaldo Carvalho de Melo
2025-05-13 22:12 ` Ian Rogers
@ 2025-05-21 13:03 ` Likhitha Korrapati
2025-05-21 15:45 ` Ian Rogers
1 sibling, 1 reply; 17+ messages in thread
From: Likhitha Korrapati @ 2025-05-21 13:03 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Mukesh Kumar Chaurasiya
Cc: Athira Rajeev, Namhyung Kim, Jiri Olsa, Adrian Hunter, Ian Rogers,
open list:PERFORMANCE EVENTS SUBSYSTEM, linuxppc-dev,
Venkat Rao Bagalkote, Madhavan Srinivasan
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.
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] tools/lib/perf: Fix -Werror=alloc-size-larger-than in cpumap.c
2025-05-21 13:03 ` Likhitha Korrapati
@ 2025-05-21 15:45 ` Ian Rogers
2025-05-21 17:28 ` Likhitha Korrapati
0 siblings, 1 reply; 17+ messages in thread
From: Ian Rogers @ 2025-05-21 15:45 UTC (permalink / raw)
To: Likhitha Korrapati
Cc: Arnaldo Carvalho de Melo, Mukesh Kumar Chaurasiya, Athira Rajeev,
Namhyung Kim, Jiri Olsa, Adrian Hunter,
open list:PERFORMANCE EVENTS SUBSYSTEM, linuxppc-dev,
Venkat Rao Bagalkote, Madhavan Srinivasan
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
> 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
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] tools/lib/perf: Fix -Werror=alloc-size-larger-than in cpumap.c
2025-05-21 15:45 ` Ian Rogers
@ 2025-05-21 17:28 ` Likhitha Korrapati
2025-05-21 17:39 ` Ian Rogers
0 siblings, 1 reply; 17+ messages in thread
From: Likhitha Korrapati @ 2025-05-21 17:28 UTC (permalink / raw)
To: Ian Rogers
Cc: Arnaldo Carvalho de Melo, Mukesh Kumar Chaurasiya, Athira Rajeev,
Namhyung Kim, Jiri Olsa, Adrian Hunter,
open list:PERFORMANCE EVENTS SUBSYSTEM, linuxppc-dev,
Venkat Rao Bagalkote, Madhavan Srinivasan
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
>>
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] tools/lib/perf: Fix -Werror=alloc-size-larger-than in cpumap.c
2025-05-21 17:28 ` Likhitha Korrapati
@ 2025-05-21 17:39 ` Ian Rogers
0 siblings, 0 replies; 17+ messages in thread
From: Ian Rogers @ 2025-05-21 17:39 UTC (permalink / raw)
To: Likhitha Korrapati
Cc: Arnaldo Carvalho de Melo, Mukesh Kumar Chaurasiya, Athira Rajeev,
Namhyung Kim, Jiri Olsa, Adrian Hunter,
open list:PERFORMANCE EVENTS SUBSYSTEM, linuxppc-dev,
Venkat Rao Bagalkote, Madhavan Srinivasan
On Wed, May 21, 2025 at 10:28 AM Likhitha Korrapati
<likhitha@linux.ibm.com> wrote:
>
> 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.
Ok, why is that a fix? The function declarations are near identical
and perf_cpu_map__nr is implemented in terms of __perf_cpu_map__nr:
```
static int __perf_cpu_map__nr(const struct perf_cpu_map *cpus)
{
return RC_CHK_ACCESS(cpus)->nr;
}
int perf_cpu_map__nr(const struct perf_cpu_map *cpus)
{
return cpus ? __perf_cpu_map__nr(cpus) : 1;
}
```
My guess is that being static allows all of the code to be analyzed in
the compilation unit and thereby create the warning/error, your change
is just defeating the analysis. The analysis could easily kick in
again for Link Time Optimization. I'd prefer making these `__nr`
functions return `unsigned int` or size_t over changes like this.
Thanks,
Ian
> 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
> >>
> >
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] tools/lib/perf: Fix -Werror=alloc-size-larger-than in cpumap.c
2025-04-25 17:46 ` Arnaldo Carvalho de Melo
2025-04-29 5:11 ` Likhitha Korrapati
2025-05-02 7:44 ` Mukesh Kumar Chaurasiya
@ 2025-05-02 9:05 ` Likhitha Korrapati
2 siblings, 0 replies; 17+ messages in thread
From: Likhitha Korrapati @ 2025-05-02 9:05 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Athira Rajeev
Cc: Namhyung Kim, Jiri Olsa, Adrian Hunter, Ian Rogers,
open list:PERFORMANCE EVENTS SUBSYSTEM, linuxppc-dev,
Venkat Rao Bagalkote, Madhavan Srinivasan
Hi Arnaldo,
On 4/25/25 23:16, Arnaldo Carvalho de Melo wrote:
> On Fri, Apr 25, 2025 at 08:19:02PM +0530, Athira Rajeev wrote:
>>> On 14 Apr 2025, at 7:08 AM, Madhavan Srinivasan <maddy@linux.ibm.com> wrote:
>>> On 4/7/25 5:38 PM, Venkat Rao Bagalkote wrote:
>>>> On 07/04/25 12:10 am, Athira Rajeev wrote:
>>>>>> On 6 Apr 2025, at 10:04 PM, Likhitha Korrapati <likhitha@linux.ibm.com> wrote:
>
>>>>>> perf build break observed when using gcc 13-3 (FC39 ppc64le)
>>>>>> with the following error.
>
>>>>>> cpumap.c: In function 'perf_cpu_map__merge':
>>>>>> cpumap.c:414:20: error: argument 1 range [18446744069414584320, 18446744073709551614] exceeds maximum object size 9223372036854775807 [-Werror=alloc-size-larger-than=]
>>>>>> 414 | tmp_cpus = malloc(tmp_len * sizeof(struct perf_cpu));
>>>>>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>> In file included from cpumap.c:4:
>>>>>> /usr/include/stdlib.h:672:14: note: in a call to allocation function 'malloc' declared here
>>>>>> 672 | extern void *malloc (size_t __size) __THROW __attribute_malloc__
>>>>>> | ^~~~~~
>>>>>> cc1: all warnings being treated as errors
>
>>>>>> Error happens to be only in gcc13-3 and not in latest gcc 14.
>>>>>> Even though git-bisect pointed bad commit as:
>>>>>> 'commit f5b07010c13c ("libperf: Don't remove -g when EXTRA_CFLAGS are used")',
>>>>>> issue is with tmp_len being "int". It holds number of cpus and making
>>>>>> it "unsigned int" fixes the issues.
>
>>>>>> After the fix:
>
>>>>>> CC util/pmu-flex.o
>>>>>> CC util/expr-flex.o
>>>>>> LD util/perf-util-in.o
>>>>>> LD perf-util-in.o
>>>>>> AR libperf-util.a
>>>>>> LINK perf
>>>>>> GEN python/perf.cpython-312-powerpc64le-linux-gnu.so
>
>>>>>> Signed-off-by: Likhitha Korrapati <likhitha@linux.ibm.com>
>>>>> Looks good to me
>
>>>>> Reviewed-by: Athira Rajeev <atrajeev@linux.ibm.com>
>
>>>> Tested this patch on perf-tools-next repo, and this patch fixes the issue.
>
>>>> Tested-by: Venkat Rao Bagalkote <venkat88@linux.ibm.com>
>
>>> Arnaldo, Namhyung,
>
>>> can you consider pulling this fix? since it is breaking the build in gcc13-3 or
>>> if you have any comments do let us know.
>
> This isn't the only place in that file where this pattern exists:
>
> ⬢ [acme@toolbx perf-tools-next]$ grep malloc tools/lib/perf/cpumap.c
> cpus = malloc(sizeof(*cpus) + sizeof(struct perf_cpu) * nr_cpus);
> tmp_cpus = malloc(tmp_len * sizeof(struct perf_cpu));
> tmp_cpus = malloc(tmp_len * sizeof(struct perf_cpu));
> ⬢ [acme@toolbx perf-tools-next]$
>
>
> struct perf_cpu_map *perf_cpu_map__alloc(int nr_cpus)
> {
> RC_STRUCT(perf_cpu_map) *cpus;
> struct perf_cpu_map *result;
>
> if (nr_cpus == 0)
> return NULL;
>
> cpus = malloc(sizeof(*cpus) + sizeof(struct perf_cpu) * nr_cpus);
>
>
> int perf_cpu_map__merge(struct perf_cpu_map **orig, struct perf_cpu_map *other)
> {
> struct perf_cpu *tmp_cpus;
> int tmp_len;
> int i, j, k;
> struct perf_cpu_map *merged;
>
> if (perf_cpu_map__is_subset(*orig, other))
> return 0;
> if (perf_cpu_map__is_subset(other, *orig)) {
> perf_cpu_map__put(*orig);
> *orig = perf_cpu_map__get(other);
> return 0;
> }
>
> tmp_len = __perf_cpu_map__nr(*orig) + __perf_cpu_map__nr(other);
> tmp_cpus = malloc(tmp_len * sizeof(struct perf_cpu));
>
>
> struct perf_cpu_map *perf_cpu_map__intersect(struct perf_cpu_map *orig,
> struct perf_cpu_map *other)
> {
> struct perf_cpu *tmp_cpus;
> int tmp_len;
> int i, j, k;
> struct perf_cpu_map *merged = NULL;
>
> if (perf_cpu_map__is_subset(other, orig))
> return perf_cpu_map__get(orig);
> if (perf_cpu_map__is_subset(orig, other))
> return perf_cpu_map__get(other);
>
> tmp_len = max(__perf_cpu_map__nr(orig), __perf_cpu_map__nr(other));
> tmp_cpus = malloc(tmp_len * sizeof(struct perf_cpu));
>
> I'm trying to figure out why its only in perf_cpu_map__merge() that this
> triggers :-\
>
> 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.
>
> But wouldn't turning this into a calloc() be better?
>
> Like:
>
> diff --git a/tools/lib/perf/cpumap.c b/tools/lib/perf/cpumap.c
> index 4454a5987570cfbc..99d21618a252ac0e 100644
> --- a/tools/lib/perf/cpumap.c
> +++ 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?
>
> - Arnaldo
I am not sure if using max() is right incase of perf_cpu_map_merge as
this is a merge functionality. As this is summation we get a value
greater than the max of orig and other cpus. And this might be correct
in perf_cpu_map__intersect() but will cause issues in perf_cpu_map__marge().
Can you please eloborate if you meant this or there is something else I
missed.
I tried the following:
int perf_cpu_map__nr(const struct perf_cpu_map *cpus)
{
return cpus ? __perf_cpu_map__nr(cpus) : 1;
}
static int __perf_cpu_map__nr(const struct perf_cpu_map *cpus)
{
return RC_CHK_ACCESS(cpus)->nr;
}
This got introduced via commit 7d1b529f164d33ad4514b272bcec65036873d717
where it assumes cpu map as non-null.
And we are checking this non-null in perf_cpu_map__merge()
if (perf_cpu_map__is_subset(*orig, other))
return 0;
if (perf_cpu_map__is_subset(other, *orig)) {
perf_cpu_map__put(*orig);
*orig = perf_cpu_map__get(other);
return 0;
}
Using perf_cpu_map__nr instead of __perf_cpu_map__nr and this works as
it has a check.
--- 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;
Regards,
Likhitha.
^ permalink raw reply [flat|nested] 17+ messages in thread