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

[James Clark]

On 19/08/2024 11:59 am, Mike Leach wrote:
> 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
> 

Hi Mike,

I tested the new OpenCSD and I don't see the error anymore in the
disassembly script. I'm not sure if we need to go any further and add
the backwards check, it looks like just a later symptom and the checks
that you've added already prevent it.

If you release a new version I can send the perf patch. I was going to
use these flags if that looks right to you? As far as I know that's the
set that can be always on and won't fail on bad hardware?

I also assumed that ETM4_OPFLG_PKTDEC_AA64_OPCODE_CHK can be given even
for etmv3 and it's just a nop?

diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
index e917985bbbe6..90967fd807e6 100644
--- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
+++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
@@ -685,9 +685,14 @@ cs_etm_decoder__create_etm_decoder(struct cs_etm_decoder_params *d_params,
                 return 0;
  
         if (d_params->operation == CS_ETM_OPERATION_DECODE) {
+               int decode_flags = OCSD_CREATE_FLG_FULL_DECODER;
+#ifdef OCSD_OPFLG_N_UNCOND_DIR_BR_CHK
+               decode_flags |= OCSD_OPFLG_N_UNCOND_DIR_BR_CHK | OCSD_OPFLG_CHK_RANGE_CONTINUE |
+                               ETM4_OPFLG_PKTDEC_AA64_OPCODE_CHK;
+#endif
                 if (ocsd_dt_create_decoder(decoder->dcd_tree,
                                            decoder->decoder_name,
-                                          OCSD_CREATE_FLG_FULL_DECODER,
+                                          decode_flags,
                                            trace_config, &csid))
                         return -1;

> On Fri, 9 Aug 2024 at 16:20, James Clark <james.clark at 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 at 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.
> 
> 
>