Re: [PATCH v6 5/5] perf test: Test lazy symbol name loading

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] perf test: missing skip when .opd or .gnu_debugdata disables lazy names
- [Medium] perf test: script_lazy_load_symbols.sh fails before reaching skip check
- [Medium] perf test: assertion failure or OOB read in sym_loader__init() on kernel DSO
- [Low] perf test symbol-lazy: missing explicit <stdio.h> and <stdbool.h> includes
--

commit 80f2faac89471ac12d3bce19d66d36a7c65ef72f
Author: Alireza Haghdoost <haghdoost@uber.com>

perf test: Test lazy symbol name loading

This commit adds a unit test suite in tools/perf/tests/symbol-lazy.c and two
shell tests (lazy_load_symbols_parity.sh and script_lazy_load_symbols.sh) to
verify lazy symbol name loading, concurrency, and parity with eager loading.

> diff --git a/tools/perf/tests/shell/lazy_load_symbols_parity.sh b/tools/perf/tests/shell/lazy_load_symbols_parity.sh
> new file mode 100755
> index 0000000000000..af1555d7f8d09
> --- /dev/null
> +++ b/tools/perf/tests/shell/lazy_load_symbols_parity.sh
> @@ -0,0 +1,284 @@

[ ... ]

> +# check_parity NAME FILE [eager]
> +# Lazy loading must leave names of FILE unread, or none if "eager" is given.
> +check_parity() {
> +	local out="${tmpdir}/parity.out"
> +	local lazy="[0-9][0-9]* symbols with lazy names"
> +
> +	if [ "$3" = eager ]; then
> +		lazy="no lazy names"
> +	fi
> +	perf test --dso "$2" -vv "Lazy and eager symbol parity" > "${out}" 2>&1 || true
> +	if grep -q ': Ok$' "${out}" && grep -q "^$2: ${lazy}$" "${out}"; then
> +		echo "Lazy-load parity $1 [Success]"
> +		return
> +	fi
> +	grep -E 'symbols$|names$|mismatch|lookup|no symbols' "${out}" || true
> +	echo "Lazy-load parity $1 [Failed]"
> +	err=1
> +}

[Severity: Medium]
In tools/perf/util/symbol-elf.c, dso__load_sym_internal() disables lazy
symbol name loading when an .opd section is present (such as on PPC64 ELFv1),
when symbols come from .gnu_debugdata, or when .strtab has SHF_COMPRESSED:

tools/perf/util/symbol-elf.c:dso__load_sym_internal() {
	...
	bool lazy_allowed = symbol_conf.lazy_load_symbols && !dso__kernel(dso) &&
			  !kmodule && syms_ss->type == dso__symtab_type(dso) &&
			  syms_ss->type != DSO_BINARY_TYPE__GNU_DEBUGDATA &&
			  !runtime_ss->opdsec;
	...
}

In lazy_load_symbols_parity.sh, test_split_debug() and test_dynsym_only()
call check_parity() without the eager argument, requiring a non-zero count of
lazy names in the output of test__symbol_lazy_parity(). Similarly, in
tools/perf/tests/shell/script_lazy_load_symbols.sh, skip_test_missing_symbol
passes as long as eager symbol loading resolves test_loop, and then
test_lazy_load_identical() and test_lazy_load_mount_ns() require lazy names
on perf_path.

Won't these tests fail with err=1 instead of skipping on PPC64 ELFv1 hosts
where the compiler emits an .opd section, or when the perf binary uses
.gnu_debugdata MiniDebugInfo or SHF_COMPRESSED string tables?

> diff --git a/tools/perf/tests/shell/script_lazy_load_symbols.sh b/tools/perf/tests/shell/script_lazy_load_symbols.sh
> new file mode 100755
> index 0000000000000..88840bfafdf64
> --- /dev/null
> +++ b/tools/perf/tests/shell/script_lazy_load_symbols.sh
> @@ -0,0 +1,177 @@

[ ... ]

