Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Medium] kallsyms: gen_lineinfo excludes STT_NOTYPE and unsized symbols in sym_starts - [Medium] kallsyms: collect_symbol_starts() includes SHN_ABS and mapping symbols - [Medium] kallsyms: line-0 entries overwrite valid coverage in deduplicate() - [Low] scripts: missing libelf in pkg-config query for gen_lineinfo in Makefile --
commit 939ac5438a3c14ba3c356cc9994f3c7b50467285 Author: Sasha Levin <[email protected]> kallsyms: embed source file:line info in kernel stack traces This commit adds CONFIG_KALLSYMS_LINEINFO and a host tool (scripts/gen_lineinfo) to extract DWARF .debug_line address-to-source-line mappings into vmlinux .rodata so stack traces include (file:line) annotations. > diff --git a/scripts/Makefile b/scripts/Makefile > index d7fbb7ca098b1..94b09cf7c1839 100644 > --- a/scripts/Makefile > +++ b/scripts/Makefile > @@ -4,6 +4,7 @@ > # the kernel for the build process. > > hostprogs-always-y += kallsyms > +hostprogs-always-$(CONFIG_KALLSYMS_LINEINFO) += gen_lineinfo > hostprogs-always-$(BUILD_C_RECORDMCOUNT) += recordmcount > hostprogs-always-$(CONFIG_BUILDTIME_TABLE_SORT) += sorttable > hostprogs-always-$(CONFIG_ASN1) += asn1_compiler > @@ -40,6 +41,8 @@ HOSTCFLAGS_asn1_compiler.o = -I$(srctree)/include > HOSTCFLAGS_sign-file.o = $(shell $(HOSTPKG_CONFIG) --cflags libcrypto 2> > /dev/null) > HOSTCFLAGS_sign-file.o += -I$(srctree)/tools/include/uapi/ > HOSTLDLIBS_sign-file = $(shell $(HOSTPKG_CONFIG) --libs libcrypto 2> > /dev/null || echo -lcrypto) > +HOSTCFLAGS_gen_lineinfo.o = $(shell $(HOSTPKG_CONFIG) --cflags libdw 2> > /dev/null) > +HOSTLDLIBS_gen_lineinfo = $(shell $(HOSTPKG_CONFIG) --libs libdw 2> > /dev/null || echo -ldw -lelf) [Severity: Low] Should HOSTCFLAGS_gen_lineinfo.o and HOSTLDLIBS_gen_lineinfo in scripts/Makefile query pkg-config for both libdw and libelf? scripts/gen_lineinfo.c directly includes <elf.h> and <gelf.h> and calls libelf APIs in main(), collect_symbol_starts(), and find_vmlinux_sym() (such as elf_version(), elf_begin(), elf_nextscn(), gelf_getshdr(), elf_getdata(), gelf_getsym(), elf_strptr(), elf_errmsg(), elf_errno(), and elf_end()). In elfutils, libdw.pc lists libelf under Requires.private rather than Requires. When pkg-config is installed on the host, running: $(HOSTPKG_CONFIG) --libs libdw succeeds with exit status 0 and outputs only -ldw, which prevents the "|| echo -ldw -lelf" fallback from running. On toolchains that default to --no-copy-dt-needed-entries where libdw.so is a symlink to libdw.so.1 rather than a linker script, linking scripts/gen_lineinfo can fail with undefined references to the libelf symbols. Would querying "$(HOSTPKG_CONFIG) --cflags libdw libelf" and "$(HOSTPKG_CONFIG) --libs libdw libelf" avoid this build failure? > diff --git a/scripts/gen_lineinfo.c b/scripts/gen_lineinfo.c > new file mode 100644 > index 0000000000000..7562ac81acca5 > --- /dev/null > +++ b/scripts/gen_lineinfo.c [ ... ] > +static void collect_symbol_starts(Elf *elf, unsigned long long text_addr) > +{ > + Elf_Scn *scn = NULL; > + GElf_Shdr shdr; > + > + while ((scn = elf_nextscn(elf, scn)) != NULL) { > + Elf_Data *data; > + size_t nsyms; > + > + if (!gelf_getshdr(scn, &shdr)) > + continue; > + if (shdr.sh_type != SHT_SYMTAB || !shdr.sh_entsize) > + continue; > + > + data = elf_getdata(scn, NULL); > + if (!data) > + continue; > + > + nsyms = shdr.sh_size / shdr.sh_entsize; > + for (size_t i = 0; i < nsyms; i++) { > + GElf_Sym sym; > + unsigned long long raw; > + > + if (!gelf_getsym(data, i, &sym)) > + continue; > + switch (GELF_ST_TYPE(sym.st_info)) { > + case STT_FUNC: > + case STT_NOTYPE: > + break; > + default: > + continue; > + } > + if (sym.st_value < text_addr) > + continue; > + if (text_end_addr && sym.st_value >= text_end_addr) > + continue; > + > + raw = sym.st_value - text_addr; > + if (raw > UINT_MAX) > + continue; > + > + if (num_sym_starts >= sym_starts_capacity) { > + sym_starts_capacity = sym_starts_capacity ? > + sym_starts_capacity * 2 : 16384; > + sym_starts = xrealloc(sym_starts, > + sym_starts_capacity * > + sizeof(*sym_starts)); > + } > + if (num_text_starts >= text_starts_capacity) { > + text_starts_capacity = text_starts_capacity ? > + text_starts_capacity * 2 : 16384; > + text_starts = xrealloc(text_starts, > + text_starts_capacity * > + sizeof(*text_starts)); > + } > + text_starts[num_text_starts].offset = (unsigned int)raw; > + text_starts[num_text_starts].size = > + sym.st_size > UINT_MAX ? UINT_MAX : > + (unsigned int)sym.st_size; > + num_text_starts++; > + > + if (GELF_ST_TYPE(sym.st_info) != STT_FUNC) > + continue; > + > + sym_starts[num_sym_starts].offset = (unsigned int)raw; > + sym_starts[num_sym_starts].size = > + sym.st_size > UINT_MAX ? UINT_MAX : > + (unsigned int)sym.st_size; > + num_sym_starts++; > + } > + } > + > + sort_starts(sym_starts, &num_sym_starts); > + sort_starts(text_starts, &num_text_starts); > + > + /* > + * An unsized label reaches the next symbol, and a sequence beginning > + * there cannot run past it. > + */ > + for (unsigned int i = 1; i < num_text_starts; i++) { > + if (!text_starts[i - 1].size) > + text_starts[i - 1].size = text_starts[i - 1].offset ? > + text_starts[i].offset - > + text_starts[i - 1].offset : 0; [Severity: Medium] Can unfiltered ELF symbols in collect_symbol_starts() in scripts/gen_lineinfo.c corrupt the symbol extents in text_starts[]? In collect_symbol_starts(), every STT_FUNC and STT_NOTYPE symbol in SHT_SYMTAB whose st_value falls in [text_addr, text_end_addr) is added to text_starts[] without checking sym.st_shndx or the symbol name: - SHN_ABS symbols (such as __kcfi_typeid_* hashes) whose values happen to fall in [text_addr, text_end_addr) are included. - Architecture mapping symbols ($x, $d on arm64/arm/riscv/loongarch) and assembler local labels (.L*) are included. When the loop at the end of collect_symbol_starts() backfills sizes for unsized entries: for (unsigned int i = 1; i < num_text_starts; i++) { if (!text_starts[i - 1].size) text_starts[i - 1].size = text_starts[i].offset - text_starts[i - 1].offset; } an internal .L* or $x label inside an unsized assembly routine truncates that routine's extent at the first internal label, while an unsized mapping or SHN_ABS symbol at a sequence boundary is given a non-zero size reaching to the next symbol, which can cause starts_new_sequence() and resolve_cu_row_groups() to misclassify DWARF sequence boundaries. Additionally, why does collect_symbol_starts() exclude STT_NOTYPE and unsized symbols from sym_starts[]? At runtime, __sprint_symbol() -> kallsyms_lookup_buildid() resolves symbols using the kallsyms table, which includes STT_NOTYPE code symbols and unsized assembly labels (such as Clang .cold subfunctions, SYM_CODE_START routines, SYM_INNER_LABEL entry points like entry_SYSCALL_64_after_hwframe, and CONFIG_PROPELLER_CLANG basic-block symbols), and passes sym_start to kallsyms_lookup_lineinfo(): if (lineinfo_addrs[low] < min_offset) return false; Because collect_symbol_starts() skips GELF_ST_TYPE(sym.st_info) != STT_FUNC when populating sym_starts[] and never backfills zero sizes in sym_starts[], synthesize_symbol_starts() skips those symbols (or rejects st_size == 0 at "if (end <= start) continue;") and deduplicate() sets at_symbol_start = false at their start offsets. Any frame inside those symbols before a new file:line transition will then fail the min_offset check in kallsyms_lookup_lineinfo() and lose its annotation. [ ... ] > +static void synthesize_symbol_starts(void) > +{ > + unsigned int base_entries = num_entries; > + unsigned int cursor = 0; > + > + if (!base_entries || !num_sym_starts) > + return; > + > + sort_unique(seq_ends, &num_seq_ends); > + qsort(asm_spans, num_asm_spans, sizeof(*asm_spans), compare_asm_spans); > + > + for (unsigned int i = 0; i < num_sym_starts; i++) { > + unsigned int start = sym_starts[i].offset; > + unsigned int end = start + sym_starts[i].size; > + > + while (cursor + 1 < base_entries && > + entries[cursor + 1].offset <= start) > + cursor++; > + > + if (entries[cursor].offset > start) > + continue; /* nothing covers this symbol */ > + if (entries[cursor].offset == start) > + continue; /* already has its own entry */ > + if (seq_end_between(entries[cursor].offset, start)) > + continue; /* coverage stopped before here */ > + > + if (end <= start) > + continue; /* size overflowed */ > + > + /* > + * A row reaching over a symbol with none inside it is padding > + * or a split-out fragment in C, carrying the line of whatever > + * it came from. In assembly it is one macro expanding to a > + * function per invocation, which the row does describe. > + */ > + if (!in_asm_span(start) && > + (cursor + 1 >= base_entries || > + entries[cursor + 1].offset >= end)) > + continue; > + > + add_entry(start, entries[cursor].file_id, entries[cursor].line); > + } > +} > + > +static void deduplicate(void) > +{ > + unsigned int sym_cursor = 0; > + unsigned int i, j; > + > + if (num_entries < 2) > + return; > + > + /* Sort by offset, then file_id, then line for stability */ > + qsort(entries, num_entries, sizeof(*entries), compare_entries); > + > + synthesize_symbol_starts(); > + qsort(entries, num_entries, sizeof(*entries), compare_entries); > + > + /* > + * Remove duplicate entries: > + * - Same offset: keep last. Earlier rows at that address cover no > + * bytes, so the final one is what describes the instruction, and > + * it is what addr2line reports. For an inlined call that is the > + * inlined body rather than the call site. > + * - Same file:line as previous kept entry: redundant for binary > + * search -- any address between them resolves to the earlier one > + * > + * Entries sitting on a symbol start are exempt from the second rule: > + * they are the only thing standing between that symbol and the > + * kernel's boundary check. > + */ > + j = 0; > + for (i = 1; i < num_entries; i++) { > + bool at_symbol_start; > + > + if (entries[i].offset == entries[j].offset) { > + /* > + * Rows from different compile units can share an > + * address, and coverage is their union: a real > + * location beats another unit's no-coverage marker. > + * Within one unit resolve_cu_row_groups() has already > + * picked the row that describes the address. > + */ > + if (entries[i].line || !entries[j].line) > + entries[j] = entries[i]; > + continue; > + } > + > + while (sym_cursor < num_sym_starts && > + sym_starts[sym_cursor].offset < entries[i].offset) > + sym_cursor++; > + at_symbol_start = sym_cursor < num_sym_starts && > + sym_starts[sym_cursor].offset == > entries[i].offset; > + > + if (!at_symbol_start && > + entries[i].file_id == entries[j].file_id && > + entries[i].line == entries[j].line) > + continue; > + > + j++; > + if (j != i) > + entries[j] = entries[i]; > + } > + num_entries = j + 1; > +} [Severity: Medium] Can line-0 end_sequence entries overwrite valid source coverage in synthesize_symbol_starts() and deduplicate() in scripts/gen_lineinfo.c? There appear to be three ways a line-0 entry can clobber a valid line entry across compile units: 1. deduplicate() calls synthesize_symbol_starts() before collapsing same-offset entries across compile units. When an earlier CU emits a valid row (line > 0) at offset X and a later CU emits an end_sequence row (line == 0) at the same offset X, the line-0 entry has a higher seq and sorts last at offset X. In synthesize_symbol_starts(), the cursor loop: while (cursor + 1 < base_entries && entries[cursor + 1].offset <= start) cursor++; advances to the last entry at offset X, picking the line-0 entry instead of the valid line entry. 2. In process_dwarf(), every sequence end passed to record_seq_end() also emits a line-0 entry into entries[] at that same offset. Because cursor in synthesize_symbol_starts() stops at the largest entry offset <= start, there can never be a sequence end strictly between entries[cursor].offset and start without a corresponding entry in entries[] that would have advanced cursor to it. Thus seq_end_between(entries[cursor].offset, start) never triggers, and because synthesize_symbol_starts() does not check whether entries[cursor].line == 0, a line-0 sequence-end entry at entries[cursor].offset gets copied to start. 3. In deduplicate(), same-offset union resolution is interleaved with adjacent (file_id, line) deduplication. Suppose entries[j] is at offset A with (file_id=1, line=10), and at offset B > A (not a symbol start) CU1 has (file_id=1, line=10) followed by CU2 having an end_sequence (file_id=0, line=0) at the same offset B: - For CU1's entry at offset B, entries[i].offset != entries[j].offset, and since (file_id, line) matches entries[j], the loop hits continue without updating entries[j].offset to B. - For CU2's line-0 entry at offset B, entries[i].offset (B) != entries[j].offset (A), so it misses the "if (entries[i].offset == entries[j].offset)" union check and is appended as a new entry at offset B with line 0, cutting off valid coverage from offset B onward. Would collapsing same-offset entries across CUs in a separate pass before running synthesize_symbol_starts() and before collapsing adjacent identical (file_id, line) entries (and checking entries[cursor].line != 0 in synthesize_symbol_starts()) avoid losing coverage here? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
