[PATCH V6 0/4] arm-cs-trace-disasm.py/perf must accommodate non-zero DSO text offset

[PATCH V6 0/4] arm-cs-trace-disasm.py/perf must accommodate non-zero DSO text offset

[Steve Clevenger]

Changes in V6:
  - In arm-cs-trace-disasm.py, zero map_pgoff for kernel files. Add
    map_pgoff to start/end address for dso not found message.
  - Added "Reviewed-by" trailer for patches 1-3 previously reviewed
    by Leo Yan in V4 and V5.

Changes in V5:
  - In symbol-elf.c, branch to exit_close label if open file.
  - In trace_event_python.c, correct indentation. set_sym_in_dict
    call parameter "map_pgoff" renamed as "addr_map_pgoff" to
    match local naming.

Changes in V4:
  - In trace-event-python.c, fixed perf-tools-next merge problem.

Changes in V3:
  - Rebased to linux-perf-tools branch.
  - Squash symbol-elf.c and symbol.h into same commit.
  - In map.c, merge dso__is_pie() call into existing if statement.
  - In arm-cs-trace-disasm.py, remove debug artifacts.

Changes in V2:
  - In dso__is_pie() (symbol-elf.c), Decrease indentation, add null pointer
    checks per Leo Yan review.
  - Updated mailing list distribution.

Steve Clevenger (4):
  Add dso__is_pie call to identify ELF PIE
  Force MAPPING_TYPE__IDENTIY for PIE
  Add map pgoff to python dictionary based on MAPPING_TYPE
  Adjust objdump start/end range per map pgoff parameter

 .../scripts/python/arm-cs-trace-disasm.py     | 12 ++--
 tools/perf/util/map.c                         |  4 +-
 .../scripting-engines/trace-event-python.c    | 13 +++-
 tools/perf/util/symbol-elf.c                  | 61 +++++++++++++++++++
 tools/perf/util/symbol.h                      |  1 +
 5 files changed, 82 insertions(+), 9 deletions(-)

-- 
2.44.0

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

[Steve Clevenger]

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@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))
 		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))
 
 	print_srccode(comm, param_dict, sample, symbol, dso)
-- 
2.44.0

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

[James Clark]

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@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.

>   		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)

