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

PATCH V5 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 | 9 ++++++---
 1 file changed, 6 insertions(+), 3 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..e8cf5d80d850 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):
@@ -267,7 +270,7 @@ def process_event(param_dict):
 
 		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))
 
-- 
2.25.1

[PATCH V5 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>
---
 .../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.25.1

[PATCH V5 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>
---
 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.25.1

PATCH V5 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>
---
 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.25.1

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

[Steve Clevenger]

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     |  9 ++-
 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, 80 insertions(+), 8 deletions(-)

-- 
2.25.1

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

[Leo Yan]

On 8/29/2024 12:17 AM, Steve Clevenger wrote:
> 
> 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.

For the series:

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

Hi Steve,

Later when you respin a new version patches, it is better to amend the review
and ACK tags you have received. (Just remind, b4 is your friend for helping do
these things).

Thanks,
Leo

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

[Steve Clevenger]

On 8/29/2024 12:55 AM, Leo Yan wrote:
> On 8/29/2024 12:17 AM, Steve Clevenger wrote:
>>
>> 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.
> 
> For the series:
> 
> Reviewed-by: Leo Yan <leo.yan@arm.com>
> 
> Hi Steve,
> 
> Later when you respin a new version patches, it is better to amend the review
> and ACK tags you have received. (Just remind, b4 is your friend for helping do
> these things).
> 
> Thanks,
> Leo

Got it. Thanks, Leo. By the way, I noticed when I rebased to
perf-tools-next the kernel version was 6.11-rc3. Is there any chance
this patch gets into 6.11?

Steve

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

[Leo Yan]

On 8/29/24 18:39, Steve Clevenger wrote:> 
> On 8/29/2024 12:55 AM, Leo Yan wrote:
>> On 8/29/2024 12:17 AM, Steve Clevenger wrote:
>>>
>>> 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.
>>
>> For the series:
>>
>> Reviewed-by: Leo Yan <leo.yan@arm.com>
>>
>> Hi Steve,
>>
>> Later when you respin a new version patches, it is better to amend the review
>> and ACK tags you have received. (Just remind, b4 is your friend for helping do
>> these things).
>>
>> Thanks,
>> Leo
> 
> Got it. Thanks, Leo. By the way, I noticed when I rebased to
> perf-tools-next the kernel version was 6.11-rc3. Is there any chance
> this patch gets into 6.11?

Linus' master branch now is 6.11-rc5, it is not far away from the 6.11.

I think we still have much chance to get it done before it. Added maintainers
in case any missing.

Thanks,
Leo

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

[Steve Clevenger]

On 8/30/2024 12:20 AM, Leo Yan wrote:
> On 8/29/24 18:39, Steve Clevenger wrote:>
>> On 8/29/2024 12:55 AM, Leo Yan wrote:
>>> On 8/29/2024 12:17 AM, Steve Clevenger wrote:
>>>>
>>>> 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.
>>>
>>> For the series:
>>>
>>> Reviewed-by: Leo Yan <leo.yan@arm.com>
>>>
>>> Hi Steve,
>>>
>>> Later when you respin a new version patches, it is better to amend
>>> the review
>>> and ACK tags you have received. (Just remind, b4 is your friend for
>>> helping do
>>> these things).
>>>
>>> Thanks,
>>> Leo
>>
>> Got it. Thanks, Leo. By the way, I noticed when I rebased to
>> perf-tools-next the kernel version was 6.11-rc3. Is there any chance
>> this patch gets into 6.11?
> 
> Linus' master branch now is 6.11-rc5, it is not far away from the 6.11.
> 
> I think we still have much chance to get it done before it. Added
> maintainers
> in case any missing.
> 
> Thanks,
> Leo

Hi Leo,

That's great. Unfortuantely, I need to submit a V6 for patch 4/4, which
is the arm-cs-trace-disasm.py script. Kernel instruction trace requires
zeroing map_pgoff. perf makes most offset decisions, but this one is in
the script. It's a one liner.

I know you're a b4 fan, but I've never used it. If I'm able to make
progress with it I'll use it to resubmit. Otherwise more of the same.

Steve

Steve

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

[Leo Yan]

Hi Steve,

On 8/31/2024 8:19 PM, Steve Clevenger wrote:

[...]

> Hi Leo,
> 
> That's great. Unfortuantely, I need to submit a V6 for patch 4/4, which
> is the arm-cs-trace-disasm.py script. Kernel instruction trace requires
> zeroing map_pgoff. perf makes most offset decisions, but this one is in
> the script. It's a one liner.

Agreed the kernel symbols should be zero map offset. 

Thanks for correcting this.

> I know you're a b4 fan, but I've never used it. If I'm able to make
> progress with it I'll use it to resubmit. Otherwise more of the same.

For using the b4 command:

$ b4 am <Message-ID>

You can find the message ID in the lore webpage, e.g. for this series,
in the cover letter link [1], click the 'raw' hyper link, then you can
fine the Message ID is:
cover.1724879699.git.scclevenger@os.amperecomputing.com

Run 'b4 am cover.1724879699.git.scclevenger@os.amperecomputing.com' for
downloading the patches (with automatic amending ACK and review tags).

As result, you will get the file:
v5_20240828_scclevenger_arm_cs_trace_disasm_py_perf_must_accommodate_non_zero_dso_text_offset.mbx

$ git am v5_20240828_scclevenger_arm_cs_trace_disasm_py_perf_must_accommodate_non_zero_dso_text_offset.mbx

Alternatively, if you are using new b4 version, you can directly use
the command 'b4 shazam <Message-ID>' for downloading series and applying
it on your local branch in one go.

I tried the b4 command for your series but failed, I can see you used `git
send-email` but the series was not discontinuous so b4 command cannot fetch
the whole series.

You can just work on you existed patches - don't forget to append my review
tags for patches 01 ~ 03. For the patch 04, after you send out I will review
it.

Thanks,
Leo    
  
[1] https://lore.kernel.org/linux-perf-users/7a2c53c0-0bac-4a53-824d-6efdff8ed3ce@os.amperecomputing.com/T/#mc76180d8076cf3414ba81e7c379cda93297aa4a9