[PATCH v2 0/5] perf tools: Random fixes for the symbol code

[PATCH v2 0/5] perf tools: Random fixes for the symbol code

[Namhyung Kim]

Hello,

These are assorted random fixes found by Sashiko review during the last
perf dso/symbol code rework.

v2 changes)

 * add perf_env for completeness in the maps tests
 * convert to bfd_arch_bits_per_address for PE files

Thanks,
Namhyung


Namhyung Kim (5):
  perf test: Fix a memory leak in PE file parsing test
  perf test: Make sure maps to have a machine
  perf tools: Remove NULL check for maps__machine()
  perf symbol: Check result when reading section headers
  perf symbol: Ensure dso__set_is_64_bit() for libbfd

 tools/perf/tests/maps.c            | 30 +++++++++++++++++++++++++-----
 tools/perf/tests/pe-file-parsing.c | 13 ++++++++++---
 tools/perf/util/dlfilter.c         |  3 +--
 tools/perf/util/event.c            |  3 ---
 tools/perf/util/libbfd.c           |  1 +
 tools/perf/util/map.c              |  9 +++------
 tools/perf/util/python.c           |  2 +-
 tools/perf/util/symbol-elf.c       |  4 +++-
 8 files changed, 44 insertions(+), 21 deletions(-)

-- 
2.56.0

[PATCH v2 1/5] perf test: Fix a memory leak in PE file parsing test

[Namhyung Kim]

