| Message ID | 20230725211157.17031-3-will@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix 'faddr2line' for LLVM arm64 builds | expand |
On Tue, Jul 25, 2023 at 10:11:57PM +0100, Will Deacon wrote: > @@ -185,7 +186,7 @@ __faddr2line() { > found=2 > break > fi > - done < <(${READELF} --symbols --wide $objfile | sed 's/\[.*\]//' | ${AWK} -v sec=$sym_sec '$7 == sec' | sort --key=2) > + done < <(${READELF} --symbols --wide $objfile | sed -f ${IGNORED_SYMS} -e 's/\[.*\]//' | ${AWK} -v sec=$sym_sec '$7 == sec' | sort --key=2) > > if [[ $found = 0 ]]; then > warn "can't find symbol: sym_name: $sym_name sym_sec: $sym_sec sym_addr: $sym_addr sym_elf_size: $sym_elf_size" Looks good, though the outer loop has another readelf incantation: done < <(${READELF} --symbols --wide $objfile | sed 's/\[.*\]//' | ${AWK} -v fn=$sym_name '$4 == "FUNC" && $8 == fn') It should probably have the same sed options? Also it looks like it's wrongly checking for FUNC.
On Tue, Jul 25, 2023 at 02:38:05PM -0700, Josh Poimboeuf wrote: > On Tue, Jul 25, 2023 at 10:11:57PM +0100, Will Deacon wrote: > > @@ -185,7 +186,7 @@ __faddr2line() { > > found=2 > > break > > fi > > - done < <(${READELF} --symbols --wide $objfile | sed 's/\[.*\]//' | ${AWK} -v sec=$sym_sec '$7 == sec' | sort --key=2) > > + done < <(${READELF} --symbols --wide $objfile | sed -f ${IGNORED_SYMS} -e 's/\[.*\]//' | ${AWK} -v sec=$sym_sec '$7 == sec' | sort --key=2) > > > > if [[ $found = 0 ]]; then > > warn "can't find symbol: sym_name: $sym_name sym_sec: $sym_sec sym_addr: $sym_addr sym_elf_size: $sym_elf_size" > > Looks good, though the outer loop has another readelf incantation: > > done < <(${READELF} --symbols --wide $objfile | sed 's/\[.*\]//' | ${AWK} -v fn=$sym_name '$4 == "FUNC" && $8 == fn') > > It should probably have the same sed options? Hmm, I don't think it's needed there, is it? The awk expression has a strict match on $sym_name, which is going to be something extracted from a kernel log and therefore exists in kallsyms. > Also it looks like it's wrongly checking for FUNC. Yes, I agree that should be dropped for the reasons you gave before. So I can spin a v3, with an extra patch to avoid checking against FUNC. Will
On Thu, Jul 27, 2023 at 01:18:52PM +0100, Will Deacon wrote: > On Tue, Jul 25, 2023 at 02:38:05PM -0700, Josh Poimboeuf wrote: > > On Tue, Jul 25, 2023 at 10:11:57PM +0100, Will Deacon wrote: > > > @@ -185,7 +186,7 @@ __faddr2line() { > > > found=2 > > > break > > > fi > > > - done < <(${READELF} --symbols --wide $objfile | sed 's/\[.*\]//' | ${AWK} -v sec=$sym_sec '$7 == sec' | sort --key=2) > > > + done < <(${READELF} --symbols --wide $objfile | sed -f ${IGNORED_SYMS} -e 's/\[.*\]//' | ${AWK} -v sec=$sym_sec '$7 == sec' | sort --key=2) > > > > > > if [[ $found = 0 ]]; then > > > warn "can't find symbol: sym_name: $sym_name sym_sec: $sym_sec sym_addr: $sym_addr sym_elf_size: $sym_elf_size" > > > > Looks good, though the outer loop has another readelf incantation: > > > > done < <(${READELF} --symbols --wide $objfile | sed 's/\[.*\]//' | ${AWK} -v fn=$sym_name '$4 == "FUNC" && $8 == fn') > > > > It should probably have the same sed options? > > Hmm, I don't think it's needed there, is it? The awk expression has a > strict match on $sym_name, which is going to be something extracted from > a kernel log and therefore exists in kallsyms. Yes, I think you're right. > > Also it looks like it's wrongly checking for FUNC. > > Yes, I agree that should be dropped for the reasons you gave before. > > So I can spin a v3, with an extra patch to avoid checking against FUNC. Sounds good, thanks!
diff --git a/scripts/faddr2line b/scripts/faddr2line index 62a3fa6f6f59..da734af90036 100755 --- a/scripts/faddr2line +++ b/scripts/faddr2line @@ -64,6 +64,7 @@ else UTIL_PREFIX=${CROSS_COMPILE:-} fi +IGNORED_SYMS=$(dirname $0)/sysmap-ignored-syms.sed READELF="${UTIL_PREFIX}readelf" ADDR2LINE="${UTIL_PREFIX}addr2line" AWK="awk" @@ -185,7 +186,7 @@ __faddr2line() { found=2 break fi - done < <(${READELF} --symbols --wide $objfile | sed 's/\[.*\]//' | ${AWK} -v sec=$sym_sec '$7 == sec' | sort --key=2) + done < <(${READELF} --symbols --wide $objfile | sed -f ${IGNORED_SYMS} -e 's/\[.*\]//' | ${AWK} -v sec=$sym_sec '$7 == sec' | sort --key=2) if [[ $found = 0 ]]; then warn "can't find symbol: sym_name: $sym_name sym_sec: $sym_sec sym_addr: $sym_addr sym_elf_size: $sym_elf_size"
Some symbols emitted in the readelf output but filtered from System.map can confuse the 'faddr2line' symbol size calculation, resulting in the erroneous rejection of valid offsets. This is especially prevalent when building an arm64 kernel with CONFIG_CFI_CLANG=y, where most functions are prefixed with a 32-bit data value in a '$d.n' section. For example: 447538: ffff800080014b80 548 FUNC GLOBAL DEFAULT 2 do_one_initcall 104: ffff800080014c74 0 NOTYPE LOCAL DEFAULT 2 $x.73 106: ffff800080014d30 0 NOTYPE LOCAL DEFAULT 2 $x.75 111: ffff800080014da4 0 NOTYPE LOCAL DEFAULT 2 $d.78 112: ffff800080014da8 0 NOTYPE LOCAL DEFAULT 2 $x.79 36: ffff800080014de0 200 FUNC LOCAL DEFAULT 2 run_init_process Adding a warning to do_one_initcall() results in: | WARNING: CPU: 0 PID: 1 at init/main.c:1236 do_one_initcall+0xf4/0x260 Which 'faddr2line' refuses to accept: $ ./scripts/faddr2line vmlinux do_one_initcall+0xf4/0x260 skipping do_one_initcall address at 0xffff800080014c74 due to size mismatch (0x260 != 0x224) no match for do_one_initcall+0xf4/0x260 Filter out entries from readelf using the 'sysmap-ignored-syms.sed' script used to construct System.map, so that the size of a symbol is calculated as a delta to the next symbol present in ksymtab. Cc: Josh Poimboeuf <jpoimboe@kernel.org> Cc: John Stultz <jstultz@google.com> Signed-off-by: Will Deacon <will@kernel.org> --- scripts/faddr2line | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)