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

[PATCH V7 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.

A zero start_addr is filtered to prevent output of dso address range
check failures. 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 | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 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..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")
 
 	cpu = sample["cpu"]
 	ip = sample["ip"]
@@ -243,9 +244,11 @@ def process_event(param_dict):
 	# Record for previous sample packet
 	cpu_data[str(cpu) + 'addr'] = 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)
+	# Filter out zero start_address. Optionally identify CS_ETM_TRACE_ON packet
+	# if start_addr=0 and stop_addr=4.
+	if (start_addr == 0):
+		if ((stop_addr == 4) and (options.verbose == True)):
+			print("CPU%d: CS_ETM_TRACE_ON packet is inserted" % cpu)
 		return
 
 	if (start_addr < int(dso_start) or start_addr > int(dso_end)):
@@ -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 + int(map_pgoff), stop_addr + int(map_pgoff)))
 
 	print_srccode(comm, param_dict, sample, symbol, dso)
-- 
2.44.0

[PATCH V7 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 V7 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 V7 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

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

[Steve Clevenger]

Changes in V7:
  - In arm-cs-trace-disasm.py, fix print message core dump resulting
    from mixed type arithmetic.
  - Modify CS_ETM_TRACE_ON filter to filter zero start_addr. The
    CS_ETM_TRACE_ON message is changed to print only in verbose mode.
  - Removed verbose mode only notification for start_addr/stop_addr
    outside of dso address range.

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     | 14 +++--
 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, 83 insertions(+), 10 deletions(-)

-- 
2.44.0

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

[Leo Yan]

On 9/5/24 23:28, Steve Clevenger wrote:
> 
> Extract map_pgoff parameter from the dictionary, and adjust start/end
> range passed to objdump based on the value.
> 
> A zero start_addr is filtered to prevent output of dso address range
> check failures. 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 | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 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..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)'.

Here about below code?

            map_pgoff_str = get_optional(param_dict, "map_pgoff")
	   if map_pgoff_str == "":
		map_pgoff = 0
            else:
		map_pgoff = int(map_pgoff_str)

With above change, 'map_pgoff' is an int type. As a result, the changes
below can simply add 'map_pgoff'.

With these changes, LGTM:

Reviewed-by: Leo Yan <leo.yan@arm.com>


>          cpu = sample["cpu"]
>          ip = sample["ip"]
> @@ -243,9 +244,11 @@ def process_event(param_dict):
>          # Record for previous sample packet
>          cpu_data[str(cpu) + 'addr'] = 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)
> +       # Filter out zero start_address. Optionally identify CS_ETM_TRACE_ON packet
> +       # if start_addr=0 and stop_addr=4.
> +       if (start_addr == 0):
> +               if ((stop_addr == 4) and (options.verbose == True)):
> +                       print("CPU%d: CS_ETM_TRACE_ON packet is inserted" % cpu)
>                  return
> 
>          if (start_addr < int(dso_start) or start_addr > int(dso_end)):
> @@ -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 + int(map_pgoff), stop_addr + int(map_pgoff)))

> 
>          print_srccode(comm, param_dict, sample, symbol, dso)
> --
> 2.44.0
> 

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

[Steve Clevenger]

On 9/6/2024 4:27 AM, Leo Yan wrote:
> 
> 
> On 9/5/24 23:28, Steve Clevenger wrote:
>>
>> Extract map_pgoff parameter from the dictionary, and adjust start/end
>> range passed to objdump based on the value.
>>
>> A zero start_addr is filtered to prevent output of dso address range
>> check failures. 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 | 14 +++++++++-----
>>   1 file changed, 9 insertions(+), 5 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..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.

> Here about below code?
> 
>            map_pgoff_str = get_optional(param_dict, "map_pgoff")
>        if map_pgoff_str == "":
>         map_pgoff = 0
>            else:
>         map_pgoff = int(map_pgoff_str)
> 
> With above change, 'map_pgoff' is an int type. As a result, the changes
> below can simply add 'map_pgoff'.

