Message-ID: <be4fae8b-d49f-4a90-a617-1ac2b22a2356@linaro.org>
Date: Wed, 8 Oct 2025 15:11:09 +0100
From: James Clark <james.clark@...aro.org>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>,
 Ian Rogers <irogers@...gle.com>, 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 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.

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