[PATCH] perf scripts python arm-cs-trace-disasm.py: Skip disasm if address continuity is broken
Ganapatrao Kulkarni
gankulkarni at os.amperecomputing.com
Thu Aug 8 04:14:46 PDT 2024
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
More information about the linux-arm-kernel
mailing list