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

[Mike Leach <mike.leach@linaro.org>]

Hi James

On Thu, 8 Aug 2024 at 10:32, James Clark <james.clark@linaro.org> wrote:
>
>
>
> On 07/08/2024 5:48 pm, Leo Yan wrote:
> > 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.
>
> 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.
>

Which address is going backwards here? - OpenCSD generates trace
ranges only by walking forwards from the last known address till it
hits a branch. Unless this wraps round 0x000000 this will never result
in a backwards address as far as I can see.
Do you have an example dump with OpenCSD outputting a range packet
with backwards addresses?

Mike

> >
> >>
> >> 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.
>
> 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.



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK