Re: [PATCH] perf scripts python arm-cs-trace-disasm.py: Skip disasm if address continuity is broken

[Leo Yan <leo.yan@arm.com>]

Hi all,

On 8/7/2024 3:53 PM, James Clark wrote:

A minor suggestion: if the discussion is too long, please delete the
irrelevant message ;)

[...]

>> --- a/tools/perf/scripts/python/arm-cs-trace-disasm.py
>> +++ b/tools/perf/scripts/python/arm-cs-trace-disasm.py
>> @@ -257,6 +257,11 @@ def process_event(param_dict):
>>                  print("Stop address 0x%x is out of range [ 0x%x .. 0x%x
>> ] for dso %s" % (stop_addr, int(dso_start), int(dso_end), dso))
>>                  return
>>
>> +       if (stop_addr < start_addr):
>> +               if (options.verbose == True):
>> +                       print("Packet Dropped, Discontinuity detected
>> [stop_add:0x%x start_addr:0x%x ] for dso %s" % (stop_addr, start_addr,
>> dso))
>> +               return
>> +
> 
> I suppose my only concern with this is that it hides real errors and
> Perf shouldn't be outputting samples that go backwards. Considering that
> fixing this in OpenCSD and Perf has a much wider benefit I think that
> should be the ultimate goal. I'm putting this on my todo list for now
> (including Steve's merging idea).

In the perf's util/cs-etm.c file, it handles DISCONTINUITY with:

   case CS_ETM_DISCONTINUITY:
        /*
         * The trace is discontinuous, if the previous packet is
         * instruction packet, set flag PERF_IP_FLAG_TRACE_END
         * for previous packet.
         */
        if (prev_packet->sample_type == CS_ETM_RANGE)
                prev_packet->flags |= PERF_IP_FLAG_BRANCH |
                                      PERF_IP_FLAG_TRACE_END;

I am wandering if OpenCSD has passed the correct info so Perf decoder can
detect the discontinuity. If yes, then the flag 'PERF_IP_FLAG_TRACE_END' will
be set (it is a general flag in branch sample), then we can consider use it in
the python script to handle discontinuous data.

> 
> But in the mean time what about having a force option?
> 
>> +       if (stop_addr < start_addr):
>> +               if (options.verbose == True or not options.force):
>> +                       print("Packet Dropped, Discontinuity detected
>> [stop_add:0x%x start_addr:0x%x ] for dso %s" % (stop_addr, start_addr,
>> dso))
>> +               if (not options.force):
>> +                       return

If the stop address is less than the start address, it must be something
wrong. In this case, we can report a warning for discontinuity and directly
return (also need to save the `addr` into global variable for next parsing).

I prefer to not add force option for this case - eventually, this will consume
much time for reporting this kind of failure and need to root causing it. A
better way is we just print out the reasoning in the log and continue to dump.

Thanks,
Leo