Re: [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@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.

[PATCH V6 3/4] Add map pgoff to python dictionary based on MAPPING_TYPE

[Steve Clevenger]

Add map_pgoff parameter to python dictionary so it can be seen by the
python script, arm-cs-trace-disasm.py. map_pgoff is forced to zero in
the dictionary if file type is MAPPING_TYPE__IDENTITY. Otherwise, the
map_pgoff value is directly added to the dictionary.

Signed-off-by: Steve Clevenger <scclevenger@os.amperecomputing.com>
Reviewed-by: Leo Yan <leo.yan@arm.com>
---
 .../util/scripting-engines/trace-event-python.c     | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
index 6971dd6c231f..7b96504d5406 100644
--- a/tools/perf/util/scripting-engines/trace-event-python.c
+++ b/tools/perf/util/scripting-engines/trace-event-python.c
@@ -798,7 +798,8 @@ static int set_regs_in_dict(PyObject *dict,
 static void set_sym_in_dict(PyObject *dict, struct addr_location *al,
 			    const char *dso_field, const char *dso_bid_field,
 			    const char *dso_map_start, const char *dso_map_end,
-			    const char *sym_field, const char *symoff_field)
+			    const char *sym_field, const char *symoff_field,
+			    const char *map_pgoff)
 {
 	char sbuild_id[SBUILD_ID_SIZE];
 
@@ -814,6 +815,12 @@ static void set_sym_in_dict(PyObject *dict, struct addr_location *al,
 			PyLong_FromUnsignedLong(map__start(al->map)));
 		pydict_set_item_string_decref(dict, dso_map_end,
 			PyLong_FromUnsignedLong(map__end(al->map)));
+		if (al->map->mapping_type == MAPPING_TYPE__DSO)
+			pydict_set_item_string_decref(dict, map_pgoff,
+				PyLong_FromUnsignedLongLong(al->map->pgoff));
+		else
+			pydict_set_item_string_decref(dict, map_pgoff,
+				PyLong_FromUnsignedLongLong(0));
 	}
 	if (al->sym) {
 		pydict_set_item_string_decref(dict, sym_field,
@@ -900,7 +907,7 @@ static PyObject *get_perf_sample_dict(struct perf_sample *sample,
 	pydict_set_item_string_decref(dict, "comm",
 			_PyUnicode_FromString(thread__comm_str(al->thread)));
 	set_sym_in_dict(dict, al, "dso", "dso_bid", "dso_map_start", "dso_map_end",
-			"symbol", "symoff");
+			"symbol", "symoff", "map_pgoff");
 
 	pydict_set_item_string_decref(dict, "callchain", callchain);
 
@@ -925,7 +932,7 @@ static PyObject *get_perf_sample_dict(struct perf_sample *sample,
 			PyBool_FromLong(1));
 		set_sym_in_dict(dict_sample, addr_al, "addr_dso", "addr_dso_bid",
 				"addr_dso_map_start", "addr_dso_map_end",
-				"addr_symbol", "addr_symoff");
+				"addr_symbol", "addr_symoff", "addr_map_pgoff");
 	}
 
 	if (sample->flags)
-- 
2.44.0

[PATCH V6 2/4] Force MAPPING_TYPE__IDENTIY for PIE

[Steve Clevenger]

Use dso__is_pie() to check whether the DSO file is a Position
Independent Executable (PIE). If PIE, change the MAPPING_TYPE to
MAPPING_TYPE__IDENTITY so a zero map pgoff (text offset) is passed
into the script.

Signed-off-by: Steve Clevenger <scclevenger@os.amperecomputing.com>
Reviewed-by: Leo Yan <leo.yan@arm.com>
---
 tools/perf/util/map.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index e781c8d56a9a..c846faec177b 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -173,8 +173,8 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
 		assert(!dso__kernel(dso));
 		map__init(result, start, start + len, pgoff, dso, prot, flags);
 
-		if (anon || no_dso) {
-			map->mapping_type = MAPPING_TYPE__IDENTITY;
+		if (anon || no_dso || dso__is_pie(dso)) {
+			map__set_mapping_type(map, MAPPING_TYPE__IDENTITY);
 
 			/*
 			 * Set memory without DSO as loaded. All map__find_*
-- 
2.44.0

[PATCH V6 1/4] Add dso__is_pie call to identify ELF PIE

[Steve Clevenger]

Add dso__is_pie global to read the .dynamic section DT_FLAGS_1 entry for
the DF_1_PIE flag. This identifies position executable code.

Signed-off-by: Steve Clevenger <scclevenger@os.amperecomputing.com>
Reviewed-by: Leo Yan <leo.yan@arm.com>
---
 tools/perf/util/symbol-elf.c | 61 ++++++++++++++++++++++++++++++++++++
 tools/perf/util/symbol.h     |  1 +
 2 files changed, 62 insertions(+)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index e398abfd13a0..babe47976922 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -662,6 +662,67 @@ static int dso__synthesize_plt_got_symbols(struct dso *dso, Elf *elf,
 	return err;
 }
 
+/*
+ * Check dynamic section DT_FLAGS_1 for a Position Independent
+ * Executable (PIE).
+ */
+bool dso__is_pie(struct dso *dso)
+{
+	Elf *elf = NULL;
+	Elf_Scn *scn = NULL;
+	GElf_Ehdr ehdr;
+	GElf_Shdr shdr;
+	bool is_pie = false;
+	char dso_path[PATH_MAX];
+	int fd = -1;
+
+	if (!dso || (elf_version(EV_CURRENT) == EV_NONE))
+		goto exit;	// false
+
+	dso__build_id_filename(dso, dso_path, sizeof(dso_path), false);
+
+	fd = open(dso_path, O_RDONLY);
+
+	if (fd < 0) {
+		pr_debug("%s: cannot read cached %s.\n", __func__, dso_path);
+		goto exit;	// false
+	}
+
+	elf = elf_begin(fd, ELF_C_READ, NULL);
+	gelf_getehdr(elf, &ehdr);
+
+	if (ehdr.e_type == ET_DYN) {
+		Elf_Data *data;
+		GElf_Dyn *entry;
+		int n_entries = shdr.sh_size / sizeof(GElf_Dyn);
+
+		scn = elf_section_by_name(elf, &ehdr, &shdr, ".dynamic", NULL);
+		if (!scn)
+			goto exit_close;	// false
+
+		data = (Elf_Data *) elf_getdata(scn, NULL);
+		if (!data || !data->d_buf)
+			goto exit_close;	// false
+
+		// check DT_FLAGS_1
+		for (int i = 0; i < n_entries; i++) {
+			entry = ((GElf_Dyn *) data->d_buf) + i;
+			if (entry->d_tag == DT_FLAGS_1) {
+				if ((entry->d_un.d_val & DF_1_PIE) != 0) {
+					is_pie = true;
+					break;
+				}
+			}
+		} // end for
+	}
+
+exit_close:
+	elf_end(elf);
+	close(fd);
+exit:
+	return is_pie;
+}
+
 /*
  * We need to check if we have a .dynsym, so that we can handle the
  * .plt, synthesizing its symbols, that aren't on the symtabs (be it
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 3fb5d146d9b1..33ea2596ce31 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -127,6 +127,7 @@ void dso__insert_symbol(struct dso *dso,
 			struct symbol *sym);
 void dso__delete_symbol(struct dso *dso,
 			struct symbol *sym);
+bool dso__is_pie(struct dso *dso);
 
 struct symbol *dso__find_symbol(struct dso *dso, u64 addr);
 struct symbol *dso__find_symbol_nocache(struct dso *dso, u64 addr);
-- 
2.44.0