Skip to contentSkip to conversation

arm-cs-trace-disasm.py/perf must accommodate non-zero DSO text offset

PATCH Superseded Steve Clevenger 2 participants 8 messages Aug 20, 2024 – Aug 26, 2024 perf events

v1 of 11 Superseded Latest is v11 →

lore
Share & tools
lore
#0 lore raw

cover arm-cs-trace-disasm.py/perf must accommodate non-zero DSO text offset

From: "steve.c.clevenger.ampere" <[email protected]>

Fedora 37 distributed shared binary and executable mapped files show a zero text section offset. Starting with the Fedora 38 distribution, the shared binary and executable mapped files show a non-zero text section offset for some binaries. The text offset parameter is never passed into the arm-cs-trace-disasm.py script to allow the script to adjust the start/end address range passed to objdump. This adjustment is required to correctly offset into the dso text section. Not doing so results in an incorrect user instruction trace display for Fedora 38 (and later) user trace output.

steve.c.clevenger.ampere (5):
Add dso__is_pie call to identify ELF PIE
Add dso__is_pie prototype
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     | 17 +++++-
 tools/perf/util/map.c                         |  5 +-
 .../scripting-engines/trace-event-python.c    | 13 ++++-
 tools/perf/util/symbol-elf.c                  | 55 +++++++++++++++++++
 tools/perf/util/symbol.h                      |  1 +
 5 files changed, 85 insertions(+), 6 deletions(-)
Signature · 1 line
2.25.1
#1 lore raw

PATCH 1/5 Add dso__is_pie call to identify ELF PIE

From: "steve.c.clevenger.ampere" <[email protected]>

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.

---
 tools/perf/util/symbol-elf.c | 55 ++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)
tools/perf/util/symbol-elf.c +55 -0
index e398abfd13a0..1d4bd222b314 100644
@@ -662,6 +662,61 @@ 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))
+		return is_pie;	// 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);
+		return is_pie;	// false
+	}
+
+	elf = elf_begin(fd, ELF_C_READ, NULL);
+	gelf_getehdr(elf, &ehdr);
+
+	if (ehdr.e_type == ET_DYN) {
+		scn = elf_section_by_name(elf, &ehdr, &shdr, ".dynamic", NULL);
+		if (scn) {	// check DT_FLAGS_1
+			Elf_Data *data;
+			GElf_Dyn *entry;
+			int n_entries = shdr.sh_size / sizeof(GElf_Dyn);
+
+			data = (Elf_Data *) elf_getdata(scn, NULL);
+			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
+		}
+	}
+
+	elf_end(elf);
+	close(fd);
+
+	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
Signature · 1 line
2.25.1
Reviewed-by on v1 1/5
#2 lore raw

Re: [PATCH 1/5] Add dso__is_pie call to identify ELF PIE

Hi Steve,

Quoted 38 lines · Steve Clevenger

From: "steve.c.clevenger.ampere" <[email protected]>

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.

---
 tools/perf/util/symbol-elf.c | 55 ++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)
tools/perf/util/symbol-elf.c +20 -0
index e398abfd13a0..1d4bd222b314 100644
@@ -662,6 +662,61 @@ 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))
+		return is_pie;	// false
+
+	dso__build_id_filename(dso, dso_path, sizeof(dso_path), false);
+
+	fd = open(dso_path, O_RDONLY);

tools/perf/util/symbol-elf.c static int dso__synthesize_plt_got_symbols(struct dso *dso, Elf *elf, · line 694

Quoted 10 lines
tools/perf/util/symbol-elf.c +10 -0
+
+	if (fd < 0) {
+		pr_debug("%s: cannot read cached %s.\n", __func__, dso_path);
+		return is_pie;	// false
+	}
+
+	elf = elf_begin(fd, ELF_C_READ, NULL);
+	gelf_getehdr(elf, &ehdr);
+
+	if (ehdr.e_type == ET_DYN) {

The code looks good to me, just several nitpicks.

To avoid indentation, we can firstly check the failure case and directly exit for it.

        if (ehdr.e_type != ET_DYN)
                goto exit_elf_end;

tools/perf/util/symbol-elf.c static int dso__synthesize_plt_got_symbols(struct dso *dso, Elf *elf, · line 695

Quoted 1 line
tools/perf/util/symbol-elf.c +1 -0
+		scn = elf_section_by_name(elf, &ehdr, &shdr, ".dynamic", NULL);

Ditto.

        if (!scn)
                goto exit_elf_end;

tools/perf/util/symbol-elf.c static int dso__synthesize_plt_got_symbols(struct dso *dso, Elf *elf, · line 701

Quoted 6 lines
tools/perf/util/symbol-elf.c +6 -0
+		if (scn) {	// check DT_FLAGS_1
+			Elf_Data *data;
+			GElf_Dyn *entry;
+			int n_entries = shdr.sh_size / sizeof(GElf_Dyn);
+
+			data = (Elf_Data *) elf_getdata(scn, NULL);

For a safe code, it is good to check if pointers (data and data->d_buf) are valid before dereference them.

       if (!data || !data->d_buf)
               goto exit_elf_end;

With above changes:

Quoted 25 lines
tools/perf/util/symbol-elf.c +18 -0
+			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
+		}
+	}
+
+	elf_end(elf);
+	close(fd);
+
+	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
Signature · 1 line
2.25.1
#3 lore raw

Re: [PATCH 1/5] Add dso__is_pie call to identify ELF PIE

Quoted 77 lines · Leo Yan

Hi Steve,

Quoted 38 lines · Steve Clevenger

From: "steve.c.clevenger.ampere" <[email protected]>

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.

---
 tools/perf/util/symbol-elf.c | 55 ++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)
tools/perf/util/symbol-elf.c +20 -0
index e398abfd13a0..1d4bd222b314 100644
@@ -662,6 +662,61 @@ 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))
+		return is_pie;	// false
+
+	dso__build_id_filename(dso, dso_path, sizeof(dso_path), false);
+
+	fd = open(dso_path, O_RDONLY);

