Message-ID: <CAP-5=fVzOQv5OVk8mchZ_w11iHr02AB7c6jn1tKSE4pmHiq14A@mail.gmail.com>
Date: Wed, 8 Oct 2025 08:07:44 -0700
From: Ian Rogers <irogers@...gle.com>
To: James Clark <james.clark@...aro.org>
Cc: Dan Carpenter <dan.carpenter@...aro.org>, Arnaldo Carvalho de Melo <acme@...nel.org>, 
	Charlie Jenkins <charlie@...osinc.com>, Peter Zijlstra <peterz@...radead.org>, 
	Ingo Molnar <mingo@...hat.com>, Namhyung Kim <namhyung@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, Leo Yan <leo.yan@....com>, 
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] perf tests: Don't retest sections in "Object code reading"

On Wed, Oct 8, 2025 at 7:11 AM James Clark <james.clark@...aro.org> wrote:
> On 08/10/2025 2:39 pm, Dan Carpenter wrote:
> > On Wed, Oct 08, 2025 at 11:21:34AM +0100, James Clark wrote:
> >> On 08/10/2025 9:32 am, James Clark wrote:
> >>> On 07/10/2025 9:01 pm, Arnaldo Carvalho de Melo wrote:
> >>>> On Tue, Oct 07, 2025 at 10:10:12AM +0100, James Clark wrote:
> >>>>> On 06/10/2025 4:21 pm, Ian Rogers wrote:
> >>>>>> On Mon, Oct 6, 2025 at 6:11 AM James Clark
> >>>>>> <james.clark@...aro.org> wrote:
> >>>>>>> +       data = zalloc(sizeof(*data));
> >>>>>>> +       if (!data)
> >>>>>>> +               return true;
> >>>>
> >>>>>>> +       data->addr = addr;
> >>>>>>> +       strlcpy(data->path, path, sizeof(data->path));
> >>>>>> nit: perhaps strdup rather than having 4kb per tested_section.
> >>>>
> >>>>> Oh yeah that would have been better, not sure why I didn't do it
> >>>>> that way.
> >>>>> Although the max sections I saw was around 50, and it's usually
> >>>>> a lot less
> >>>>> so it's probably not worth the churn to change it now that
> >>>>> Arnaldo's applied
> >>>>> it?
> >>>>
> >>>> I see you submitted a patch for using strdup() and then there is a need
> >>>> for checking the strdup(), etc.
> >>>>
> >>>> Since at this point this is an improvement on a test and all is sitting
> >>>> in linux-next and the window is closing for v6.18, lets leave this for
> >>>> the next window, ok?
> >>>>
> >>>
> >>> Makes sense.
> >>>
> >>>> These would be good things for some tool to catch, before it gets sent,
> >>>> but that is another rabbit hole :-)
> >>>>
> >>>> Thanks,
> >>>>
> >>>> - Arnaldo
> >>>
> >>> Does Smatch work on Perf? I imagine it would catch this if it does. Or
> >>> just plain old cppcheck. I'll take a look.
> >>>
> >>> James
> >>>
> >>
> >> Smatch doesn't know about strdup and seems to be more focused on kernel so
> >> probably isn't a good fit.
> >>
> >
> > No one is going to write a check which tells people when to use a fixed
> > array vs when to allocate memory dynamically.  Those sorts of decisions
> > are too complicated.
> >
>
> I mean "doesn't know about strdup" in that it would have to know that
> it's an allocator and can return NULL which should be checked etc. Not
> that it should know about Ian's original suggestion to use strdup in the
> first place.

Ooh, is smatch smart about errptrs? perf is using the hashmap code
from libbpf and rather than doing the more conventional C thing of
setting errno and returning NULL from hashmap__new it encodes ENOMEM
as an errptr thereby breaking NULL tests. We seem to always forget
this and eye-ball checks of hashmap__new results being compared to
NULL are reasonable. For example this fix for the issue:
https://lore.kernel.org/lkml/20250917095422.60923-1-wangfushuai@baidu.com/
I believe in the kernel coccinelle will spot this but we don't have it
as part of the perf build.

I think to make the C compilers smart enough to catch this we need to
do something like:
struct ERRNO_OR(hashmap) hashmap__new(...)
where ERRNO_OR would be a macro to add a wrapping struct and then we'd
force users of the hashmap__new result to check if the value were
errno or a pointer.

Another source code analysis tool it'd be interesting to have on the
perf build would be clang-tidy.

Thanks,
Ian

> >> Cppcheck struggles with a lot of the kernely style that's used in Perf,
> >> especially the headers. We'd either have to silence a lot of the warnings,
> >> or start with almost no warnings enabled.
> >>
> >> It doesn't have a warning for usage of a malloc return value without NULL
> >> checking, so in this case it wouldn't be useful.
> >
> > In the kernel we care a lot about missing NULL checks but in userspace
> > it doesn't really matter in the same way...  It's easy enough to write
> > a check for this in Smatch.
> >
> >>
> >> I'm not 100% convinced that the effort of integrating one of these into the
> >> build system would be worth it. I know that once they're in, there would be
> >> constant maintenance of silencing false positives etc. And a lot of the
> >> warnings are more style or opinions about undefined behavior according to
> >> the standard, but aren't real based on what the compiler actually does.
> >>
> >
> > The thing about false positives is that people should just ignore all the
> > warnings except the new ones.  In the kernel, this works well because we
> > fix all the real bugs right away.  And if we haven't eventually someone
> > will look at the code again and after 10 years or so it all either gets
> > fixed or it doesn't matter.
> >
>
> This requires some infrastructure though. The easiest way I imagined it
> working would be more like how we have compiler warnings and -Werror
> currently.
>
> Not that we couldn't come up with some kind of infrastructure to track
> new errors. But I think it would be applied too sporadically to block
> people from sending a patch in the first place.
>
> >> As an example, cppcheck on code-reading.c with --enable=all gives 17
> >> portability, 11 style, 3 warning and 1 error outputs. Out of all of these
> >> only two of the warnings are significant, from commit 0f9ad973b095 ("perf
> >> tests code-reading: Handle change in objdump output from binutils >= 2.41 on
> >> riscv"):
> >>
> >>    token = strsep(&version, ".");
> >>    version_tmp = atoi(token);
> >>    if (token)
> >>        version_num += version_tmp * 100;
> >>
> >>    token = strsep(&version, ".");
> >>    version_tmp = atoi(token);
> >>    if (token)
> >>        version_num += version_tmp;
> >>
> >> "token" has already been passed to atoi() so can't be NULL in the if
> >> statement. I think the atoi() needs to come after the null check.
> >>
> >
> > Yep.  Smatch does cross function analysis so it would have caught that.
> > (I haven't tested).
> >
> > regards,
> > dan carpenter
> >
>
> I ran it on this file and it didn't say much. Although I don't know if
> I'm using it properly:
>
>    smatch --full-path -I. -I../include -I../lib/perf/include -Iutil -I../
>      arch/x86/include -I../lib tests/code-reading.c
>
>    tests/code-reading.c: note: in included file:
>
>    /usr/include/x86_64-linux-gnu//sys/param.h:93:10: warning:
> preprocessor token roundup redefined
>
>    tests/code-reading.c: note: in included file (through
> ../include/linux/kernel.h):
>    ../include/linux/math.h:17:9: this was the original definition
>
>

Powered by blists - more mailing lists