Re: [PATCH 1/2] perf probe: Reset old content before processing the next event string
From: Li Huafei
Date: Mon Nov 11 2024 - 21:02:24 EST
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
>>
>>
>
>