I propose:
	map_pgoff_str = get_optional(param_dict, "map_pgoff"
	if (map_pgoff_str.isdigit()):
	    map_pgoff = int(map_pgoff)
	else:
	    map_pgoff = 0

The int() conversions in the print() statement can then be removed.

Steve

> With these changes, LGTM:
> 
> Reviewed-by: Leo Yan <leo.yan@arm.com>
> 
> 
>>          cpu = sample["cpu"]
>>          ip = sample["ip"]
>> @@ -243,9 +244,11 @@ def process_event(param_dict):
>>          # Record for previous sample packet
>>          cpu_data[str(cpu) + 'addr'] = 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)
>> +       # Filter out zero start_address. Optionally identify
>> CS_ETM_TRACE_ON packet
>> +       # if start_addr=0 and stop_addr=4.
>> +       if (start_addr == 0):
>> +               if ((stop_addr == 4) and (options.verbose == True)):
>> +                       print("CPU%d: CS_ETM_TRACE_ON packet is
>> inserted" % cpu)
>>                  return
>>
>>          if (start_addr < int(dso_start) or start_addr > int(dso_end)):
>> @@ -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 + int(map_pgoff), stop_addr +
>> int(map_pgoff)))
> 
>>
>>          print_srccode(comm, param_dict, sample, symbol, dso)
>> -- 
>> 2.44.0
>>

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

[Steve Clevenger]

On 9/6/2024 10:27 AM, Steve Clevenger wrote:
> 
> 
> On 9/6/2024 4:27 AM, Leo Yan wrote:
>>
>>
>> On 9/5/24 23:28, Steve Clevenger wrote:
>>>
>>> Extract map_pgoff parameter from the dictionary, and adjust start/end
>>> range passed to objdump based on the value.
>>>
>>> A zero start_addr is filtered to prevent output of dso address range
>>> check failures. 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 | 14 +++++++++-----
>>>   1 file changed, 9 insertions(+), 5 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..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.

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.

Steve

> 
>> Here about below code?
>>
>>            map_pgoff_str = get_optional(param_dict, "map_pgoff")
>>        if map_pgoff_str == "":
>>         map_pgoff = 0
>>            else:
>>         map_pgoff = int(map_pgoff_str)
>>
>> With above change, 'map_pgoff' is an int type. As a result, the changes
>> below can simply add 'map_pgoff'.
> 
> I propose:
> 	map_pgoff_str = get_optional(param_dict, "map_pgoff"
> 	if (map_pgoff_str.isdigit()):
> 	    map_pgoff = int(map_pgoff)
> 	else:
> 	    map_pgoff = 0
> 
> The int() conversions in the print() statement can then be removed.
> 
> Steve
> 
>> With these changes, LGTM:
>>
>> Reviewed-by: Leo Yan <leo.yan@arm.com>
>>
>>
>>>          cpu = sample["cpu"]
>>>          ip = sample["ip"]
>>> @@ -243,9 +244,11 @@ def process_event(param_dict):
>>>          # Record for previous sample packet
>>>          cpu_data[str(cpu) + 'addr'] = 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)
>>> +       # Filter out zero start_address. Optionally identify
>>> CS_ETM_TRACE_ON packet
>>> +       # if start_addr=0 and stop_addr=4.
>>> +       if (start_addr == 0):
>>> +               if ((stop_addr == 4) and (options.verbose == True)):
>>> +                       print("CPU%d: CS_ETM_TRACE_ON packet is
>>> inserted" % cpu)
>>>                  return
>>>
>>>          if (start_addr < int(dso_start) or start_addr > int(dso_end)):
>>> @@ -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 + int(map_pgoff), stop_addr +
>>> int(map_pgoff)))
>>
>>>
>>>          print_srccode(comm, param_dict, sample, symbol, dso)
>>> -- 
>>> 2.44.0
>>>
> 
> _______________________________________________
> CoreSight mailing list -- coresight@lists.linaro.org
> To unsubscribe send an email to coresight-leave@lists.linaro.org

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

[Leo Yan]

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

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

[Steve Clevenger]

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