Re: [PATCH v10 4/4] kallsyms: add KUnit tests for lineinfo feature

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] kallsyms: identical bodies in lineinfo_kunit targets allow IPA-ICF folding
- [Low] kallsyms: unreachable failure path in test_assembly_no_lineinfo()
- [Low] kallsyms: missing has_lineinfo() assertion in test_many_lines_mid_function()
- [Low] kallsyms: inaccurate lineinfo comments in sprint_with_lineinfo() and test
- [Low] kallsyms: test_static_function() duplicates test_short_function()
--

commit fbb509d58818b09aeb1b086d5f710841ca770e58
Author: Sasha Levin <sashal@kernel.org>

kallsyms: add KUnit tests for lineinfo feature

This commit adds a KUnit test module (CONFIG_LINEINFO_KUNIT_TEST) to verify
kallsyms source file and line annotations in stack traces, and exports
sprint_backtrace() and sprint_backtrace_build_id() for KUnit testing.

> diff --git a/lib/tests/lineinfo_kunit.c b/lib/tests/lineinfo_kunit.c
> new file mode 100644
> index 0000000000000..066f342bd78ae
> --- /dev/null
> +++ b/lib/tests/lineinfo_kunit.c
> @@ -0,0 +1,1110 @@
[ ... ]
> +/*
> + * Format a symbol with lineinfo annotation.  Lineinfo is appended only
> + * via the sprint_backtrace*() entry points (kernel/kallsyms.c only adds
> + * the "(file:line)" suffix in stack-trace context — sprint_symbol() is
> + * used by %ps and many existing format strings tack literal "()" after
> + * %ps, where the annotation would render as "foo (file:line)()").
> + *
> + * sprint_backtrace() subtracts 1 from the address to handle tail-call
> + * return-address corrections; pass @addr + 1 to recover the original.
> + */
> +static int sprint_with_lineinfo(char *buf, unsigned long addr)
> +{
> +	return sprint_backtrace(buf, addr + 1);
> +}

[Severity: Low]
Is the comment above sprint_with_lineinfo() still accurate?

In kernel/kallsyms.c, sprint_symbol() and sprint_symbol_build_id() also pass
add_lineinfo = 1 (backing %pS and %pSb), while sprint_symbol_no_offset() backs
lowercase %ps and passes add_lineinfo = 0.  Later in this file,
test_sprint_symbol_format() and test_pS_format() also assert that
sprint_symbol() and %pS include lineinfo.

[ ... ]
> +static noinline int lineinfo_target_with_arg(int x)
> +{
> +	barrier();
> +	return x + 1;
> +}
[ ... ]
> +static noinline int lineinfo_cross_file_caller(int x)
> +{
> +	return lineinfo_header_helper(x);
> +}

[Severity: Medium]
Can GCC's identical-code folding (IPA-ICF) merge lineinfo_cross_file_caller()
into lineinfo_target_with_arg() and cause test_cross_file_inline() to fail?

Unlike the 0-argument target functions which return distinct constants (42, 1,
99, 10), lineinfo_target_with_arg(), lineinfo_header_helper() in
lib/tests/lineinfo_kunit.h (inlined into lineinfo_cross_file_caller()), and
lineinfo_target_second_file() in lib/tests/lineinfo_kunit_second.c all share
the exact same signature and body:

	barrier();
	return x + 1;

After __always_inline expansion of lineinfo_header_helper() into
lineinfo_cross_file_caller(), lineinfo_target_with_arg() and
lineinfo_cross_file_caller() are two static noinline functions in the same
translation unit with identical GIMPLE bodies.  When built with GCC at -O2 or
-Os without -fno-optimize-sibling-calls (for example when
CONFIG_FRAME_POINTER=n), -fipa-icf-functions merges
lineinfo_cross_file_caller() into lineinfo_target_with_arg() and replaces
lineinfo_cross_file_caller() with a tail-call thunk whose DWARF .debug_line
entry only references lib/tests/lineinfo_kunit.c.

When that happens, test_cross_file_inline() finds no address in
lineinfo_cross_file_caller() resolving to lineinfo_kunit.h and fails.  Would
giving each function a distinct return expression (such as x + 1, x + 2,
x + 3) or marking them __noipa prevent the compiler from folding them?

[ ... ]
> +static void test_static_function(struct kunit *test)
> +{
> +	char *buf = alloc_sym_buf(test);
> +	unsigned long addr = target_addr(lineinfo_target_short);
> +
> +	skip_without_module_lineinfo(test);
> +
> +	sprint_with_lineinfo(buf, addr);
> +	KUNIT_EXPECT_TRUE_MSG(test, has_lineinfo(buf),
> +			      "No lineinfo in: %s", buf);
> +}
[ ... ]
> +static void test_short_function(struct kunit *test)
> +{
> +	char *buf = alloc_sym_buf(test);
> +	unsigned long addr = target_addr(lineinfo_target_short);
> +
> +	skip_without_module_lineinfo(test);
> +
> +	sprint_with_lineinfo(buf, addr);
> +	KUNIT_EXPECT_TRUE_MSG(test, has_lineinfo(buf),
> +			      "No lineinfo for short function in: %s", buf);
> +}

[Severity: Low]
Does test_static_function() duplicate test_short_function()?  Both functions
resolve target_addr(lineinfo_target_short) with sprint_with_lineinfo(buf, addr)
and check has_lineinfo(buf), and all of the local target functions in
lib/tests/lineinfo_kunit.c are already static.

