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

[Leo Yan]

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