tools/perf/util/symbol-elf.c static int dso__synthesize_plt_got_symbols(struct dso *dso, Elf *elf, · line 694

Quoted 10 lines
tools/perf/util/symbol-elf.c +10 -0
+
+	if (fd < 0) {
+		pr_debug("%s: cannot read cached %s.\n", __func__, dso_path);
+		return is_pie;	// false
+	}
+
+	elf = elf_begin(fd, ELF_C_READ, NULL);
+	gelf_getehdr(elf, &ehdr);
+
+	if (ehdr.e_type == ET_DYN) {

The code looks good to me, just several nitpicks.

To avoid indentation, we can firstly check the failure case and directly exit for it.

        if (ehdr.e_type != ET_DYN)
                goto exit_elf_end;

tools/perf/util/symbol-elf.c static int dso__synthesize_plt_got_symbols(struct dso *dso, Elf *elf, · line 695

Quoted 1 line
tools/perf/util/symbol-elf.c +1 -0
+		scn = elf_section_by_name(elf, &ehdr, &shdr, ".dynamic", NULL);

Ditto.

        if (!scn)
                goto exit_elf_end;

tools/perf/util/symbol-elf.c static int dso__synthesize_plt_got_symbols(struct dso *dso, Elf *elf, · line 701

Quoted 6 lines
tools/perf/util/symbol-elf.c +6 -0
+		if (scn) {	// check DT_FLAGS_1
+			Elf_Data *data;
+			GElf_Dyn *entry;
+			int n_entries = shdr.sh_size / sizeof(GElf_Dyn);
+
+			data = (Elf_Data *) elf_getdata(scn, NULL);

For a safe code, it is good to check if pointers (data and data->d_buf) are valid before dereference them.

Quoted 4 lines
       if (!data || !data->d_buf)
               goto exit_elf_end;

With above changes:

Thanks Leo. I understand your comment about excess indentation, but I don't believe there's an excess here. Valid points about NULL pointer checks. I've made changes based on your review. Please look for V2 of this patch series. Besides addressing your comments, V2 is mostly to update the mailing lists.

Steve C.

Quoted 28 lines
Quoted 25 lines
tools/perf/util/symbol-elf.c +18 -0
+			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
+		}
+	}
+
+	elf_end(elf);
+	close(fd);
+
+	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
Signature · 1 line
2.25.1
#4 lore raw

PATCH 2/5 Add dso__is_pie prototype

In reply to Steve Clevenger

From: "steve.c.clevenger.ampere" <[email protected]>

Add prototype to dso__is_pie() global.

---
 tools/perf/util/symbol.h | 1 +
 1 file changed, 1 insertion(+)
tools/perf/util/symbol.h +1 -0
index 3fb5d146d9b1..33ea2596ce31 100644
@@ -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);
Signature · 1 line
2.25.1
#5 lore raw

PATCH 3/5 Force MAPPING_TYPE__IDENTIY for PIE

In reply to Steve Clevenger

From: "steve.c.clevenger.ampere" <[email protected]>

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.

---
 tools/perf/util/map.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
tools/perf/util/map.c +4 -1
index e1d14936a60d..df7c06fc373e 100644
@@ -171,8 +171,11 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
 		assert(!dso__kernel(dso));
 		map__init(result, start, start + len, pgoff, dso);
 
+		if (map->pgoff && !no_dso)
+			no_dso = dso__is_pie(dso);	// PIE check
+
 		if (anon || no_dso) {
-			map->mapping_type = MAPPING_TYPE__IDENTITY;
+			map__set_mapping_type(map, MAPPING_TYPE__IDENTITY);
 
 			/*
 			 * Set memory without DSO as loaded. All map__find_*
Signature · 1 line
2.25.1
#6 lore raw

PATCH 4/5 Add map pgoff to python dictionary based on MAPPING_TYPE

In reply to Steve Clevenger

From: "steve.c.clevenger.ampere" <[email protected]>

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.

---
 .../util/scripting-engines/trace-event-python.c     | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)
tools/perf/util/scripting-engines/trace-event-python.c +10 -3
index fb00f3ad6815..8a056c3574ec 100644
@@ -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,
@@ -898,7 +905,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);
 
@@ -923,7 +930,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", "map_pgoff");
 	}
 
 	if (sample->flags)
Signature · 1 line
2.25.1
#7 lore raw

PATCH 5/5 Adjust objdump start/end range per map pgoff parameter

In reply to Steve Clevenger

From: "steve.c.clevenger.ampere" <[email protected]>

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.

---
 .../perf/scripts/python/arm-cs-trace-disasm.py  | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)
tools/perf/scripts/python/arm-cs-trace-disasm.py +15 -2
index d973c2baed1c..6bf806078f9a 100755
@@ -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"]
@@ -250,13 +251,25 @@ 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 map_pgoff != None and map_pgoff != '[unknown]':
+		if (dso == "[kernel.kallsyms]"):
+			dso_vm_start = 0
+			map_pgoff = '[unknown]'
+		else:
+			dso_vm_start = int(dso_start)
+			start_addr += map_pgoff
+			stop_addr += map_pgoff
+			map_pgoff = '[unknown]'
+
 	if (options.objdump_name != None):
 		# It doesn't need to decrease virtual memory offset for disassembly
 		# for kernel dso and executable file dso, so in this case we set
Signature · 1 line
2.25.1