[PATCH V6 4/4] Adjust objdump start/end range per map pgoff parameter

[Steve Clevenger]

On 9/5/2024 6:45 AM, James Clark wrote:
> 
> 
> On 05/09/2024 1:11 am, Steve Clevenger wrote:
>> Extract map_pgoff parameter from the dictionary, and adjust start/end
>> range passed to objdump based on the value.
>>
>> The start_addr/stop_addr address checks are changed to print a warning
>> only if verbose == True. This script repeatedly sees a zero value passed
>> in for
>>       start_addr = cpu_data[str(cpu) + 'addr']
>>
>> These zero values are not a new problem. The start_addr/stop_addr warning
>> clutters the instruction trace output, hence this change.
>>
>> Signed-off-by: Steve Clevenger <scclevenger at os.amperecomputing.com>
>> ---
>>   tools/perf/scripts/python/arm-cs-trace-disasm.py | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/perf/scripts/python/arm-cs-trace-disasm.py b/tools/
>> perf/scripts/python/arm-cs-trace-disasm.py
>> index 7aff02d84ffb..35a2ab60ca12 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")
>>         cpu = sample["cpu"]
>>       ip = sample["ip"]
>> @@ -249,11 +250,13 @@ def process_event(param_dict):
>>           return
>>         if (start_addr < int(dso_start) or start_addr > int(dso_end)):
>> -        print("Start address 0x%x is out of range [ 0x%x .. 0x%x ]
>> for dso %s" % (start_addr, int(dso_start), int(dso_end), dso))
>> +        if (options.verbose == True):
>> +            print("Start address 0x%x is out of range [ 0x%x ..
>> 0x%x ] for dso %s" % (start_addr, int(dso_start), int(dso_end), dso))
>>           return
>>         if (stop_addr < int(dso_start) or stop_addr > int(dso_end)):
>> -        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))
>> +        if (options.verbose == True):
>> +            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))
> 
> What about skipping printing if the address is 0 rather than if it's not
> verbose? Seems like a zero is expected because that's for
> discontinuities, but other non zero addresses that don't match are a
> genuine issue.
> 
> And there is already an early exit for zero addresses above, maybe that
> just needs to be fixed to make it more general. But this one probably
> does need the verbosity change:
> 
>   # Handle CS_ETM_TRACE_ON packet if start_addr=0 and stop_addr=4
>   if (start_addr == 0 and stop_addr == 4):
>     print("CPU%d: CS_ETM_TRACE_ON packet is inserted" % cpu)
>     return
> 
> Also minor nit, but I think this change in verbosity is a separate
> change to the change to add the map offset.

In my experience, non-zero start addresses outside the dso address range
are exceedingly rare. I don't recall the last time I saw one. To avoid
having to pick through thousands of generated messages with zero start
addresses, I'll filter these out.

I also don't recall ever seeing the CS_ETM_TRACE_ON notification message
embedded in the instruction trace output. And a typical trace capture
has many thousands of TRACE_ON packets. I only have Ampere HW to use for
CoreSight trace, but how valid is the test with start_addr=0 and
stop_addr=4? I'll change the script to optionally print the message in
verbose mode for these conditions.

> 
>>           return
>>         if (options.objdump_name != None):
>> @@ -262,13 +265,14 @@ def process_event(param_dict):
>>           # vm_start to zero.
>>           if (dso == "[kernel.kallsyms]" or dso_start == 0x400000):
>>               dso_vm_start = 0
>> +            map_pgoff = 0
>>           else:
>>               dso_vm_start = int(dso_start)
>>             dso_fname = get_dso_file_path(dso, dso_bid)
>>           if path.exists(dso_fname):
>> -            print_disam(dso_fname, dso_vm_start, start_addr, stop_addr)
>> +            print_disam(dso_fname, dso_vm_start, start_addr +
>> map_pgoff, stop_addr + map_pgoff)
>>           else:
>> -            print("Failed to find dso %s for address range [ 0x%x ..
>> 0x%x ]" % (dso, start_addr, stop_addr))
>> +            print("Failed to find dso %s for address range [ 0x%x ..
>> 0x%x ]" % (dso, start_addr + map_pgoff, stop_addr + map_pgoff))
> 
> I get this error here for this line:
> 
>   $ perf script -k vmlinux -s python:tools/perf/scripts/python/arm-cs\
>      -trace-disasm.py -- -d -k vmlinux
> 
>   Traceback (most recent call last):
>   File "tools/perf/scripts/python/arm-cs-trace-disasm.py", line 331, in
>      process_event
>   print_disam(dso_fname, dso_vm_start, start_addr + map_pgoff, stop_addr
>     + map_pgoff)
>   TypeError: unsupported operand type(s) for +: 'int' and 'str'
>   Fatal Python error: handler_call_die: problem in Python trace event
>     handler
>   Python runtime state: initialized
> 
>   Current thread 0x000075db4c249400 (most recent call first):
>     <no Python frame>
> 
>   Extension modules: perf_trace_context, apt_pkg (total: 2)
>   Aborted (core dumped)

My mistake. I intermixed the types, and didn't catch it during test. I
Look for Version 7 to address your comments.

Steve C.