Re: [PATCH V7 4/4] Adjust objdump start/end range per map pgoff parameter

[Steve Clevenger <scclevenger@os.amperecomputing.com>]

On 9/9/2024 1:33 PM, Leo Yan wrote:
> On 9/7/2024 12:20 AM, Steve Clevenger wrote:
> 
> [...]
> 
>>>>> diff --git a/tools/perf/scripts/python/arm-cs-trace-disasm.py b/tools/
>>>>> perf/scripts/python/arm-cs-trace-disasm.py
>>>>> index 7aff02d84ffb..a867e0db02b8 100755
>>>>> --- a/tools/perf/scripts/python/arm-cs-trace-disasm.py
>>>>> +++ b/tools/perf/scripts/python/arm-cs-trace-disasm.py
>>>>> @@ -187,6 +187,7 @@ def process_event(param_dict):
>>>>>          dso_start = get_optional(param_dict, "dso_map_start")
>>>>>          dso_end = get_optional(param_dict, "dso_map_end")
>>>>>          symbol = get_optional(param_dict, "symbol")
>>>>> +       map_pgoff = get_optional(param_dict, "map_pgoff")
>>>>
>>>>
>>>> I am concerned the two sentences below are inconsistence: one uses
>>>> 'start_addr + map_pgoff' and the other uses 'start_addr + int(map_pgoff)'.
>>>>
>>>
>>> Valid point. It's working fine as is, but how is it even working? I look
>>> at print_disam/read_disasm, and no type conversion is done in either
>>> call. The dso_start parameter for these calls is actually dso_vm_start
>>> which is the dso_start integer conversion.
> 
> I agreed with you. After reading code, I have same conclusion that we don't
> need to type conversion to int type.
> 
>> Python thinks the map_pgoff object is already an integer. For example,
>>         map_pgoff = get_optional(param_dict, "map_pgoff")
>>         print("%d" % map_pgoff.isdigit())
>>
>> AttributeError: 'int' object has no attribute 'isdigit'
>> Fatal Python error: handler_call die: problem in Python trace event handler
>>
>> Converting map_pgoff to a string works in the print statement.
>>
>> print("%d" % str(map_pgoff).isdigit())
>> 1
>>
>> I'm not sure, but it's possible get_optional() during some call had
>> converted it to '[unknown]' which would cause a problem.
>>
>> I can explicitly force to integer.
> 
> For backward compatibility, we can use below code:
> 
>    map_pgoff = get_optional(param_dict, "map_pgoff")
>    if (map_pgoff == '[unknown]'):
>        map_pgoff = 0
> 
> Then in the later flow, we should can always use "map_pgoff" as an int type.
> 
> I struggled for James reported issue.
> 
> The variables “map_pgoff,” “dso_start,” and “dso_end” are set together in the
> Python engine. All of them should be of type int if the DSO is found, or all
> should be ‘[unknown]’ if the DSO is missing. We have checked the types for
> “dso_start” and “dso_end”, and if either is ‘[unknown]’ the flow will directly
> bail out. Thus, in theory, “map_pgoff” will not cause trouble if it is an
> ‘[unknown]’ string.
> 
> One possibility is that James has applied your patches but has not built perf.
> So, the field “map_pgoff” is not passed from the Python engine, which will
> cause the reported error. I think the proposed above change can effectively
> avoid the error.
> 
> Thanks,
> Leo

I added that exact'[unknown]' check in last week. There's no need to
explicitly convert to an integer although Python doesn't complain when
about integer to integer conversions. It's probably just a no op. I'll
commit the change later today.

Steve