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

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

On 8/8/24 10:32, James Clark wrote:

[...]

>>> 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.
> 
> No OpenCSD isn't passing the correct info here. Higher up in the thread
> I suggested an OpenCSD patch that makes it detect the error earlier and
> fixes the issue. It also needs to output a discontinuity when the
> address goes backwards. So two fixes and then the script works without
> modifications.

Great! Just remind, with the fixes above, we might still need to enhance the
script to consume the PERF_IP_FLAG_TRACE_END flag, this can allow the script
to be reliable.

[...]
  
>> 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.
> 
> But in this case we've identified all the known issues that would cause
> the script to fail and we can fix them in Perf and OpenCSD. There may
> not even be any more issues that will cause the script to fail in the
> future so there's no point in softening the error IMO. That will only
> hide future issues (of which there may be none) and make root causing
> harder when it hits some other tool.

It is fine for me - with friendly logs as discussed in other replies.

Thanks,
Leo