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

[Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>]

On 08-08-2024 04:21 pm, James Clark wrote:
> 
> 
> 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.")
>

Thanks James and Leo for your comments.
I will send the V2 with the changes as discussed.

Thanks.
Ganapat