> +test_lazy_load_identical() {
> +	echo "Lazy-load output matches eager loader"
> +
> +	if ! perf record -o "${perfdata}" -g -- perf test -w thloop 2> /dev/null
> +	then
> +		echo "Lazy-load identical [Skipped record not supported]"
> +		return 0
> +	fi
> +
> +	if ! perf script -i "${perfdata}" 2> /dev/null > "${eager_out}" || \
> +	   ! perf script -v --lazy-load-symbols -i "${perfdata}" \
> +		2> "${lazy_err}" > "${lazy_out}"
> +	then
> +		echo "Lazy-load identical [Failed perf script error]"
> +		err=1
> +		return
> +	fi
> +	if ! grep -Eq "${lazy_names}" "${lazy_err}"
> +	then
> +		echo "Lazy-load identical [Failed lazy loader fell back to eager]"
> +		err=1
> +		return
> +	fi
> +	if ! grep -F "${perf_path}: " "${lazy_err}" | grep -Eq "${lazy_names}"
> +	then
> +		echo "Lazy-load identical [Failed controlled DSO has no lazy names]"
> +		err=1
> +		return
> +	fi
> +
> +	# The comparison is only meaningful if something actually resolved;
> +	# two all-[unknown] outputs would also match.
> +	if ! grep -q "${testsym}" "${eager_out}"
> +	then
> +		echo "Lazy-load identical [Skipped no ${testsym} resolved]"
> +		return 0
> +	fi

[Severity: Medium]
In perf script, user-space DSOs are only loaded on demand via
thread__find_symbol() -> map__find_symbol() -> map__load() -> dso__load()
when a sample or callchain IP falls within that DSO's map.

If perf record succeeds but captures no samples in perf_path (for example, in
a virtualized or CPU-constrained environment), dso__load_sym_internal() is
never called for perf_path, so lazy_err will not contain lazy symbol messages
for perf_path.

Because test_lazy_load_identical() checks lazy_err before checking whether
testsym was resolved in eager_out, won't it fail with err=1 before ever
reaching the skip check for testsym? Should the check that testsym is present
in eager_out happen first (and likewise in test_lazy_load_mount_ns() when the
1-second attached recording captures no samples in testsym)?

> diff --git a/tools/perf/tests/symbol-lazy.c b/tools/perf/tests/symbol-lazy.c
> new file mode 100644
> index 0000000000000..52d4a9850e19b
> --- /dev/null
> +++ b/tools/perf/tests/symbol-lazy.c
> @@ -0,0 +1,544 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <inttypes.h>
> +#include <limits.h>
> +#include <pthread.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/mman.h>
> +#include <unistd.h>
> +#include <linux/kernel.h>

[Severity: Low]
In tools/perf/tests/symbol-lazy.c, lazy_fixture__init() uses FILE *,
fdopen(), fputc(), fwrite(), and fclose(), and test__symbol_lazy_choice() and
other functions use bool, true, false, and ELF symbol constants (STT_FUNC,
STT_NOTYPE, STT_GNU_IFUNC, STB_GLOBAL, STB_WEAK, STB_LOCAL).

Could <stdio.h>, <stdbool.h>, and <elf.h> be explicitly included at the top
of tools/perf/tests/symbol-lazy.c so the build does not rely on transitive
header inclusions, particularly when building against musl libc?

[ ... ]

> +static int sym_loader__init(struct sym_loader *l, const char *filename, bool lazy)
> +{
> +	memset(l, 0, sizeof(*l));
> +	perf_env__init(&l->env);
> +	symbol_conf.lazy_load_symbols = lazy;
> +	l->machine = machine__new_host(&l->env);
> +	if (l->machine)
> +		l->thread = machine__findnew_thread(l->machine, 100, 100);
> +	if (l->thread)
> +		l->map = map__new(l->machine, 0x100000, 0xffffffff, 0, &dso_id_empty,
> +				  PROT_EXEC, /*flags=*/0, (char *)filename, l->thread);
> +	if (!l->map) {
> +		sym_loader__exit(l);
> +		return TEST_FAIL;
> +	}

[Severity: Medium]
In sym_loader__init(), machine__new_host() populates l->machine->dsos with
the host kernel and all currently loaded kernel modules (which have
dso__kernel(dso) != DSO_SPACE__USER).

If a user runs perf test with --dso pointing to a currently loaded kernel
module or kernel DSO, test__symbol_lazy_parity() passes that path into
sym_loader__init() -> map__new(), where machine__findnew_dso_id() returns the
existing kernel DSO:

tools/perf/util/map.c:map__new() {
	...
	dso = machine__findnew_dso_id(machine, filename, id);

	if (dso == NULL)
		goto out_delete;

	assert(!dso__kernel(dso));
	...
}

In default builds, won't this trigger assert(!dso__kernel(dso)), and in
builds with -DNDEBUG, cause dso__load() -> dso__load_sym_internal() ->
map__kmap() to read past the end of the map allocation because map__new() did
not allocate space for struct kmap? Should sym_loader__init() handle kernel
DSOs similarly to create_map() in tools/perf/tests/symbols.c?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-perf-symbol-memory-send-v6-0-7e90415f0101@uber.com?part=5