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

[Leo Yan <leo.yan@arm.com>]

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