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

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

Hi,

A new branch of OpenCSD is available - ocsd-consistency-checks-1.5.4-rc1

Testing I managed to do confirms the N atom on unconditional branches
appear to work. I do not have a test case for the range
discontinuities.

The checks are enabled using operation flags on decoder creation. See
the docs for details.

Mike

On Fri, 9 Aug 2024 at 16:20, James Clark <james.clark@linaro.org> wrote:
>
>
>
> On 09/08/2024 3:13 pm, Mike Leach wrote:
> > 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
> >
> The example I have I think is something like this:
>
> 1.  Start address / trace on
> 2.  E
> 3.  Output range
>      ...
> 4.  Periodic address update
>      ...
> 5.  E
> 6.  Output range
>
> If decode has gone wrong (but undetectably) between steps 1 and 3. Then
> the next steps still output a second range based on the last periodic
> address received. (I think it might not necessarily have to be a
> periodic address but could also be indirect address packet?). Perf
> converts the ranges into branch samples by taking the end of the first
> range and beginning of the second range. Then the disassembly script
> converts those samples into ranges again by taking the source and
> destination of the last two branch samples.
>
> The original issue that Ganapat saw was that the periodic address causes
> OpenCSD to put the source address of the second range somewhere before
> the first one, even though it didn't output a branch or discontinuity
> that would explain how it got there.
>
> But yes you're right the ranges themselves always go forwards from the
> point of view of their own start and end addresses.
>
> I thought it might be possible for OpenCSD to check against the last
> range output? Although I wasn't sure if maybe it's actually valid to do
> a backwards jump like that without the trace on/off packets with address
> filtering or something?
>
> The root cause is still the incorrect image, but I think this check
> along with the other direct branch check should make it pretty difficult
> for people to make the mistake.



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