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

[James Clark]

On 08/08/2024 8:42 am, Leo Yan wrote:
> On 8/8/2024 5:36 AM, Ganapatrao Kulkarni wrote:
>>
>> On 08-08-2024 12:50 am, Leo Yan wrote:
>>> On 8/7/2024 5:18 PM, Ganapatrao Kulkarni wrote:
>>>
>>>> Is below diff with force option looks good?
>>>>
>>>> diff --git a/tools/perf/scripts/python/arm-cs-trace-disasm.py
>>>> b/tools/perf/scripts/python/arm-cs-trace-disasm.py
>>>> index d973c2baed1c..efe34f308beb 100755
>>>> --- a/tools/perf/scripts/python/arm-cs-trace-disasm.py
>>>> +++ b/tools/perf/scripts/python/arm-cs-trace-disasm.py
>>>> @@ -36,7 +36,10 @@ option_list = [
>>>>                       help="Set path to objdump executable file"),
>>>>           make_option("-v", "--verbose", dest="verbose",
>>>>                       action="store_true", default=False,
>>>> -                   help="Enable debugging log")
>>>> +                   help="Enable debugging log"),
>>>> +       make_option("-f", "--force", dest="force",
>>>> +                   action="store_true", default=False,
>>>> +                   help="Force decoder to continue")
>>>>    ]
>>>>
>>>>    parser = OptionParser(option_list=option_list)
>>>> @@ -257,6 +260,12 @@ 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 or options.force):
>>>> +                       print("Packet Discontinuity detected [stop_add:0x%x start_addr:0x%x ] for dso %s" % (stop_addr, start_addr, dso))

The options.force for the print should be "options.verbose or not 
options.force" I think? You want to print the error until the user adds 
-f, then hide it. Unless verbose is on.

>>>> +               if (options.force):
>>>> +                       return

Oops I had this one the wrong way around in my example. This way is correct.

>>>
>>> I struggled a bit for the code - it is confused that force mode bails out
>>> and the non-force mode continues to run. I prefer to always bail out for
>>> the discontinuity case, as it is pointless to continue in this case.
>>
>> Kept bail out with force option since I though it is not good to hide
>> the error in normal use, otherwise we never able to notice this error in
>> the future and it becomes default hidden. Eventually this error should
>> be fixed.
> 
> As James said, the issue should be fixed in OpenCSD or Perf decoding flow.
> 
> Thus, perf tool should be tolerant errors - report warning and drop
> discontinuous samples. This would be easier for developers later if face
> the same issue, they don't need to spend time to locate issue and struggle
> for overriding the error.
> 
> If you prefer to use force option, it might be better to give reasoning and
> *suggestion* in one go, something like:
> 
>      if (stop_addr < start_addr):
>         print("Packet Discontinuity detected [stop_add:0x%x start_addr:0x%x ] for dso %s" % (stop_addr, start_addr, dso))
>         print("Use option '-f' following the script for force mode"
>         if (options.force)
>             return
> 
> Either way is fine for me. Thanks a lot for taking time on the issue.
> 
> Leo
>   

But your diff looks good Ganapat, I think send a patch with Leo's extra 
help message added and the first force flipped.