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

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

Hi James,

On 19-07-2024 08:09 pm, James Clark wrote:
> 
> 
> On 19/07/2024 10:26 am, Ganapatrao Kulkarni wrote:
>> To generate the instruction tracing, script uses 2 contiguous packets
>> address range. If there a continuity brake due to discontiguous branch
>> address, it is required to reset the tracing and start tracing with the
>> new set of contiguous packets.
>>
>> Adding change to identify the break and complete the remaining tracing
>> of current packets and restart tracing from new set of packets, if
>> continuity is established.
>>
> 
> Hi Ganapatrao,
> 
> Can you add a before and after example of what's changed to the commit 
> message? It wasn't immediately obvious to me if this is adding missing 
> output, or it was correcting the tail end of the output that was 
> previously wrong.

It is adding tail end of the trace as well avoiding the segfault of the 
perf application. With out this change the perf segfaults with as below log


./perf script --script=python:./scripts/python/arm-cs-trace-disasm.py -- 
-d objdump -k ../../vmlinux -v $* > dump
objdump: error: the stop address should be after the start address
Traceback (most recent call last):
   File "./scripts/python/arm-cs-trace-disasm.py", line 271, in 
process_event
     print_disam(dso_fname, dso_vm_start, start_addr, stop_addr)
   File "./scripts/python/arm-cs-trace-disasm.py", line 105, in print_disam
     for line in read_disam(dso_fname, dso_start, start_addr, stop_addr):
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   File "./scripts/python/arm-cs-trace-disasm.py", line 99, in read_disam
     disasm_output = check_output(disasm).decode('utf-8').split('\n')
                     ^^^^^^^^^^^^^^^^^^^^
   File "/usr/lib64/python3.12/subprocess.py", line 466, in check_output
     return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   File "/usr/lib64/python3.12/subprocess.py", line 571, in run
     raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['objdump', '-d', '-z', 
'--start-address=0xffff80008125b758', 
'--stop-address=0xffff80008125a934', '../../vmlinux']' returned non-zero 
exit status 1.
Fatal Python error: handler_call_die: problem in Python trace event handler
Python runtime state: initialized

Current thread 0x0000ffffb05054e0 (most recent call first):
   <no Python frame>

Extension modules: perf_trace_context, systemd._journal, 
systemd._reader, systemd.id128, report._py3report, _dbus_bindings, 
problem._py3abrt (total: 7)
Aborted (core dumped)

> 
>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>> ---
>>   tools/perf/scripts/python/arm-cs-trace-disasm.py | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/tools/perf/scripts/python/arm-cs-trace-disasm.py 
>> b/tools/perf/scripts/python/arm-cs-trace-disasm.py
>> index d973c2baed1c..ad10cee2c35e 100755
>> --- a/tools/perf/scripts/python/arm-cs-trace-disasm.py
>> +++ b/tools/perf/scripts/python/arm-cs-trace-disasm.py
>> @@ -198,6 +198,10 @@ def process_event(param_dict):
>>           cpu_data[str(cpu) + 'addr'] = addr
>>           return
>> +    if (cpu_data.get(str(cpu) + 'ip') == None):
>> +        cpu_data[str(cpu) + 'ip'] = ip
>> +
> 
> Do you need to write into the global cpu_data here? Doesn't it get 
> overwritten after you load it back into 'prev_ip'

No, the logic is same as holding the addr of previous packet.
Saving the previous packet saved ip in to prev_ip before overwriting 
with the current packet.
> 
>    prev_ip = cpu_data[str(cpu) + 'ip']
> 
>    ... then ...
> 
>    # Record for previous sample packet
>    cpu_data[str(cpu) + 'addr'] = addr
>    cpu_data[str(cpu) + 'ip'] = stop_addr
> 
> Would a local variable not accomplish the same thing?

No, We need global to hold the ip of previous packet.
> 
>> +    prev_ip = cpu_data[str(cpu) + 'ip']
>>       if (options.verbose == True):
>>           print("Event type: %s" % name)
>> @@ -243,12 +247,18 @@ def process_event(param_dict):
>>       # Record for previous sample packet
>>       cpu_data[str(cpu) + 'addr'] = addr
>> +    cpu_data[str(cpu) + 'ip'] = stop_addr
>>       # 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
>> +    if (stop_addr < start_addr):
>> +        # Continuity of the Packets broken, set start_addr to previous
>> +        # packet ip to complete the remaining tracing of the address 
>> range.
>> +        start_addr = prev_ip
>> +
>>       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))
>>           return

Thanks,
Ganapat