[ ... ]
> +static void test_symbol_start_addr(struct kunit *test)
> +{
> +	char *buf = alloc_sym_buf(test);
> +	unsigned long addr = target_addr(lineinfo_target_normal);
> +
> +	skip_without_module_lineinfo(test);
> +
> +	/*
> +	 * sprint_backtrace() subtracts 1 from the input and reports offset
> +	 * relative to the (decremented) address, so an exact "+0x0/" can't
> +	 * be expected here.  Verify the symbol resolves and carries lineinfo.
> +	 */
> +	sprint_with_lineinfo(buf, addr);
> +	KUNIT_EXPECT_TRUE_MSG(test,
> +			      strnstr(buf, "lineinfo_target_normal",
> +				      KSYM_SYMBOL_LEN) != NULL,
> +			      "Didn't resolve to expected function: %s", buf);
> +	KUNIT_EXPECT_TRUE_MSG(test, has_lineinfo(buf),
> +			      "No lineinfo at function start: %s", buf);
> +}

[Severity: Low]
In test_symbol_start_addr(), is the comment about how sprint_backtrace()
computes the offset accurate?

In __sprint_symbol(), symbol_offset (-1) is added to address for the lookup
and then subtracted back from offset:

	address += symbol_offset;
	len = kallsyms_lookup_buildid(address, &size, &offset, &modname, &buildid,
				       buffer);
	...
	sym_start = address - offset;
	offset -= symbol_offset;

Because sprint_with_lineinfo() passes addr + 1 to sprint_backtrace(),
__sprint_symbol() looks up addr (offset 0) and then adjusts offset to +0x1/
(the undecremented input offset), whereas calling sprint_symbol(buf, addr)
directly would report +0x0/ with lineinfo.

[ ... ]
> +static void test_assembly_no_lineinfo(struct kunit *test)
> +{
> +#if IS_BUILTIN(CONFIG_LINEINFO_KUNIT_TEST)
> +	char *buf = alloc_sym_buf(test);
> +	unsigned long addr = (unsigned long)_text;
> +
> +	sprint_with_lineinfo(buf, addr);
> +	/*
> +	 * _text is typically an asm entry point with no DWARF line info.
> +	 * If it has lineinfo, it's a C-based entry — skip in that case.
> +	 */
> +	if (has_lineinfo(buf))
> +		kunit_skip(test, "_text has lineinfo (C entry?): %s", buf);
> +
> +	KUNIT_EXPECT_FALSE_MSG(test, has_lineinfo(buf),
> +			       "Unexpected lineinfo for asm symbol: %s", buf);
> +#else
> +	kunit_skip(test, "_text not accessible from modules");
> +#endif
> +}

[Severity: Low]
Can the KUNIT_EXPECT_FALSE_MSG(test, has_lineinfo(buf), ...) assertion in
test_assembly_no_lineinfo() ever fail?

If has_lineinfo(buf) is true, the preceding if (has_lineinfo(buf)) check calls
kunit_skip(), which aborts the test via kunit_try_catch_throw(), so
has_lineinfo(buf) is always false when KUNIT_EXPECT_FALSE_MSG() is reached.
Also, since CONFIG_KALLSYMS_LINEINFO depends on CONFIG_DEBUG_INFO (which passes
-g in KBUILD_AFLAGS), assembler .S entry points at _text normally have DWARF
line info as well, causing this test to always skip.

[ ... ]
> +static void test_many_lines_mid_function(struct kunit *test)
> +{
> +	char *buf = alloc_sym_buf(test);
> +	unsigned long addr = target_addr(lineinfo_target_many_lines);
> +	unsigned int line;
> +	unsigned long mid_addr;
> +
> +	skip_without_module_lineinfo(test);
> +
> +	/* Get function size from sprint_with_lineinfo output */
> +	sprint_with_lineinfo(buf, addr);
> +	KUNIT_ASSERT_TRUE(test, has_lineinfo(buf));
> +
> +	/*
> +	 * Try an address 8 bytes into the function (past prologue).  Hide
> +	 * the base from the compiler so this does not become a static
> +	 * mid-function relocation (sym+8), which objtool flags as a
> +	 * relocation to !ENDBR with CONFIG_X86_KERNEL_IBT.
> +	 */
> +	OPTIMIZER_HIDE_VAR(addr);
> +	mid_addr = addr + 8;
> +	sprint_with_lineinfo(buf, mid_addr);
> +
> +	/*
> +	 * Should still resolve to lineinfo_target_many_lines.
> +	 * Lineinfo should be present with a plausible line number.
> +	 */
> +	KUNIT_EXPECT_TRUE_MSG(test,
> +			      strnstr(buf, "lineinfo_target_many_lines",
> +				      KSYM_SYMBOL_LEN) != NULL,
> +			      "Mid-function addr resolved to wrong symbol: %s",
> +			      buf);
> +	if (has_lineinfo(buf)) {
> +		line = extract_line(buf);
> +		KUNIT_EXPECT_GE_MSG(test, line, (unsigned int)50,
> +				    "Line %u too low for mid-function", line);
> +		KUNIT_EXPECT_LE_MSG(test, line, (unsigned int)700,
> +				    "Line %u too high for mid-function", line);
> +	}
> +}

[Severity: Low]
In test_many_lines_mid_function(), if sprint_with_lineinfo(buf, mid_addr) fails
to produce lineinfo, won't the test silently pass because the line-number
checks are guarded by if (has_lineinfo(buf)) instead of asserting
KUNIT_ASSERT_TRUE(test, has_lineinfo(buf))?

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