Re: [PATCH v3] perf script: perf script tests fails with segfault

[Adrian Hunter <adrian.hunter@intel.com>]

On 01/05/2025 02:13, Ian Rogers wrote:
> On Mon, Apr 28, 2025 at 11:52 PM Aditya Bodkhe <adityab1@linux.ibm.com> wrote:
>>
>> From: Aditya Bodkhe <aditya.b1@linux.ibm.com>
>>
>> perf script: pert script tests fails with segmentation fault as below:
>>
>>  92: perf script tests:
>>  --- start ---
>> test child forked, pid 103769
>> DB test
>> [ perf record: Woken up 1 times to write data ]
>> [ perf record: Captured and wrote 0.012 MB /tmp/perf-test-script.7rbftEpOzX/perf.data (9 samples) ]
>> /usr/libexec/perf-core/tests/shell/script.sh: line 35: 103780 Segmentation fault      (core dumped) perf script -i "${perfdatafile}" -s "${db_test}"
>>  --- Cleaning up ---
>>  ---- end(-1) ----
>>         92: perf script tests                                               : FAILED!
>>
>> Backtrace pointed to :
>>         #0  0x0000000010247dd0 in maps.machine ()
>>         #1  0x00000000101d178c in db_export.sample ()
>>         #2  0x00000000103412c8 in python_process_event ()
>>         #3  0x000000001004eb28 in process_sample_event ()
>>         #4  0x000000001024fcd0 in machines.deliver_event ()
>>         #5  0x000000001025005c in perf_session.deliver_event ()
>>         #6  0x00000000102568b0 in __ordered_events__flush.part.0 ()
>>         #7  0x0000000010251618 in perf_session.process_events ()
>>         #8  0x0000000010053620 in cmd_script ()
>>         #9  0x00000000100b5a28 in run_builtin ()
>>         #10 0x00000000100b5f94 in handle_internal_command ()
>>         #11 0x0000000010011114 in main ()
>>
>> Further investigation reveals that this occurs in the `perf script tests`,
>> because it uses `db_test.py` script. This script sets `perf_db_export_mode = True`.
>>
>> With `perf_db_export_mode` enabled, if a sample originates from a hypervisor,
>> perf doesn't set maps for "[H]" sample in the code. Consequently, `al->maps` remains NULL
>> when `maps__machine(al->maps)` is called from `db_export__sample`.
>>
>> As al->maps can be NULL in case of Hypervisor samples , use thread->maps
>> because even for Hypervisor sample, machine should exist.
>> If we don't have machine for some reason, return -1 to avoid segmentation fault.
>>
>> Reported-by: Disha Goel <disgoel@linux.ibm.com>
>> Signed-off-by: Aditya Bodkhe <aditya.b1@linux.ibm.com>
>> Suggested-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>> Changelog:
>> v2 -> v3:
>> This patch contains changes suggested by Adrian Hunter<adrian.hunter@intel.com> to add extra spaces
>> before ---start--- and ---end---
>> to be able to apply patch without any issues
>>
>> v1 -> v2:
>> The below patch contains changes suggested by Adrian Hunter<adrian.hunter@intel.com> to handle the segmentation fault ,
>> as well as initialisation of al.thread
>>
>>  tools/perf/util/db-export.c                           | 11 ++++++++---
>>  .../perf/util/scripting-engines/trace-event-python.c  |  2 +-
>>  2 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/perf/util/db-export.c b/tools/perf/util/db-export.c
>> index 50f916374d87..8f52e8cefcf3 100644
>> --- a/tools/perf/util/db-export.c
>> +++ b/tools/perf/util/db-export.c
>> @@ -181,7 +181,7 @@ static int db_ids_from_al(struct db_export *dbe, struct addr_location *al,
>>         if (al->map) {
>>                 struct dso *dso = map__dso(al->map);
>>
>> -               err = db_export__dso(dbe, dso, maps__machine(al->maps));
>> +               err = db_export__dso(dbe, dso, maps__machine(thread__maps(al->thread)));
> 
> Would it be better to add a new helper function to minimize the chance
> of a segv, like:
> ```
> struct machine *addr_location__machine(struct addr_location *al)
> {
>   struct machine *machine = NULL;
>   struct maps *maps = al->maps;
> 
>   if (maps)
>     machine = maps__machine(maps)
>   if (!machine && al->thread) {
>     maps = thread__maps(al->thread);
>     if (maps)
>       machine = maps__machine(maps)
>  }
>   return machine;
> }
> ```

Those things have already been checked in db_export__sample(),
and there will still be a segfault if there is no machine,
so it does not really help.

>>                 if (err)
>>                         return err;
>>                 *dso_db_id = dso__db_id(dso);
>> @@ -256,6 +256,7 @@ static struct call_path *call_path_from_sample(struct db_export *dbe,
>>                 al.map = map__get(node->ms.map);
>>                 al.maps = maps__get(thread__maps(thread));
>>                 al.addr = node->ip;
>> +               al.thread = thread__get(thread);
>>
>>                 if (al.map && !al.sym)
>>                         al.sym = dso__find_symbol(map__dso(al.map), al.addr);
>> @@ -358,14 +359,18 @@ int db_export__sample(struct db_export *dbe, union perf_event *event,
>>         };
>>         struct thread *main_thread;
>>         struct comm *comm = NULL;
>> -       struct machine *machine;
>> +       struct machine *machine = NULL;
>>         int err;
>>
>> +       if (thread__maps(thread))
>> +               machine = maps__machine(thread__maps(thread));
>> +       if (!machine)
>> +               return -1;
>> +
> 
> Perhaps the machine should be stashed somewhere like struct db_export ?

Different samples can be from different machines i.e. VMs.