[PATCH v2 1/4] perf symbol: Fix symbol loading from libbfd

[PATCH v2 1/4] perf symbol: Fix symbol loading from libbfd

[Namhyung Kim]

The dso__load() should return a positive number (number of symbols
loaded) when it succeded to load symbols.  But dso__load_bfd_symbols()
only returns 0 and it's hard to check if it actually loaded symbols or
not.  Let's return the value of dso__has_symbol() and check it in
dso__load() so that it can return 1 for success.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/r/sashiko-outbox-162540@kernel.org
Fixes: eac9a4342e5447cad ("perf symbols: Try reading the symbol table with libbfd")
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/pe-file-parsing.c | 2 +-
 tools/perf/util/libbfd.c           | 2 +-
 tools/perf/util/symbol.c           | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/tests/pe-file-parsing.c b/tools/perf/tests/pe-file-parsing.c
index 30c7da79e109b480..3979f38bf127c218 100644
--- a/tools/perf/tests/pe-file-parsing.c
+++ b/tools/perf/tests/pe-file-parsing.c
@@ -59,7 +59,7 @@ static int run_dir(const char *d)
 	TEST_ASSERT_VAL("Failed to get dso", dso);
 
 	ret = dso__load_bfd_symbols(dso, debugfile);
-	TEST_ASSERT_VAL("Failed to load symbols", ret == 0);
+	TEST_ASSERT_VAL("Failed to load symbols", ret > 0);
 
 	dso__sort_by_name(dso);
 	sym = dso__find_symbol_by_name(dso, "main", &idx);
diff --git a/tools/perf/util/libbfd.c b/tools/perf/util/libbfd.c
index a907d7c1ac948808..a4dcb406c79e9cee 100644
--- a/tools/perf/util/libbfd.c
+++ b/tools/perf/util/libbfd.c
@@ -463,7 +463,7 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile)
 	symbols__fixup_duplicate(dso__symbols(dso));
 	dso__set_adjust_symbols(dso, true);
 
-	err = 0;
+	err = dso__has_symbols(dso);
 out_free:
 	free(symbols);
 out_close:
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 5d98888d068c741a..a394e742f9b18812 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1930,8 +1930,8 @@ int dso__load(struct dso *dso, struct map *map)
 		if (nsexit)
 			nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
 
-		if (bfdrc == 0) {
-			ret = 0;
+		if (bfdrc > 0) {
+			ret = 1;
 			break;
 		}
 
-- 
2.55.0

[PATCH v2 2/4] perf symbol: Add missing symsrc__destroy() for libbfd

[Namhyung Kim]

When libbfd loaded symbols and there's a pending syms_ss, it'd be lost
or overwrite existing symbols.  Let's destroy it before breaking the
loop so that it can safely return from the function.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/r/sashiko-outbox-162540@kernel.org
Fixes: eac9a4342e5447cad ("perf symbols: Try reading the symbol table with libbfd")
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index a394e742f9b18812..55a4b59195a08be5 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1932,7 +1932,7 @@ int dso__load(struct dso *dso, struct map *map)
 
 		if (bfdrc > 0) {
 			ret = 1;
-			break;
+			goto out_destroy;
 		}
 
 		if (!is_reg || sirc < 0)
@@ -1997,6 +1997,7 @@ int dso__load(struct dso *dso, struct map *map)
 			ret += nr_plt;
 	}
 
+out_destroy:
 	for (; ss_pos > 0; ss_pos--)
 		symsrc__destroy(&ss_[ss_pos - 1]);
 out_free:
-- 
2.55.0

Re: [PATCH v2 2/4] perf symbol: Add missing symsrc__destroy() for libbfd

[sashiko-bot]

> When libbfd loaded symbols and there's a pending syms_ss, it'd be lost
> or overwrite existing symbols.  Let's destroy it before breaking the
> loop so that it can safely return from the function.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/r/sashiko-outbox-162540@kernel.org
> Fixes: eac9a4342e5447cad ("perf symbols: Try reading the symbol table with libbfd")
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

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

Re: [PATCH v2 2/4] perf symbol: Add missing symsrc__destroy() for libbfd

[Ian Rogers]

On Sun, Oct 11, 2026 at 10:30 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> When libbfd loaded symbols and there's a pending syms_ss, it'd be lost
> or overwrite existing symbols.  Let's destroy it before breaking the
> loop so that it can safely return from the function.
>
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/r/sashiko-outbox-162540@kernel.org
> Fixes: eac9a4342e5447cad ("perf symbols: Try reading the symbol table with libbfd")
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks!
Ian

> ---
>  tools/perf/util/symbol.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index a394e742f9b18812..55a4b59195a08be5 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1932,7 +1932,7 @@ int dso__load(struct dso *dso, struct map *map)
>
>                 if (bfdrc > 0) {
>                         ret = 1;
> -                       break;
> +                       goto out_destroy;
>                 }
>
>                 if (!is_reg || sirc < 0)
> @@ -1997,6 +1997,7 @@ int dso__load(struct dso *dso, struct map *map)
>                         ret += nr_plt;
>         }
>
> +out_destroy:
>         for (; ss_pos > 0; ss_pos--)
>                 symsrc__destroy(&ss_[ss_pos - 1]);
>  out_free:
> --
> 2.55.0
>

[PATCH v2 3/4] perf symbol: Set dso symtab type for libbfd

[Namhyung Kim]

It missed to set the dso symtab type when the symbols are loaded by
libbfd.  When it happens both syms_ss and runtime_ss may not be set
and won't call dso__load_sym().

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 55a4b59195a08be5..b63fa3c03f0e732c 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1931,6 +1931,7 @@ int dso__load(struct dso *dso, struct map *map)
 			nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
 
 		if (bfdrc > 0) {
+			dso__set_symtab_type(dso, symtab_type);
 			ret = 1;
 			goto out_destroy;
 		}
-- 
2.55.0

Re: [PATCH v2 3/4] perf symbol: Set dso symtab type for libbfd

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf symbol: incomplete dso state initialization for libbfd causing annotate/unwind failure
--

commit 5475c2801d673496a1fee31f44ca921eca39f20c
Author: Namhyung Kim <namhyung@kernel.org>

perf symbol: Set dso symtab type for libbfd

It missed to set the dso symtab type when the symbols are loaded by
libbfd.  When it happens both syms_ss and runtime_ss may not be set
and won't call dso__load_sym().

> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 4878d4292eb85..a90ce714b174b 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1925,6 +1925,7 @@ int dso__load(struct dso *dso, struct map *map)
>  			nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
>  
>  		if (bfdrc > 0) {
> +			dso__set_symtab_type(dso, symtab_type);

[Severity: Medium]
When dso__load() takes this early exit to out_destroy, it bypasses the rest
of the loop which normally initializes symsrc_filename and binary_type:

		if (!syms_ss && symsrc__has_symtab(ss)) {
			syms_ss = ss;
			next_slot = true;
			if (!dso__symsrc_filename(dso))
				dso__set_symsrc_filename(dso, strdup(name));
		}
...
		if (next_slot) {
...
			if (dso__binary_type(dso) == DSO_BINARY_TYPE__NOT_FOUND || ... )
				dso__set_binary_type(dso, symtab_type);

Does skipping these initializations leave the DSO's binary_type as
DSO_BINARY_TYPE__NOT_FOUND?

If so, could this cause perf annotate to fail later on, since
symbol__disassemble() checks for this exact condition and returns an error?

tools/perf/util/disasm.c:symbol__disassemble()
...
	if (dso__binary_type(dso) == DSO_BINARY_TYPE__NOT_FOUND) {
		return SYMBOL_ANNOTATE_ERRNO__COULDNT_DETERMINE_FILE_TYPE;
	}
...

>  			ret = 1;
>  			goto out_destroy;
>  		}

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

Re: [PATCH v2 3/4] perf symbol: Set dso symtab type for libbfd

[Namhyung Kim]

On Mon, Oct 12, 2026 at 05:40:08AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] perf symbol: incomplete dso state initialization for libbfd causing annotate/unwind failure
> --
> 
> commit 5475c2801d673496a1fee31f44ca921eca39f20c
> Author: Namhyung Kim <namhyung@kernel.org>
> 
> perf symbol: Set dso symtab type for libbfd
> 
> It missed to set the dso symtab type when the symbols are loaded by
> libbfd.  When it happens both syms_ss and runtime_ss may not be set
> and won't call dso__load_sym().
> 
> > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> > index 4878d4292eb85..a90ce714b174b 100644
> > --- a/tools/perf/util/symbol.c
> > +++ b/tools/perf/util/symbol.c
> > @@ -1925,6 +1925,7 @@ int dso__load(struct dso *dso, struct map *map)
> >  			nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
> >  
> >  		if (bfdrc > 0) {
> > +			dso__set_symtab_type(dso, symtab_type);
> 
> [Severity: Medium]
> When dso__load() takes this early exit to out_destroy, it bypasses the rest
> of the loop which normally initializes symsrc_filename and binary_type:
> 
> 		if (!syms_ss && symsrc__has_symtab(ss)) {
> 			syms_ss = ss;
> 			next_slot = true;
> 			if (!dso__symsrc_filename(dso))
> 				dso__set_symsrc_filename(dso, strdup(name));
> 		}
> ...
> 		if (next_slot) {
> ...
> 			if (dso__binary_type(dso) == DSO_BINARY_TYPE__NOT_FOUND || ... )
> 				dso__set_binary_type(dso, symtab_type);
> 
> Does skipping these initializations leave the DSO's binary_type as
> DSO_BINARY_TYPE__NOT_FOUND?
> 
> If so, could this cause perf annotate to fail later on, since
> symbol__disassemble() checks for this exact condition and returns an error?

This is what I'm working on in another patchset.  It'd be totally fine
not to set symsrc_filename and binary_type with that change.

Thanks,
Namhyung

> 
> tools/perf/util/disasm.c:symbol__disassemble()
> ...
> 	if (dso__binary_type(dso) == DSO_BINARY_TYPE__NOT_FOUND) {
> 		return SYMBOL_ANNOTATE_ERRNO__COULDNT_DETERMINE_FILE_TYPE;
> 	}
> ...
> 
> >  			ret = 1;
> >  			goto out_destroy;
> >  		}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261012053013.14378-1-namhyung@kernel.org?part=3

[PATCH v2 4/4] perf test: Update PE file test for other locations

[Namhyung Kim]

The test__pe_file_parsing() expects the test is in the tools/perf
directory, but it could be root or a separate build directory.  Let's
update the logic to try other paths like in the shell_tests__dir_fd().

Also add some debug messages to inform users why it's skipped.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/pe-file-parsing.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/tools/perf/tests/pe-file-parsing.c b/tools/perf/tests/pe-file-parsing.c
index 3979f38bf127c218..78dae7e43603b9f8 100644
--- a/tools/perf/tests/pe-file-parsing.c
+++ b/tools/perf/tests/pe-file-parsing.c
@@ -74,10 +74,17 @@ static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
 {
 	struct stat st;
 	char path_dir[PATH_MAX];
+	static const char * const tests_dirs[] = {
+		"./tools/perf/tests",
+		"./tests",
+		"./source/tests"
+	};
 
 	/* First try development tree tests. */
-	if (!lstat("./tests", &st))
-		return run_dir("./tests");
+	for (size_t i = 0; i < ARRAY_SIZE(tests_dirs); ++i) {
+		if (!lstat(tests_dirs[i], &st))
+			return run_dir(tests_dirs[i]);
+	}
 
 	/* Then installed path. */
 	snprintf(path_dir, PATH_MAX, "%s/tests", get_argv_exec_path());
@@ -85,6 +92,7 @@ static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
 	if (!lstat(path_dir, &st))
 		return run_dir(path_dir);
 
+	pr_debug("cannot find the tests directory\n");
 	return TEST_SKIP;
 }
 
@@ -93,6 +101,7 @@ static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
 static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
 			  int subtest __maybe_unused)
 {
+	pr_debug("libbfd is not available\n");
 	return TEST_SKIP;
 }
 
-- 
2.55.0

Re: [PATCH v2 4/4] perf test: Update PE file test for other locations

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf/tests: spurious test failure due to brittle directory checking
--

commit 073d3f0f55cefe06ae3b0caf163e2a6505e656b3
Author: Namhyung Kim <namhyung@kernel.org>

perf test: Update PE file test for other locations

This updates test__pe_file_parsing() to search multiple directories to
find the tests directory, similar to shell_tests__dir_fd(), and adds
debug messages when skipped.

> diff --git a/tools/perf/tests/pe-file-parsing.c b/tools/perf/tests/pe-file-parsing.c
> index 3979f38bf127c..78dae7e43603b 100644
> --- a/tools/perf/tests/pe-file-parsing.c
> +++ b/tools/perf/tests/pe-file-parsing.c
> @@ -74,10 +74,17 @@ static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
>  {
>  	struct stat st;
>  	char path_dir[PATH_MAX];
> +	static const char * const tests_dirs[] = {
> +		"./tools/perf/tests",
> +		"./tests",
> +		"./source/tests"
> +	};
>  
>  	/* First try development tree tests. */
> -	if (!lstat("./tests", &st))
> -		return run_dir("./tests");
> +	for (size_t i = 0; i < ARRAY_SIZE(tests_dirs); ++i) {
> +		if (!lstat(tests_dirs[i], &st))
> +			return run_dir(tests_dirs[i]);

[Severity: Medium]
Will this cause a spurious test failure if a matching directory exists but
doesn't contain pe-file.exe?

The new loop eagerly calls run_dir() upon finding a matching directory
name instead of verifying the actual test file exists.

If pe-file.exe is missing from the matched directory, run_dir() calls
filename__read_build_id() which returns an error. This causes
TEST_ASSERT_VAL to immediately return TEST_FAIL, breaking the test suite
instead of falling back to the installed path.

The commit message notes this logic tries other paths like
shell_tests__dir_fd(), but shell_tests__dir_fd() merely returns a directory
descriptor and gracefully continues with zero shell tests if the directory
is empty. By contrast, run_dir() explicitly asserts file validity and
aborts the suite with a hard TEST_FAIL.

Should this verify pe-file.exe actually exists before committing to a
directory and calling run_dir()?

> +	}
>  
>  	/* Then installed path. */
>  	snprintf(path_dir, PATH_MAX, "%s/tests", get_argv_exec_path());

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

Re: [PATCH v2 4/4] perf test: Update PE file test for other locations

[Namhyung Kim]

On Mon, Oct 12, 2026 at 05:38:20AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] perf/tests: spurious test failure due to brittle directory checking
> --
> 
> commit 073d3f0f55cefe06ae3b0caf163e2a6505e656b3
> Author: Namhyung Kim <namhyung@kernel.org>
> 
> perf test: Update PE file test for other locations
> 
> This updates test__pe_file_parsing() to search multiple directories to
> find the tests directory, similar to shell_tests__dir_fd(), and adds
> debug messages when skipped.
> 
> > diff --git a/tools/perf/tests/pe-file-parsing.c b/tools/perf/tests/pe-file-parsing.c
> > index 3979f38bf127c..78dae7e43603b 100644
> > --- a/tools/perf/tests/pe-file-parsing.c
> > +++ b/tools/perf/tests/pe-file-parsing.c
> > @@ -74,10 +74,17 @@ static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
> >  {
> >  	struct stat st;
> >  	char path_dir[PATH_MAX];
> > +	static const char * const tests_dirs[] = {
> > +		"./tools/perf/tests",
> > +		"./tests",
> > +		"./source/tests"
> > +	};
> >  
> >  	/* First try development tree tests. */
> > -	if (!lstat("./tests", &st))
> > -		return run_dir("./tests");
> > +	for (size_t i = 0; i < ARRAY_SIZE(tests_dirs); ++i) {
> > +		if (!lstat(tests_dirs[i], &st))
> > +			return run_dir(tests_dirs[i]);
> 
> [Severity: Medium]
> Will this cause a spurious test failure if a matching directory exists but
> doesn't contain pe-file.exe?
> 
> The new loop eagerly calls run_dir() upon finding a matching directory
> name instead of verifying the actual test file exists.
> 
> If pe-file.exe is missing from the matched directory, run_dir() calls
> filename__read_build_id() which returns an error. This causes
> TEST_ASSERT_VAL to immediately return TEST_FAIL, breaking the test suite
> instead of falling back to the installed path.
> 
> The commit message notes this logic tries other paths like
> shell_tests__dir_fd(), but shell_tests__dir_fd() merely returns a directory
> descriptor and gracefully continues with zero shell tests if the directory
> is empty. By contrast, run_dir() explicitly asserts file validity and
> aborts the suite with a hard TEST_FAIL.
> 
> Should this verify pe-file.exe actually exists before committing to a
> directory and calling run_dir()?

It's expected to point to the tests directory in the source tree, so it
would contain the file.  But in a rare case where it runs on a directory
which happens to have a "tests", the test will fail on reading build-id.

I think it's a minor pre-existing issue and maybe we can just ignore it.

Thanks,
Namhyung

Re: [PATCH v2 4/4] perf test: Update PE file test for other locations

[Ian Rogers]

On Sun, Oct 11, 2026 at 10:30 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The test__pe_file_parsing() expects the test is in the tools/perf
> directory, but it could be root or a separate build directory.  Let's
> update the logic to try other paths like in the shell_tests__dir_fd().
>
> Also add some debug messages to inform users why it's skipped.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/tests/pe-file-parsing.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/tests/pe-file-parsing.c b/tools/perf/tests/pe-file-parsing.c
> index 3979f38bf127c218..78dae7e43603b9f8 100644
> --- a/tools/perf/tests/pe-file-parsing.c
> +++ b/tools/perf/tests/pe-file-parsing.c
> @@ -74,10 +74,17 @@ static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
>  {
>         struct stat st;
>         char path_dir[PATH_MAX];
> +       static const char * const tests_dirs[] = {
> +               "./tools/perf/tests",
> +               "./tests",
> +               "./source/tests"

I'm not sure where "source/tests" comes from?
nit: could we add the exec_path case to the list?

Thanks,
Ian

> +       };
>
>         /* First try development tree tests. */
> -       if (!lstat("./tests", &st))
> -               return run_dir("./tests");
> +       for (size_t i = 0; i < ARRAY_SIZE(tests_dirs); ++i) {
> +               if (!lstat(tests_dirs[i], &st))
> +                       return run_dir(tests_dirs[i]);
> +       }
>
>         /* Then installed path. */
>         snprintf(path_dir, PATH_MAX, "%s/tests", get_argv_exec_path());
> @@ -85,6 +92,7 @@ static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
>         if (!lstat(path_dir, &st))
>                 return run_dir(path_dir);
>
> +       pr_debug("cannot find the tests directory\n");
>         return TEST_SKIP;
>  }
>
> @@ -93,6 +101,7 @@ static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
>  static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
>                           int subtest __maybe_unused)
>  {
> +       pr_debug("libbfd is not available\n");
>         return TEST_SKIP;
>  }
>
> --
> 2.55.0
>

Re: [PATCH v2 4/4] perf test: Update PE file test for other locations

[Namhyung Kim]

On Mon, Oct 12, 2026 at 02:51:52PM -0700, Ian Rogers wrote:
> On Sun, Oct 11, 2026 at 10:30 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > The test__pe_file_parsing() expects the test is in the tools/perf
> > directory, but it could be root or a separate build directory.  Let's
> > update the logic to try other paths like in the shell_tests__dir_fd().
> >
> > Also add some debug messages to inform users why it's skipped.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/tests/pe-file-parsing.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/tests/pe-file-parsing.c b/tools/perf/tests/pe-file-parsing.c
> > index 3979f38bf127c218..78dae7e43603b9f8 100644
> > --- a/tools/perf/tests/pe-file-parsing.c
> > +++ b/tools/perf/tests/pe-file-parsing.c
> > @@ -74,10 +74,17 @@ static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
> >  {
> >         struct stat st;
> >         char path_dir[PATH_MAX];
> > +       static const char * const tests_dirs[] = {
> > +               "./tools/perf/tests",
> > +               "./tests",
> > +               "./source/tests"
> 
> I'm not sure where "source/tests" comes from?

AFAIK it's set when you pass O=$(DIR) option when you build perf and it
points the source tree.

> nit: could we add the exec_path case to the list?

It's checked right after. :)

Thanks,
Namhyung

> 
> Thanks,
> Ian
> 
> > +       };
> >
> >         /* First try development tree tests. */
> > -       if (!lstat("./tests", &st))
> > -               return run_dir("./tests");
> > +       for (size_t i = 0; i < ARRAY_SIZE(tests_dirs); ++i) {
> > +               if (!lstat(tests_dirs[i], &st))
> > +                       return run_dir(tests_dirs[i]);
> > +       }
> >
> >         /* Then installed path. */
> >         snprintf(path_dir, PATH_MAX, "%s/tests", get_argv_exec_path());
> > @@ -85,6 +92,7 @@ static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
> >         if (!lstat(path_dir, &st))
> >                 return run_dir(path_dir);
> >
> > +       pr_debug("cannot find the tests directory\n");
> >         return TEST_SKIP;
> >  }
> >
> > @@ -93,6 +101,7 @@ static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
> >  static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
> >                           int subtest __maybe_unused)
> >  {
> > +       pr_debug("libbfd is not available\n");
> >         return TEST_SKIP;
> >  }
> >
> > --
> > 2.55.0
> >

Re: [PATCH v2 4/4] perf test: Update PE file test for other locations

[Ian Rogers]

On Mon, Oct 12, 2026 at 4:06 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Mon, Oct 12, 2026 at 02:51:52PM -0700, Ian Rogers wrote:
> > On Sun, Oct 11, 2026 at 10:30 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > The test__pe_file_parsing() expects the test is in the tools/perf
> > > directory, but it could be root or a separate build directory.  Let's
> > > update the logic to try other paths like in the shell_tests__dir_fd().
> > >
> > > Also add some debug messages to inform users why it's skipped.
> > >
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > ---
> > >  tools/perf/tests/pe-file-parsing.c | 13 +++++++++++--
> > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/perf/tests/pe-file-parsing.c b/tools/perf/tests/pe-file-parsing.c
> > > index 3979f38bf127c218..78dae7e43603b9f8 100644
> > > --- a/tools/perf/tests/pe-file-parsing.c
> > > +++ b/tools/perf/tests/pe-file-parsing.c
> > > @@ -74,10 +74,17 @@ static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
> > >  {
> > >         struct stat st;
> > >         char path_dir[PATH_MAX];
> > > +       static const char * const tests_dirs[] = {
> > > +               "./tools/perf/tests",
> > > +               "./tests",
> > > +               "./source/tests"
> >
> > I'm not sure where "source/tests" comes from?
>
> AFAIK it's set when you pass O=$(DIR) option when you build perf and it
> points the source tree.

Ah, Andi's change:
https://lore.kernel.org/r/20240807231823.898979-1-ak@linux.intel.com

> > nit: could we add the exec_path case to the list?
>
> It's checked right after. :)

Right, I think you can avoid the duplicated logic by having tests_dir
also have path_dir as the last entry. It'd mean always doing the
snprintf but performance doesn't matter much in a test.

Thanks,
Ian

> Thanks,
> Namhyung
>
> >
> > Thanks,
> > Ian
> >
> > > +       };
> > >
> > >         /* First try development tree tests. */
> > > -       if (!lstat("./tests", &st))
> > > -               return run_dir("./tests");
> > > +       for (size_t i = 0; i < ARRAY_SIZE(tests_dirs); ++i) {
> > > +               if (!lstat(tests_dirs[i], &st))
> > > +                       return run_dir(tests_dirs[i]);
> > > +       }
> > >
> > >         /* Then installed path. */
> > >         snprintf(path_dir, PATH_MAX, "%s/tests", get_argv_exec_path());
> > > @@ -85,6 +92,7 @@ static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
> > >         if (!lstat(path_dir, &st))
> > >                 return run_dir(path_dir);
> > >
> > > +       pr_debug("cannot find the tests directory\n");
> > >         return TEST_SKIP;
> > >  }
> > >
> > > @@ -93,6 +101,7 @@ static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
> > >  static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
> > >                           int subtest __maybe_unused)
> > >  {
> > > +       pr_debug("libbfd is not available\n");
> > >         return TEST_SKIP;
> > >  }
> > >
> > > --
> > > 2.55.0
> > >

Re: [PATCH v2 4/4] perf test: Update PE file test for other locations

[Namhyung Kim]

On Mon, Oct 12, 2026 at 04:11:27PM -0700, Ian Rogers wrote:
> On Mon, Oct 12, 2026 at 4:06 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Mon, Oct 12, 2026 at 02:51:52PM -0700, Ian Rogers wrote:
> > > On Sun, Oct 11, 2026 at 10:30 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > > >
> > > > The test__pe_file_parsing() expects the test is in the tools/perf
> > > > directory, but it could be root or a separate build directory.  Let's
> > > > update the logic to try other paths like in the shell_tests__dir_fd().
> > > >
> > > > Also add some debug messages to inform users why it's skipped.
> > > >
> > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > > ---
> > > >  tools/perf/tests/pe-file-parsing.c | 13 +++++++++++--
> > > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/tools/perf/tests/pe-file-parsing.c b/tools/perf/tests/pe-file-parsing.c
> > > > index 3979f38bf127c218..78dae7e43603b9f8 100644
> > > > --- a/tools/perf/tests/pe-file-parsing.c
> > > > +++ b/tools/perf/tests/pe-file-parsing.c
> > > > @@ -74,10 +74,17 @@ static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
> > > >  {
> > > >         struct stat st;
> > > >         char path_dir[PATH_MAX];
> > > > +       static const char * const tests_dirs[] = {
> > > > +               "./tools/perf/tests",
> > > > +               "./tests",
> > > > +               "./source/tests"
> > >
> > > I'm not sure where "source/tests" comes from?
> >
> > AFAIK it's set when you pass O=$(DIR) option when you build perf and it
> > points the source tree.
> 
> Ah, Andi's change:
> https://lore.kernel.org/r/20240807231823.898979-1-ak@linux.intel.com
> 
> > > nit: could we add the exec_path case to the list?
> >
> > It's checked right after. :)
> 
> Right, I think you can avoid the duplicated logic by having tests_dir
> also have path_dir as the last entry. It'd mean always doing the
> snprintf but performance doesn't matter much in a test.

Ah ok, will do!

Thanks,
Namhyung

Re: [PATCH v2 1/4] perf symbol: Fix symbol loading from libbfd

[sashiko-bot]

> The dso__load() should return a positive number (number of symbols
> loaded) when it succeded to load symbols.  But dso__load_bfd_symbols()
> only returns 0 and it's hard to check if it actually loaded symbols or
> not.  Let's return the value of dso__has_symbol() and check it in
> dso__load() so that it can return 1 for success.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/r/sashiko-outbox-162540@kernel.org
> Fixes: eac9a4342e5447cad ("perf symbols: Try reading the symbol table with libbfd")
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

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

Re: [PATCH v2 1/4] perf symbol: Fix symbol loading from libbfd

[Ian Rogers]

On Sun, Oct 11, 2026 at 10:30 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The dso__load() should return a positive number (number of symbols
> loaded) when it succeded to load symbols.  But dso__load_bfd_symbols()
> only returns 0 and it's hard to check if it actually loaded symbols or
> not.  Let's return the value of dso__has_symbol() and check it in
> dso__load() so that it can return 1 for success.
>
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/r/sashiko-outbox-162540@kernel.org
> Fixes: eac9a4342e5447cad ("perf symbols: Try reading the symbol table with libbfd")
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/tests/pe-file-parsing.c | 2 +-
>  tools/perf/util/libbfd.c           | 2 +-
>  tools/perf/util/symbol.c           | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/tests/pe-file-parsing.c b/tools/perf/tests/pe-file-parsing.c
> index 30c7da79e109b480..3979f38bf127c218 100644
> --- a/tools/perf/tests/pe-file-parsing.c
> +++ b/tools/perf/tests/pe-file-parsing.c
> @@ -59,7 +59,7 @@ static int run_dir(const char *d)
>         TEST_ASSERT_VAL("Failed to get dso", dso);
>
>         ret = dso__load_bfd_symbols(dso, debugfile);
> -       TEST_ASSERT_VAL("Failed to load symbols", ret == 0);
> +       TEST_ASSERT_VAL("Failed to load symbols", ret > 0);
>
>         dso__sort_by_name(dso);
>         sym = dso__find_symbol_by_name(dso, "main", &idx);
> diff --git a/tools/perf/util/libbfd.c b/tools/perf/util/libbfd.c
> index a907d7c1ac948808..a4dcb406c79e9cee 100644
> --- a/tools/perf/util/libbfd.c
> +++ b/tools/perf/util/libbfd.c
> @@ -463,7 +463,7 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile)
>         symbols__fixup_duplicate(dso__symbols(dso));
>         dso__set_adjust_symbols(dso, true);
>
> -       err = 0;
> +       err = dso__has_symbols(dso);
>  out_free:
>         free(symbols);
>  out_close:
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 5d98888d068c741a..a394e742f9b18812 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1930,8 +1930,8 @@ int dso__load(struct dso *dso, struct map *map)
>                 if (nsexit)
>                         nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
>
> -               if (bfdrc == 0) {
> -                       ret = 0;
> +               if (bfdrc > 0) {
> +                       ret = 1;
>                         break;
>                 }
>
> --
> 2.55.0
>