Re: [PATCH 1/2] perf probe: Reset old content before processing the next event string

[Li Huafei]

On 2024/11/11 12:47, Masami Hiramatsu (Google) wrote:
> On Sat, 9 Nov 2024 02:19:08 +0800
> Li Huafei <lihuafei1@xxxxxxxxxx> wrote:
> 
>> I added two probe events:
>>
>>   # perf probe -f -a schedule+8
>>   Added new event:
>>     probe:schedule       (on schedule+8)
>>
>>   You can now use it in all perf tools, such as:
>>
>>           perf record -e probe:schedule -aR sleep 1
>>
>>   # perf probe -f -a schedule+20
>>   Added new event:
>>     probe:schedule_1     (on schedule+20)
>>
>>   You can now use it in all perf tools, such as:
>>
>>           perf record -e probe:schedule_1 -aR sleep 1
>>
>> However, 'perf probe -l' shows the same offset:
>>
>>   # perf probe -l
>>     probe:schedule       (on schedule+8@kernel/sched/core.c)
>>     probe:schedule_1     (on schedule+8@kernel/sched/core.c)
>>
>> __show_perf_probe_events() does not clean up the 'pev' content when
>> parsing the rawlist. If the 'pev->offset' is not set while processing
>> the next probe event string, the offset value of the previous event will
>> be used.  After adding debug information, it was found that indeed there
>> was line number information when processing 'probe:schedule_1', so the
>> offset was not set and used the offset from 'probe:schedule'.
>>
>> To fix this, in the loop that parses the rawlist, reset the contents of
>> 'tev' and 'pev' to ensure it does not affect the next parsing.
>>
>> After the modification,  'perf probe -l' shows the following:
>>
>>   # perf probe -l
>>     probe:schedule       (on schedule+8@kernel/sched/core.c)
>>     probe:schedule_1     (on schedule:-6751@kernel/sched/core.c)
>>
>> Note that 'probe:schedule_1' is displayed with line number, but the line
>> number seem to be incorrect. This issue is independent of the problem
>> fixed by the current patch and will be addressed in the next patch.
>>
> 
> Good catch! But we should do the cleanup in clear_perf_probe_event()
> and clear_probe_trace_event().
> 
>> Fixes: d8f9da240495 ("perf tools: Use zfree() where applicable")
> 
> What we need is to revert this change for above 2 functions, because
> without that, it "clear"s the structure correctly. Current code
> releases allocated fields, but not clear all fields. This can lead
> another bug.


Right, I will make this change in the next version.

Thanks，
Huafei

> 
> Thank you,
> 
>> Signed-off-by: Li Huafei <lihuafei1@xxxxxxxxxx>
>> ---
>>  tools/perf/util/probe-event.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>> index a17c9b8a7a79..ec0b11f8d881 100644
>> --- a/tools/perf/util/probe-event.c
>> +++ b/tools/perf/util/probe-event.c
>> @@ -2695,6 +2695,8 @@ static int __show_perf_probe_events(int fd, bool is_kprobe,
>>  next:
>>  		clear_perf_probe_event(&pev);
>>  		clear_probe_trace_event(&tev);
>> +		memset(&tev, 0, sizeof(tev));
>> +		memset(&pev, 0, sizeof(pev));
>>  		if (ret < 0)
>>  			break;
>>  	}
>> -- 
>> 2.25.1
>>
>>
> 
>