The get_argv_exec_path() returns a dynamically allocated string so it
should be freed.  Let's check the return value first so that it
doesn't crash snprintf().

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/r/sashiko-outbox-168781@kernel.org
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/pe-file-parsing.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/tools/perf/tests/pe-file-parsing.c b/tools/perf/tests/pe-file-parsing.c
index 190520ffc3ad6d0c..ed95aa5d8ca2af92 100644
--- a/tools/perf/tests/pe-file-parsing.c
+++ b/tools/perf/tests/pe-file-parsing.c
@@ -74,19 +74,26 @@ static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
 {
 	struct stat st;
 	char path_dir[PATH_MAX];
-	const char * const tests_dirs[] = {
+	static const char * const tests_dirs[] = {
 		"./tools/perf/tests",
 		"./tests",
 		"./source/tests",
-		path_dir,
 	};
-	snprintf(path_dir, PATH_MAX, "%s/tests", get_argv_exec_path());
+	char *exec_path;
 
 	for (size_t i = 0; i < ARRAY_SIZE(tests_dirs); ++i) {
 		if (!lstat(tests_dirs[i], &st))
 			return run_dir(tests_dirs[i]);
 	}
 
+	exec_path = get_argv_exec_path();
+	TEST_ASSERT_VAL("Failed to allocate exec_path", exec_path);
+	snprintf(path_dir, PATH_MAX, "%s/tests", exec_path);
+	free(exec_path);
+
+	if (!lstat(path_dir, &st))
+		return run_dir(path_dir);
+
 	pr_debug("cannot find the tests directory\n");
 	return TEST_SKIP;
 }
-- 
2.56.0

Re: [PATCH v2 1/5] perf test: Fix a memory leak in PE file parsing test

[sashiko-bot]

> The get_argv_exec_path() returns a dynamically allocated string so it
> should be freed.  Let's check the return value first so that it
> doesn't crash snprintf().
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/r/sashiko-outbox-168781@kernel.org
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019055207.39325-1-namhyung@kernel.org?part=1

[PATCH v2 2/5] perf test: Make sure maps to have a machine

[Namhyung Kim]

It's be easier if we could guarantee that maps always have a machine.
IIUC this is true for all real use cases, but only the test code has
an exception.  Let's get rid of it and make sure they have machines.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/maps.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/tools/perf/tests/maps.c b/tools/perf/tests/maps.c
index 2c05d62f40dcdf72..ff6d25f9c043034b 100644
--- a/tools/perf/tests/maps.c
+++ b/tools/perf/tests/maps.c
@@ -1,12 +1,14 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <inttypes.h>
+#include <unistd.h>
 #include <linux/compiler.h>
 #include <linux/kernel.h>
-#include "tests.h"
+#include "debug.h"
+#include "dso.h"
+#include "env.h"
 #include "map.h"
 #include "maps.h"
-#include "dso.h"
-#include "debug.h"
+#include "tests.h"
 
 struct map_def {
 	const char *name;
@@ -100,8 +102,15 @@ static int test__maps__merge_in(struct test_suite *t __maybe_unused, int subtest
 	};
 	struct map *map_kcore1, *map_kcore2, *map_kcore3;
 	int ret;
-	struct maps *maps = maps__new(NULL);
+	struct perf_env host_env;
+	struct machine *machine;
+	struct maps *maps;
 
+	perf_env__init(&host_env);
+	machine = machine__new_live(&host_env, /*kernel_maps=*/false, getpid());
+	maps = maps__new(machine);
+
+	TEST_ASSERT_VAL("failed to create machine", machine);
 	TEST_ASSERT_VAL("failed to create maps", maps);
 
 	for (i = 0; i < ARRAY_SIZE(bpf_progs); i++) {
@@ -159,6 +168,8 @@ static int test__maps__merge_in(struct test_suite *t __maybe_unused, int subtest
 	map__zput(map_kcore1);
 	map__zput(map_kcore2);
 	map__zput(map_kcore3);
+	machine__delete(machine);
+	perf_env__exit(&host_env);
 	return TEST_OK;
 }
 
@@ -189,8 +200,15 @@ static int test__maps__fixup_overlap_and_insert(struct test_suite *t __maybe_unu
 	struct map *map_split, *map_eclipse;
 	int ret;
 	unsigned int i;
-	struct maps *maps = maps__new(NULL);
+	struct perf_env host_env;
+	struct machine *machine;
+	struct maps *maps;
+
+	perf_env__init(&host_env);
+	machine = machine__new_live(&host_env, /*kernel_maps=*/false, getpid());
+	maps = maps__new(machine);
 
+	TEST_ASSERT_VAL("failed to create machine", machine);
 	TEST_ASSERT_VAL("failed to create maps", maps);
 
 	for (i = 0; i < ARRAY_SIZE(initial_maps); i++) {
@@ -230,6 +248,8 @@ static int test__maps__fixup_overlap_and_insert(struct test_suite *t __maybe_unu
 	TEST_ASSERT_VAL("eclipse check failed", !ret);
 
 	maps__zput(maps);
+	machine__delete(machine);
+	perf_env__exit(&host_env);
 	return TEST_OK;
 }
 
-- 
2.56.0

Re: [PATCH v2 2/5] perf test: Make sure maps to have a machine

[sashiko-bot]

> It's be easier if we could guarantee that maps always have a machine.
> IIUC this is true for all real use cases, but only the test code has
> an exception.  Let's get rid of it and make sure they have machines.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019055207.39325-1-namhyung@kernel.org?part=2

[PATCH v2 3/5] perf tools: Remove NULL check for maps__machine()

[Namhyung Kim]

Now maps should have a machine, let's get rid of checks for the return
value.  There are already many places to assume it returns non-NULL,
so having this kind of check could give confusion that it can return
NULL (which is not true anymore).

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/dlfilter.c | 3 +--
 tools/perf/util/event.c    | 3 ---
 tools/perf/util/map.c      | 9 +++------
 tools/perf/util/python.c   | 2 +-
 4 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/dlfilter.c b/tools/perf/util/dlfilter.c
index e11e144af62bb979..7e179d1d84d78358 100644
--- a/tools/perf/util/dlfilter.c
+++ b/tools/perf/util/dlfilter.c
@@ -233,8 +233,7 @@ static const __u8 *dlfilter__insn(void *ctx, __u32 *len)
 		if (thread__maps(al->thread)) {
 			struct machine *machine = maps__machine(thread__maps(al->thread));
 
-			if (machine)
-				perf_sample__fetch_insn(d->sample, al->thread, machine);
+			perf_sample__fetch_insn(d->sample, al->thread, machine);
 		}
 	}
 
diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index ea75816d126a14be..2f80e364659fea71 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -706,9 +706,6 @@ struct map *thread__find_map(struct thread *thread, u8 cpumode, u64 addr,
 	al->cpumode = cpumode;
 	al->filtered = 0;
 
-	if (machine == NULL)
-		return NULL;
-
 	if (cpumode == PERF_RECORD_MISC_KERNEL && perf_host) {
 		al->level = 'k';
 		maps = machine__kernel_maps(machine);
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 41cdddc987eee710..d5319ffa88a7477c 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -538,13 +538,10 @@ u64 map__rip_2objdump(const struct map *map, u64 rip)
 	 */
 	if (kmap && is_entry_trampoline(kmap->name) && kmap->kmaps) {
 		struct machine *machine = maps__machine(kmap->kmaps);
+		struct map *kernel_map = machine__kernel_map(machine);
 
-		if (machine) {
-			struct map *kernel_map = machine__kernel_map(machine);
-
-			if (kernel_map)
-				map = kernel_map;
-		}
+		if (kernel_map)
+			map = kernel_map;
 	}
 
 	if (!dso__adjust_symbols(dso))
diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index 60dbe2dad5eb8073..42c587cd54a527a4 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -4536,7 +4536,7 @@ static PyObject *pyrf_call_return__new(struct call_return *cr)
 	if (cr->thread) {
 		struct maps *maps = thread__maps(cr->thread);
 
-		pevent->machine_pid = maps && maps__machine(maps) ? maps__machine(maps)->pid : 0;
+		pevent->machine_pid = maps ? maps__machine(maps)->pid : 0;
 		pevent->pid = thread__pid(cr->thread);
 		pevent->tid = thread__tid(cr->thread);
 	} else {
-- 
2.56.0

Re: [PATCH v2 3/5] perf tools: Remove NULL check for maps__machine()

[sashiko-bot]

> Now maps should have a machine, let's get rid of checks for the return
> value.  There are already many places to assume it returns non-NULL,
> so having this kind of check could give confusion that it can return
> NULL (which is not true anymore).
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019055207.39325-1-namhyung@kernel.org?part=3

[PATCH v2 4/5] perf symbol: Check result when reading section headers

[Namhyung Kim]

Otherwise it may access uninitialized section header struct.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/r/sashiko-outbox-170233@kernel.org
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol-elf.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 1e2716c6af45712a..00ea0e19020fb256 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -196,13 +196,15 @@ Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep,
 	while ((sec = elf_nextscn(elf, sec)) != NULL) {
 		char *str;
 
-		gelf_getshdr(sec, shp);
+		if (!gelf_getshdr(sec, shp))
+			goto next;
 		str = elf_strptr(elf, ep->e_shstrndx, shp->sh_name);
 		if (str && !strcmp(name, str)) {
 			if (idx)
 				*idx = cnt;
 			return sec;
 		}
+next:
 		++cnt;
 	}
 
-- 
2.56.0

Re: [PATCH v2 4/5] perf symbol: Check result when reading section headers

[sashiko-bot]

> Otherwise it may access uninitialized section header struct.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/r/sashiko-outbox-170233@kernel.org
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019055207.39325-1-namhyung@kernel.org?part=4

[PATCH v2 5/5] perf symbol: Ensure dso__set_is_64_bit() for libbfd

[Namhyung Kim]

Otherwise it can confuse cross-platform analysis.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/r/sashiko-outbox-168776@kernel.org
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/libbfd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/libbfd.c b/tools/perf/util/libbfd.c
index a4dcb406c79e9cee..45fd70c4f7e11cd7 100644
--- a/tools/perf/util/libbfd.c
+++ b/tools/perf/util/libbfd.c
@@ -462,6 +462,7 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile)
 	symbols__fixup_end(dso__symbols(dso), false);
 	symbols__fixup_duplicate(dso__symbols(dso));
 	dso__set_adjust_symbols(dso, true);
+	dso__set_is_64_bit(dso, bfd_arch_bits_per_address(abfd) == 64);
 
 	err = dso__has_symbols(dso);
 out_free:
-- 
2.56.0

Re: [PATCH v2 5/5] perf symbol: Ensure dso__set_is_64_bit() for libbfd

[sashiko-bot]

> Otherwise it can confuse cross-platform analysis.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/r/sashiko-outbox-168776@kernel.org
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019055207.39325-1-namhyung@kernel.org?part=5