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

[James Clark <james.clark@linaro.org>]

On 08/08/2024 10:21 am, James Clark wrote:
> 
> 
> 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.

One other small detail about Leo's suggestion print out. Can you add an 
instruction of how to keep the warnings as well:

   print("Use option '-f' following the script for force mode. Add '-v' \
     to continue printing decode warnings.")