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 <[email protected]> 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/[email protected]?part=4
