* frame unwinding patches
@ 2017-02-15 23:34 Mark Wielaard
2017-02-15 23:34 ` [PATCH 3/3] Add frame pointer unwinding for aarch64 Mark Wielaard
` (4 more replies)
0 siblings, 5 replies; 37+ messages in thread
From: Mark Wielaard @ 2017-02-15 23:34 UTC (permalink / raw)
To: elfutils-devel; +Cc: Ulf Hermann
Hi,
I put all three frame pointer unwinding fallback patches on
the mjw/fp-unwind branch. I'll also sent them to the list using
git send-mail --annotate taking out the binary file patches.
Hopefully that will make them appear on the list, bypassing the
spam filters.
[PATCH 1/3] Add frame pointer unwinding as fallback on x86_64
[PATCH 2/3] Add frame pointer unwinding as fallback on arm
[PATCH 3/3] Add frame pointer unwinding for aarch64
I had to hand apply a few things because of whitespace adjustments.
Hopefully I did it right and this is how Ulf intended the patches.
If not, my apologies, and please let me know what changes you did
intend.
Cheers,
Mark
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH 3/3] Add frame pointer unwinding for aarch64 2017-02-15 23:34 frame unwinding patches Mark Wielaard @ 2017-02-15 23:34 ` Mark Wielaard 2017-02-15 23:34 ` [PATCH 2/3] Add frame pointer unwinding as fallback on arm Mark Wielaard ` (3 subsequent siblings) 4 siblings, 0 replies; 37+ messages in thread From: Mark Wielaard @ 2017-02-15 23:34 UTC (permalink / raw) To: elfutils-devel; +Cc: Ulf Hermann From: Ulf Hermann <ulf.hermann@qt.io> If we don't find any debug information for a given frame, we usually cannot unwind any further. However, the binary in question might have been compiled with frame pointers, in which case we can look up the well known frame pointer locations in the stack snapshot and use them to bridge the frames without debug information. The unwinding on aarch64 is pretty similar to the one on arm, the difference is in the offsets of LR and FP on the stack. Signed-off-by: Ulf Hermann <ulf.hermann@qt.io> --- backends/ChangeLog | 6 +++++ backends/Makefile.am | 2 +- backends/aarch64_init.c | 1 + backends/aarch64_unwind.c | 41 +++++++++++++++++++++++++++++++++ tests/ChangeLog | 7 ++++++ tests/Makefile.am | 5 ++-- tests/backtrace.aarch64.fp.core.bz2 | Bin 0 -> 7302 bytes tests/backtrace.aarch64.fp.exec.bz2 | Bin 0 -> 272102 bytes tests/run-backtrace-fp-core-aarch64.sh | 33 ++++++++++++++++++++++++++ 9 files changed, 92 insertions(+), 3 deletions(-) create mode 100644 backends/aarch64_unwind.c create mode 100644 tests/backtrace.aarch64.fp.core.bz2 create mode 100755 tests/backtrace.aarch64.fp.exec.bz2 create mode 100755 tests/run-backtrace-fp-core-aarch64.sh diff --git a/backends/ChangeLog b/backends/ChangeLog index 6837a49..02efb00 100644 --- a/backends/ChangeLog +++ b/backends/ChangeLog @@ -1,5 +1,11 @@ 2017-02-09 Ulf Hermann <ulf.hermann@qt.io> + * aarch64_unwind.c: New file + * Makefile.am (aarch64_SRCS): Add aarch64_unwind.c + * aarch64_init.c (aarch64_init): Hook aarch64_unwind + +2017-02-09 Ulf Hermann <ulf.hermann@qt.io> + * arm_unwind.c: New file * Makefile.am (arm_SRCS): Add arm_unwind.c * arm_init.c (arm_init): Hook arm_unwind diff --git a/backends/Makefile.am b/backends/Makefile.am index 14cbf04..bfb6b84 100644 --- a/backends/Makefile.am +++ b/backends/Makefile.am @@ -81,7 +81,7 @@ am_libebl_arm_pic_a_OBJECTS = $(arm_SRCS:.c=.os) aarch64_SRCS = aarch64_init.c aarch64_regs.c aarch64_symbol.c \ aarch64_corenote.c aarch64_retval.c aarch64_cfi.c \ - aarch64_initreg.c + aarch64_initreg.c aarch64_unwind.c libebl_aarch64_pic_a_SOURCES = $(aarch64_SRCS) am_libebl_aarch64_pic_a_OBJECTS = $(aarch64_SRCS:.c=.os) diff --git a/backends/aarch64_init.c b/backends/aarch64_init.c index 6395f11..0866494 100644 --- a/backends/aarch64_init.c +++ b/backends/aarch64_init.c @@ -63,6 +63,7 @@ aarch64_init (Elf *elf __attribute__ ((unused)), + ALT_FRAME_RETURN_COLUMN (used when LR isn't used) = 97 DWARF regs. */ eh->frame_nregs = 97; HOOK (eh, set_initial_registers_tid); + HOOK (eh, unwind); return MODVERSION; } diff --git a/backends/aarch64_unwind.c b/backends/aarch64_unwind.c new file mode 100644 index 0000000..bd95878 --- /dev/null +++ b/backends/aarch64_unwind.c @@ -0,0 +1,41 @@ +/* Get previous frame state for an existing frame state. + Copyright (C) 2016 The Qt Company Ltd. + This file is part of elfutils. + + This file is free software; you can redistribute it and/or modify + it under the terms of either + + * the GNU Lesser General Public License as published by the Free + Software Foundation; either version 3 of the License, or (at + your option) any later version + + or + + * the GNU General Public License as published by the Free + Software Foundation; either version 2 of the License, or (at + your option) any later version + + or both in parallel, as here. + + elfutils is distributed in the hope that it will be useful, but + WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + General Public License for more details. + + You should have received copies of the GNU General Public License and + the GNU Lesser General Public License along with this program. If + not, see <http://www.gnu.org/licenses/>. */ + +#ifdef HAVE_CONFIG_H +# include <config.h> +#endif + +#define BACKEND aarch64_ +#define FP_REG 29 +#define LR_REG 30 +#define SP_REG 31 +#define FP_OFFSET 0 +#define LR_OFFSET 8 +#define SP_OFFSET 16 + +#include "arm_unwind.c" diff --git a/tests/ChangeLog b/tests/ChangeLog index 2c210ea..71dba42 100644 --- a/tests/ChangeLog +++ b/tests/ChangeLog @@ -1,3 +1,10 @@ +2017-02-13 Ulf Hermann <ulf.hermann@qt.io> + + * Makefile.am: Add test for unwinding with frame pointers on aarch64 + * backtrace.aarch64.fp.core.bz2: New file + * backtrace.aarch64.fp.exec.bz2: New file + * run-backtrace-fp-core-aarch64.sh: New file + 2017-02-09 Ulf Hermann <ulf.hermann@qt.io> * Makefile.am: Add test for unwinding with frame pointers on arm diff --git a/tests/Makefile.am b/tests/Makefile.am index b72befc..d120ed9 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -114,7 +114,7 @@ TESTS = run-arextract.sh run-arsymtest.sh newfile test-nlist \ run-backtrace-native-biarch.sh run-backtrace-native-core.sh \ run-backtrace-native-core-biarch.sh run-backtrace-core-x86_64.sh \ run-backtrace-fp-core-x86_64.sh \ - run-backtrace-fp-core-arm.sh \ + run-backtrace-fp-core-arm.sh run-backtrace-fp-core-aarch64.sh \ run-backtrace-core-x32.sh \ run-backtrace-core-i386.sh run-backtrace-core-ppc.sh \ run-backtrace-core-s390x.sh run-backtrace-core-s390.sh \ @@ -294,8 +294,9 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh \ run-backtrace-core-x86_64.sh run-backtrace-core-i386.sh \ run-backtrace-fp-core-x86_64.sh \ run-backtrace-core-x32.sh \ - run-backtrace-fp-core-arm.sh \ + run-backtrace-fp-core-arm.sh run-backtrace-fp-core-aarch64.sh \ backtrace.arm.fp.core.bz2 backtrace.arm.fp.exec.bz2 \ + backtrace.aarch64.fp.core.bz2 backtrace.aarch64.fp.exec.bz2 \ backtrace-subr.sh backtrace.i386.core.bz2 backtrace.i386.exec.bz2 \ backtrace.x86_64.core.bz2 backtrace.x86_64.exec.bz2 \ backtrace.x86_64.fp.core.bz2 backtrace.x86_64.fp.exec.bz2 \ diff --git a/tests/run-backtrace-fp-core-aarch64.sh b/tests/run-backtrace-fp-core-aarch64.sh new file mode 100755 index 0000000..64887d2 --- /dev/null +++ b/tests/run-backtrace-fp-core-aarch64.sh @@ -0,0 +1,33 @@ +#! /bin/bash +# Copyright (C) 2017 The Qt Company +# This file is part of elfutils. +# +# This file is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# elfutils is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +. $srcdir/backtrace-subr.sh + +# The binary is generated by compiling backtrace-child without debug +# information, but with -fno-omit-frame-pointer. +# +# gcc -static -O2 -fno-omit-frame-pointer -D_GNU_SOURCE \ +# -pthread -o backtrace.aarch64.fp.exec backtrace-child.c \ +# +# Then strip the .eh_frame and .eh_frame_hdr sections from the binary: +# +# strip -R .eh_frame backtrace.aarch64.fp.exec +# strip -R .eh_frame_hdr backtrace.aarch64.fp.exec +# +# The core is generated by calling the binary with --gencore + +check_core aarch64.fp --allow-unknown -- 1.8.3.1 ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 2/3] Add frame pointer unwinding as fallback on arm 2017-02-15 23:34 frame unwinding patches Mark Wielaard 2017-02-15 23:34 ` [PATCH 3/3] Add frame pointer unwinding for aarch64 Mark Wielaard @ 2017-02-15 23:34 ` Mark Wielaard 2017-02-15 23:34 ` [PATCH 1/3] Add frame pointer unwinding as fallback on x86_64 Mark Wielaard ` (2 subsequent siblings) 4 siblings, 0 replies; 37+ messages in thread From: Mark Wielaard @ 2017-02-15 23:34 UTC (permalink / raw) To: elfutils-devel; +Cc: Ulf Hermann From: Ulf Hermann <ulf.hermann@qt.io> If we don't find any debug information for a given frame, we usually cannot unwind any further. However, the binary in question might have been compiled with frame pointers, in which case we can look up the well known frame pointer locations in the stack snapshot and use them to bridge the frames without debug information. At the moment this works only for ARM code. THUMB code uses a different mechanism for unwinding. Also, in order to figure out if a function was compiled in ARM or in THUMB mode we need a symbol table which might not be available (e.g. with JIT-compiled code). Signed-off-by: Ulf Hermann <ulf.hermann@qt.io> --- backends/ChangeLog | 6 +++ backends/Makefile.am | 3 +- backends/arm_init.c | 1 + backends/arm_unwind.c | 88 +++++++++++++++++++++++++++++++++++++ tests/ChangeLog | 7 +++ tests/Makefile.am | 3 ++ tests/backtrace.arm.fp.core.bz2 | Bin 0 -> 10997 bytes tests/backtrace.arm.fp.exec.bz2 | Bin 0 -> 245414 bytes tests/run-backtrace-fp-core-arm.sh | 37 ++++++++++++++++ 9 files changed, 144 insertions(+), 1 deletion(-) create mode 100644 backends/arm_unwind.c create mode 100644 tests/backtrace.arm.fp.core.bz2 create mode 100755 tests/backtrace.arm.fp.exec.bz2 create mode 100755 tests/run-backtrace-fp-core-arm.sh diff --git a/backends/ChangeLog b/backends/ChangeLog index 371f071..6837a49 100644 --- a/backends/ChangeLog +++ b/backends/ChangeLog @@ -1,5 +1,11 @@ 2017-02-09 Ulf Hermann <ulf.hermann@qt.io> + * arm_unwind.c: New file + * Makefile.am (arm_SRCS): Add arm_unwind.c + * arm_init.c (arm_init): Hook arm_unwind + +2017-02-09 Ulf Hermann <ulf.hermann@qt.io> + * x86_64_unwind.c: New file * Makefile.am (x86_64_SRCS): Add x86_64_unwind.c * x86_64_init.c (x86_64_init): Hook x86_64_unwind diff --git a/backends/Makefile.am b/backends/Makefile.am index 60917b9..14cbf04 100644 --- a/backends/Makefile.am +++ b/backends/Makefile.am @@ -74,7 +74,8 @@ libebl_alpha_pic_a_SOURCES = $(alpha_SRCS) am_libebl_alpha_pic_a_OBJECTS = $(alpha_SRCS:.c=.os) arm_SRCS = arm_init.c arm_symbol.c arm_regs.c arm_corenote.c \ - arm_auxv.c arm_attrs.c arm_retval.c arm_cfi.c arm_initreg.c + arm_auxv.c arm_attrs.c arm_retval.c arm_cfi.c arm_initreg.c \ + arm_unwind.c libebl_arm_pic_a_SOURCES = $(arm_SRCS) am_libebl_arm_pic_a_OBJECTS = $(arm_SRCS:.c=.os) diff --git a/backends/arm_init.c b/backends/arm_init.c index caadac6..4fa0601 100644 --- a/backends/arm_init.c +++ b/backends/arm_init.c @@ -68,6 +68,7 @@ arm_init (Elf *elf __attribute__ ((unused)), /* We only unwind the core integer registers. */ eh->frame_nregs = 16; HOOK (eh, set_initial_registers_tid); + HOOK (eh, unwind); /* Bit zero encodes whether an function address is THUMB or ARM. */ eh->func_addr_mask = ~(GElf_Addr)1; diff --git a/backends/arm_unwind.c b/backends/arm_unwind.c new file mode 100644 index 0000000..c20ebc2 --- /dev/null +++ b/backends/arm_unwind.c @@ -0,0 +1,88 @@ +/* Get previous frame state for an existing frame state. + Copyright (C) 2016 The Qt Company Ltd. + This file is part of elfutils. + + This file is free software; you can redistribute it and/or modify + it under the terms of either + + * the GNU Lesser General Public License as published by the Free + Software Foundation; either version 3 of the License, or (at + your option) any later version + + or + + * the GNU General Public License as published by the Free + Software Foundation; either version 2 of the License, or (at + your option) any later version + + or both in parallel, as here. + + elfutils is distributed in the hope that it will be useful, but + WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + General Public License for more details. + + You should have received copies of the GNU General Public License and + the GNU Lesser General Public License along with this program. If + not, see <http://www.gnu.org/licenses/>. */ + +#ifdef HAVE_CONFIG_H +# include <config.h> +#endif + +#include <stdlib.h> +#include <assert.h> + +#ifndef BACKEND +#define BACKEND arm_ +#define FP_REG 11 +#define LR_REG 14 +#define SP_REG 13 +#define FP_OFFSET 4 +#define LR_OFFSET 0 +#define SP_OFFSET 8 +#endif + +#include "libebl_CPU.h" + +/* There was no CFI. Maybe we happen to have a frame pointer and can unwind from that? */ + +bool +EBLHOOK(unwind) (Ebl *ebl __attribute__ ((unused)), Dwarf_Addr pc __attribute__ ((unused)), + ebl_tid_registers_t *setfunc, ebl_tid_registers_get_t *getfunc, + ebl_pid_memory_read_t *readfunc, void *arg, + bool *signal_framep __attribute__ ((unused))) +{ + Dwarf_Word fp, lr, sp; + + if (!getfunc(LR_REG, 1, &lr, arg)) + return false; + + if (!setfunc(-1, 1, &lr, arg)) + return false; + + if (!getfunc(FP_REG, 1, &fp, arg)) + fp = 0; + + if (!getfunc(SP_REG, 1, &sp, arg)) + sp = 0; + + Dwarf_Word newLr, newFp, newSp; + + if (!readfunc(fp + LR_OFFSET, &newLr, arg)) + newLr = 0; + + if (!readfunc(fp + FP_OFFSET, &newFp, arg)) + newFp = 0; + + newSp = fp + SP_OFFSET; + + // These are not fatal if they don't work. They will just prevent unwinding at the next frame. + setfunc(LR_REG, 1, &newLr, arg); + setfunc(FP_REG, 1, &newFp, arg); + setfunc(SP_REG, 1, &newSp, arg); + + // If the fp is invalid, we might still have a valid lr. + // But if the fp is valid, then the stack should be moving in the right direction. + return lr != 0 && (fp == 0 || newSp > sp); +} diff --git a/tests/ChangeLog b/tests/ChangeLog index e24f51b..2c210ea 100644 --- a/tests/ChangeLog +++ b/tests/ChangeLog @@ -1,5 +1,12 @@ 2017-02-09 Ulf Hermann <ulf.hermann@qt.io> + * Makefile.am: Add test for unwinding with frame pointers on arm + * backtrace.arm.fp.core.bz2: New file + * backtrace.arm.fp.exec.bz2: New file + * run-backtrace-fp-core-arm.sh: New file + +2017-02-09 Ulf Hermann <ulf.hermann@qt.io> + * Makefile.am: Add test for unwinding with frame pointers on x86_64 * backtrace.x86_64.fp.core.bz2: New file * backtrace.x86_64.fp.exec.bz2: New file diff --git a/tests/Makefile.am b/tests/Makefile.am index dd9ae81..b72befc 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -114,6 +114,7 @@ TESTS = run-arextract.sh run-arsymtest.sh newfile test-nlist \ run-backtrace-native-biarch.sh run-backtrace-native-core.sh \ run-backtrace-native-core-biarch.sh run-backtrace-core-x86_64.sh \ run-backtrace-fp-core-x86_64.sh \ + run-backtrace-fp-core-arm.sh \ run-backtrace-core-x32.sh \ run-backtrace-core-i386.sh run-backtrace-core-ppc.sh \ run-backtrace-core-s390x.sh run-backtrace-core-s390.sh \ @@ -293,6 +294,8 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh \ run-backtrace-core-x86_64.sh run-backtrace-core-i386.sh \ run-backtrace-fp-core-x86_64.sh \ run-backtrace-core-x32.sh \ + run-backtrace-fp-core-arm.sh \ + backtrace.arm.fp.core.bz2 backtrace.arm.fp.exec.bz2 \ backtrace-subr.sh backtrace.i386.core.bz2 backtrace.i386.exec.bz2 \ backtrace.x86_64.core.bz2 backtrace.x86_64.exec.bz2 \ backtrace.x86_64.fp.core.bz2 backtrace.x86_64.fp.exec.bz2 \ diff --git a/tests/run-backtrace-fp-core-arm.sh b/tests/run-backtrace-fp-core-arm.sh new file mode 100755 index 0000000..33e2ddf --- /dev/null +++ b/tests/run-backtrace-fp-core-arm.sh @@ -0,0 +1,37 @@ +#! /bin/bash +# Copyright (C) 2017 The Qt Company +# This file is part of elfutils. +# +# This file is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# elfutils is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +. $srcdir/backtrace-subr.sh + +# The binary is generated by compiling backtrace-child without debug +# information, but with -mapcs-frame -fno-omit-frame-pointer. +# -mapcs-frame makes some functions store the frame pointer in r11. +# gcc docs say one should also specify -fno-omit-frame-pointer, but +# result seems to be independent of this. +# +# gcc -static -O2 -mapcs-frame -fno-omit-frame-pointer \ +# -mfloat-abi=hard -I ../ -I ../lib/ -D_GNU_SOURCE \ +# -pthread -o backtrace.arm.fp.exec backtrace-child.c \ +# --sysroot=/my/arm/sysroot +# +# Then strip the .eh_frame section from the binary: +# +# strip -R .eh_frame backtrace.arm.fp.exec +# +# The core is generated by calling the binary with --gencore + +check_core arm.fp --allow-unknown -- 1.8.3.1 ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 1/3] Add frame pointer unwinding as fallback on x86_64 2017-02-15 23:34 frame unwinding patches Mark Wielaard 2017-02-15 23:34 ` [PATCH 3/3] Add frame pointer unwinding for aarch64 Mark Wielaard 2017-02-15 23:34 ` [PATCH 2/3] Add frame pointer unwinding as fallback on arm Mark Wielaard @ 2017-02-15 23:34 ` Mark Wielaard 2017-02-16 9:13 ` frame unwinding patches Ulf Hermann 2017-04-03 9:00 ` Milian Wolff 4 siblings, 0 replies; 37+ messages in thread From: Mark Wielaard @ 2017-02-15 23:34 UTC (permalink / raw) To: elfutils-devel; +Cc: Ulf Hermann From: Ulf Hermann <ulf.hermann@qt.io> If we don't find any debug information for a given frame, we usually cannot unwind any further. However, the binary in question might have been compiled with frame pointers, in which case we can look up the well known frame pointer locations in the stack snapshot and use them to bridge the frames without debug information. The "unwind" hook is the right place for this as it is so far only used on s390 and called only after trying to unwind with debug information. Signed-off-by: Ulf Hermann <ulf.hermann@qt.io> --- backends/ChangeLog | 6 +++ backends/Makefile.am | 2 +- backends/x86_64_init.c | 1 + backends/x86_64_unwind.c | 86 ++++++++++++++++++++++++++++++++++ tests/ChangeLog | 7 +++ tests/Makefile.am | 3 ++ tests/backtrace.x86_64.fp.core.bz2 | Bin 0 -> 9861 bytes tests/backtrace.x86_64.fp.exec.bz2 | Bin 0 -> 309484 bytes tests/run-backtrace-fp-core-x86_64.sh | 33 +++++++++++++ 9 files changed, 137 insertions(+), 1 deletion(-) create mode 100644 backends/x86_64_unwind.c create mode 100644 tests/backtrace.x86_64.fp.core.bz2 create mode 100755 tests/backtrace.x86_64.fp.exec.bz2 create mode 100755 tests/run-backtrace-fp-core-x86_64.sh diff --git a/backends/ChangeLog b/backends/ChangeLog index 1c561b5..371f071 100644 --- a/backends/ChangeLog +++ b/backends/ChangeLog @@ -1,3 +1,9 @@ +2017-02-09 Ulf Hermann <ulf.hermann@qt.io> + + * x86_64_unwind.c: New file + * Makefile.am (x86_64_SRCS): Add x86_64_unwind.c + * x86_64_init.c (x86_64_init): Hook x86_64_unwind + 2016-11-02 Mark Wielaard <mjw@redhat.com> * i386_regs.c (i386_register_info): Add fallthrough comment. diff --git a/backends/Makefile.am b/backends/Makefile.am index b553ec3..60917b9 100644 --- a/backends/Makefile.am +++ b/backends/Makefile.am @@ -59,7 +59,7 @@ am_libebl_sh_pic_a_OBJECTS = $(sh_SRCS:.c=.os) x86_64_SRCS = x86_64_init.c x86_64_symbol.c x86_64_corenote.c x86_64_cfi.c \ x86_64_retval.c x86_64_regs.c i386_auxv.c x86_64_syscall.c \ - x86_64_initreg.c x32_corenote.c + x86_64_initreg.c x86_64_unwind.c x32_corenote.c cpu_x86_64 = ../libcpu/libcpu_x86_64.a libebl_x86_64_pic_a_SOURCES = $(x86_64_SRCS) am_libebl_x86_64_pic_a_OBJECTS = $(x86_64_SRCS:.c=.os) diff --git a/backends/x86_64_init.c b/backends/x86_64_init.c index cfd0158..adfa479 100644 --- a/backends/x86_64_init.c +++ b/backends/x86_64_init.c @@ -68,6 +68,7 @@ x86_64_init (Elf *elf __attribute__ ((unused)), /* gcc/config/ #define DWARF_FRAME_REGISTERS. */ eh->frame_nregs = 17; HOOK (eh, set_initial_registers_tid); + HOOK (eh, unwind); return MODVERSION; } diff --git a/backends/x86_64_unwind.c b/backends/x86_64_unwind.c new file mode 100644 index 0000000..ade64c0 --- /dev/null +++ b/backends/x86_64_unwind.c @@ -0,0 +1,86 @@ +/* Get previous frame state for an existing frame state. + Copyright (C) 2016 The Qt Company Ltd. + This file is part of elfutils. + + This file is free software; you can redistribute it and/or modify + it under the terms of either + + * the GNU Lesser General Public License as published by the Free + Software Foundation; either version 3 of the License, or (at + your option) any later version + + or + + * the GNU General Public License as published by the Free + Software Foundation; either version 2 of the License, or (at + your option) any later version + + or both in parallel, as here. + + elfutils is distributed in the hope that it will be useful, but + WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + General Public License for more details. + + You should have received copies of the GNU General Public License and + the GNU Lesser General Public License along with this program. If + not, see <http://www.gnu.org/licenses/>. */ + +#ifdef HAVE_CONFIG_H +# include <config.h> +#endif + +#include <stdlib.h> +#include <assert.h> + +#define BACKEND x86_64_ +#include "libebl_CPU.h" + +/* There was no CFI. Maybe we happen to have a frame pointer and can unwind from that? */ + +bool +x86_64_unwind (Ebl *ebl __attribute__ ((unused)), + Dwarf_Addr pc __attribute__ ((unused)), + ebl_tid_registers_t *setfunc, ebl_tid_registers_get_t *getfunc, + ebl_pid_memory_read_t *readfunc, void *arg, + bool *signal_framep __attribute__ ((unused))) +{ + // Register 6 is supposed to be rbp, thus the conventional frame pointer + const int fpReg = 6; + const int spReg = 7; + + Dwarf_Word fp; + if (!getfunc(fpReg, 1, &fp, arg) || fp == 0) + return false; + + // Try to read old sp, so that we can avoid infinite loops below + Dwarf_Word sp; + if (!getfunc(spReg, 1, &sp, arg)) + sp = 0; + + Dwarf_Word prev_fp; + if (!readfunc(fp, &prev_fp, arg)) + prev_fp = 0; + + Dwarf_Word ret; + if (!readfunc(fp + 8, &ret, arg)) + return false; + + if (!setfunc(fpReg, 1, &prev_fp, arg)) + return false; + + fp += 16; // Pop fp and return address and write result to sp + if (!setfunc(spReg, 1, &fp, arg)) + return false; + + if (!setfunc(-1, 1, &ret, arg)) + return false; + + // If the sp didn't move up we don't actually have a new stack + // frame but rather some random data that doesn't include frame + // pointers. Break the unwinding then. + if (sp >= fp) + return false; + + return true; +} diff --git a/tests/ChangeLog b/tests/ChangeLog index bca47be..e24f51b 100644 --- a/tests/ChangeLog +++ b/tests/ChangeLog @@ -1,5 +1,12 @@ 2017-02-09 Ulf Hermann <ulf.hermann@qt.io> + * Makefile.am: Add test for unwinding with frame pointers on x86_64 + * backtrace.x86_64.fp.core.bz2: New file + * backtrace.x86_64.fp.exec.bz2: New file + * run-backtrace-fp-core-x86_64.sh: New file + +2017-02-09 Ulf Hermann <ulf.hermann@qt.io> + * backtrace.c: Add an option to allow unknown symbols in the trace * backtrace-substr.sh: Add an option to allow unknown symbols to check_core() and allow failed symbol lookups in check_err() diff --git a/tests/Makefile.am b/tests/Makefile.am index d4659cd..dd9ae81 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -113,6 +113,7 @@ TESTS = run-arextract.sh run-arsymtest.sh newfile test-nlist \ run-backtrace-native.sh run-backtrace-data.sh run-backtrace-dwarf.sh \ run-backtrace-native-biarch.sh run-backtrace-native-core.sh \ run-backtrace-native-core-biarch.sh run-backtrace-core-x86_64.sh \ + run-backtrace-fp-core-x86_64.sh \ run-backtrace-core-x32.sh \ run-backtrace-core-i386.sh run-backtrace-core-ppc.sh \ run-backtrace-core-s390x.sh run-backtrace-core-s390.sh \ @@ -290,9 +291,11 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh \ run-backtrace-native.sh run-backtrace-native-biarch.sh \ run-backtrace-native-core.sh run-backtrace-native-core-biarch.sh \ run-backtrace-core-x86_64.sh run-backtrace-core-i386.sh \ + run-backtrace-fp-core-x86_64.sh \ run-backtrace-core-x32.sh \ backtrace-subr.sh backtrace.i386.core.bz2 backtrace.i386.exec.bz2 \ backtrace.x86_64.core.bz2 backtrace.x86_64.exec.bz2 \ + backtrace.x86_64.fp.core.bz2 backtrace.x86_64.fp.exec.bz2 \ backtrace.ppc.core.bz2 backtrace.ppc.exec.bz2 \ run-backtrace-core-ppc.sh testfile66.bz2 testfile66.core.bz2 \ backtrace.s390x.core.bz2 backtrace.s390x.exec.bz2 \ diff --git a/tests/run-backtrace-fp-core-x86_64.sh b/tests/run-backtrace-fp-core-x86_64.sh new file mode 100755 index 0000000..f91f96b --- /dev/null +++ b/tests/run-backtrace-fp-core-x86_64.sh @@ -0,0 +1,33 @@ +#! /bin/bash +# Copyright (C) 2017 The Qt Company +# This file is part of elfutils. +# +# This file is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# elfutils is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +. $srcdir/backtrace-subr.sh + +# The binary is generated by compiling backtrace-child without debug +# information, but with frame pointers: +# +# gcc -static -O2 -fno-omit-frame-pointer -I ../ -I ../lib/ -D_GNU_SOURCE +# -pthread -o backtrace.x86_64.fp.exec backtrace-child.c +# +# Then strip the .eh_frame and .eh_frame_hdr sections from the binary: +# +# strip -R .eh_frame backtrace.x86_64.fp.exec +# strip -R .eh_frame_hdr backtrace.x86_64.fp.exec +# +# The core is generated by calling the binary with --gencore + +check_core x86_64.fp --allow-unknown -- 1.8.3.1 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: frame unwinding patches 2017-02-15 23:34 frame unwinding patches Mark Wielaard ` (2 preceding siblings ...) 2017-02-15 23:34 ` [PATCH 1/3] Add frame pointer unwinding as fallback on x86_64 Mark Wielaard @ 2017-02-16 9:13 ` Ulf Hermann 2017-04-03 9:00 ` Milian Wolff 4 siblings, 0 replies; 37+ messages in thread From: Ulf Hermann @ 2017-02-16 9:13 UTC (permalink / raw) To: Mark Wielaard, elfutils-devel > I had to hand apply a few things because of whitespace adjustments. > Hopefully I did it right and this is how Ulf intended the patches. > If not, my apologies, and please let me know what changes you did > intend. Thank you. The patches are correct. cheers, Ulf ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: frame unwinding patches 2017-02-15 23:34 frame unwinding patches Mark Wielaard ` (3 preceding siblings ...) 2017-02-16 9:13 ` frame unwinding patches Ulf Hermann @ 2017-04-03 9:00 ` Milian Wolff 2017-04-03 9:03 ` Ulf Hermann 2017-04-03 21:23 ` Jan Kratochvil 4 siblings, 2 replies; 37+ messages in thread From: Milian Wolff @ 2017-04-03 9:00 UTC (permalink / raw) To: elfutils-devel; +Cc: Mark Wielaard, Ulf Hermann On Thursday, February 16, 2017 12:33:27 AM CEST Mark Wielaard wrote: > Hi, > > I put all three frame pointer unwinding fallback patches on > the mjw/fp-unwind branch. I'll also sent them to the list using > git send-mail --annotate taking out the binary file patches. > Hopefully that will make them appear on the list, bypassing the > spam filters. > > [PATCH 1/3] Add frame pointer unwinding as fallback on x86_64 > [PATCH 2/3] Add frame pointer unwinding as fallback on arm > [PATCH 3/3] Add frame pointer unwinding for aarch64 > > I had to hand apply a few things because of whitespace adjustments. > Hopefully I did it right and this is how Ulf intended the patches. > If not, my apologies, and please let me know what changes you did > intend. Ping? Any progress on merging this functionality upstream? It can make quite a difference in unwinding. I just got a report from a colleague. As-is, elfutils would fail to unwind from the following location in his application: 0x1137ca4 With the x86_64 patch applied, he got a proper backtrace: 0x1137ca4 0xc45466 0xc4fffa 0xda76f3 0xd78181 0xd8069c 0xd846bd 0xd995b7 QOpenGLFunctions::glTexImage2D qopenglfunctions.h:1022 ... <proper debug symbols available rom here on> So, please consider reviewing and merging this. Thanks -- Milian Wolff mail@milianw.de http://milianw.de ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: frame unwinding patches 2017-04-03 9:00 ` Milian Wolff @ 2017-04-03 9:03 ` Ulf Hermann 2017-04-03 21:14 ` Mark Wielaard 2017-04-03 21:23 ` Jan Kratochvil 1 sibling, 1 reply; 37+ messages in thread From: Ulf Hermann @ 2017-04-03 9:03 UTC (permalink / raw) To: elfutils-devel > Ping? Any progress on merging this functionality upstream? It can make quite a > difference in unwinding. The patches have also been in perfparser releases for over a year now. I would like to see them upstream. best, Ulf ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: frame unwinding patches 2017-04-03 9:03 ` Ulf Hermann @ 2017-04-03 21:14 ` Mark Wielaard 2017-04-07 10:27 ` Mark Wielaard 0 siblings, 1 reply; 37+ messages in thread From: Mark Wielaard @ 2017-04-03 21:14 UTC (permalink / raw) To: Ulf Hermann; +Cc: elfutils-devel On Mon, Apr 03, 2017 at 11:02:53AM +0200, Ulf Hermann wrote: > > Ping? Any progress on merging this functionality upstream? > > It can make quite a difference in unwinding. > > The patches have also been in perfparser releases for over a year now. I > would like to see them upstream. Yes, sorry. The patches looked fine. Then I thought I should try adding similar support to another arch to double check. And then I got distracted by something else. I'll get back to is asap and make sure they get in before the next release end of the month. Cheers, Mark ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: frame unwinding patches 2017-04-03 21:14 ` Mark Wielaard @ 2017-04-07 10:27 ` Mark Wielaard 2017-04-11 10:16 ` Ulf Hermann 0 siblings, 1 reply; 37+ messages in thread From: Mark Wielaard @ 2017-04-07 10:27 UTC (permalink / raw) To: Ulf Hermann; +Cc: elfutils-devel [-- Attachment #1: Type: text/plain, Size: 4804 bytes --] On Mon, 2017-04-03 at 23:15 +0200, Mark Wielaard wrote: > On Mon, Apr 03, 2017 at 11:02:53AM +0200, Ulf Hermann wrote: > > > Ping? Any progress on merging this functionality upstream? > > > It can make quite a difference in unwinding. > > > > The patches have also been in perfparser releases for over a year now. I > > would like to see them upstream. > > Yes, sorry. The patches looked fine. Then I thought I should try > adding similar support to another arch to double check. And then > I got distracted by something else. I'll get back to is asap and > make sure they get in before the next release end of the month. OK. Sorry for the delay. In general I like these patches. I even made a i386 fp unwind backend function to play with the idea a bit more. See attached. Does that look sane? I do agree with Jan that frame pointer unwinding is notoriously untrustworthy. Even with some sanity checks it is hard to know whether you are doing a correct unwind. gdb gets away with it through pretty advanced frame sniffers, which take a lot of low-level compiler generation knowledge to get correct (and even then...). You only restore the pc, stack and frame pointer registers (maybe incorrectly). So afterwards, even if you got valid CFI data you might be stuck. Ideally we provide the user with a flag to check how trustworthy a frame is and/or an option to drop inexact unwinding completely. But that would be an extension for another time. I do think that in the context of dwfl_thread_getframes () it is appropriate. Currently all you can get from a Dwfl_Frame is the pc anyway. I do however have a problem with the testcases. I approved the --allow-unknown option for the backtrace fp core tests (commit f9971cb) but I now believe that was a mistake. It makes it really hard to see whether the test actually tests anything. In fact for my i386 case the testcase even succeeded when disabling the whole frame pointer unwinder fallback... I believe the issue with the missing symbol names is not caused by the frame pointer unwinding not matching real function addresses, because they should! But because of the way the testcase binaries are generated. What you do is the following: # gcc -static -O2 -fno-omit-frame-pointer -I ../ -I ../lib/ -D_GNU_SOURCE # -pthread -o backtrace.x86_64.fp.exec backtrace-child.c # # Then strip the .eh_frame and .eh_frame_hdr sections from the binary: # # strip -R .eh_frame backtrace.x86_64.fp.exec # strip -R .eh_frame_hdr backtrace.x86_64.fp.exec # # The core is generated by calling the binary with --gencore But the .eh_frame sections are allocated sections. Which means that by removing them strip will alter the address layout of the program. Which causes the symbol table addresses to not match up anymore. I think they should be generated with -fno-asynchronous-unwind-tables to avoid them being generated in the first place (in the main binary functions, but keep those from the static library functions pulled in): # gcc -static -O2 -fno-omit-frame-pointer -fno-asynchronous-unwind-tables \ # -D_GNU_SOURCE -pthread -o tests/backtrace.i386.fp.exec -I. -Ilib \ # tests/backtrace-child.c That way the difference between having a frame pointer unwinder or not is much clearer: $ eu-stack --exec ./backtrace.i386.fp.exec --core ./backtrace.i386.fp.core PID 12044 - core TID 12045: #0 0x008a7416 __kernel_vsyscall #1 0x08051ab9 raise #2 0x080485c1 sigusr2 eu-stack: dwfl_thread_getframes tid 12045 at 0x80485c0 in [exe]: No DWARF information found TID 12044: #0 0x008a7416 __kernel_vsyscall #1 0x0804ae30 pthread_join #2 0x0804847c main eu-stack: dwfl_thread_getframes tid 12044 at 0x804847b in [exe]: No DWARF information found $ LD_LIBRARY_PATH=backends:libelf:libdw src/stack --exec ./backtrace.i386.fp.exec --core ./backtrace.i386.fp.core PID 12044 - core TID 12045: #0 0x008a7416 __kernel_vsyscall #1 0x08051ab9 raise #2 0x080485c1 sigusr2 #3 0x08048699 stdarg #4 0x08048702 backtracegen #5 0x0804871b start #6 0x0804a7cf start_thread #7 0x080746fe __clone TID 12044: #0 0x008a7416 __kernel_vsyscall #1 0x0804ae30 pthread_join #2 0x0804847c main #3 0x08053188 __libc_start_main #4 0x080481e1 _start src/stack: dwfl_thread_getframes tid 12044 at 0x80481e0 in [exe]: No DWARF information found Then if we check for one of the function names in the middle of the frame pointer only unwound frames, like backtracegen, this would give us a much better test of whether things worked correctly. Could you try regenerating your test binaries with -fno-asynchronous-unwind-tables? Then I'll try to adapt the testsuite to check for the one of the frame pointer only unwound function names. Thanks, Mark [-- Attachment #2: i386_unwind.c --] [-- Type: text/x-csrc, Size: 2560 bytes --] /* Get previous frame state for an existing frame state using frame pointers. Copyright (C) 2017 Red Hat, Inc. This file is part of elfutils. This file is free software; you can redistribute it and/or modify it under the terms of either * the GNU Lesser General Public License as published by the Free Software Foundation; either version 3 of the License, or (at your option) any later version or * the GNU General Public License as published by the Free Software Foundation; either version 2 of the License, or (at your option) any later version or both in parallel, as here. elfutils is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more details. You should have received copies of the GNU General Public License and the GNU Lesser General Public License along with this program. If not, see <http://www.gnu.org/licenses/>. */ #ifdef HAVE_CONFIG_H # include <config.h> #endif #include <stdlib.h> #include <assert.h> #define BACKEND i386_ #include "libebl_CPU.h" /* Register numbers for frame and stack pointers. We take advantage of them being next to each other when calling getfunc and setfunc. */ #define ESP 4 #define EBP (ESP + 1) /* Most basic frame pointer chasing with EBP as frame pointer. PC = *(FP + 4), SP = FP + 8, FP = *FP. */ bool i386_unwind (Ebl *ebl __attribute__ ((unused)), Dwarf_Addr pc __attribute__ ((unused)), ebl_tid_registers_t *setfunc, ebl_tid_registers_get_t *getfunc, ebl_pid_memory_read_t *readfunc, void *arg, bool *signal_framep __attribute__ ((unused))) { /* sp = 0, fp = 1 */ Dwarf_Word regs[2]; /* Get current stack and frame pointers. */ if (! getfunc (ESP, 2, regs, arg)) return false; Dwarf_Word sp = regs[0]; Dwarf_Word fp = regs[1]; /* Sanity check. We only support traditional stack frames. */ if (fp == 0 || sp == 0 || fp < sp) return false; /* Get the return address from the stack, it is our new pc. */ Dwarf_Word ret_addr; if (! readfunc (fp + 4, &ret_addr, arg) || ret_addr == 0) return false; /* Get new sp and fp. */ sp = fp + 8; if (! readfunc (fp, &fp, arg) || fp == 0 || sp >= fp) return false; /* Set new sp, fp and pc. */ regs[0] = sp; regs[1] = fp; if (! setfunc (ESP, 2, regs, arg) || ! setfunc (-1, 1, &ret_addr, arg)) return false; return true; } ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: frame unwinding patches 2017-04-07 10:27 ` Mark Wielaard @ 2017-04-11 10:16 ` Ulf Hermann 2017-04-19 19:48 ` Mark Wielaard 0 siblings, 1 reply; 37+ messages in thread From: Ulf Hermann @ 2017-04-11 10:16 UTC (permalink / raw) Cc: elfutils-devel > I do agree with Jan that frame pointer unwinding is notoriously > untrustworthy. Even with some sanity checks it is hard to know whether > you are doing a correct unwind. gdb gets away with it through pretty > advanced frame sniffers, which take a lot of low-level compiler > generation knowledge to get correct (and even then...). You only restore > the pc, stack and frame pointer registers (maybe incorrectly). So > afterwards, even if you got valid CFI data you might be stuck. Yes, especially with mixed stack traces, where part of the stack has CFI and part of it doesn't, we quickly run into guesswork. I've regenerated the binaries as suggested, with the result being that raise() from libc actually has CFI, but doesn't set a frame pointer. So, the frame pointer unwinder can find raise() in the link register, but it sets up the FP register with the wrong value. Then raise() is unwound using CFI, which mixes up the registers some more. At that point we're lost. I don't see an easy way out of this. I will keep a version of the frame unwinding for perfparser as it's still better than not unwinding at all, but I do understand that it's not really suitable for mainline elfutils. br, Ulf ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: frame unwinding patches 2017-04-11 10:16 ` Ulf Hermann @ 2017-04-19 19:48 ` Mark Wielaard 2017-04-20 9:26 ` Ulf Hermann 0 siblings, 1 reply; 37+ messages in thread From: Mark Wielaard @ 2017-04-19 19:48 UTC (permalink / raw) To: Ulf Hermann; +Cc: elfutils-devel On Tue, 2017-04-11 at 12:16 +0200, Ulf Hermann wrote: > > I do agree with Jan that frame pointer unwinding is notoriously > > untrustworthy. Even with some sanity checks it is hard to know whether > > you are doing a correct unwind. gdb gets away with it through pretty > > advanced frame sniffers, which take a lot of low-level compiler > > generation knowledge to get correct (and even then...). You only restore > > the pc, stack and frame pointer registers (maybe incorrectly). So > > afterwards, even if you got valid CFI data you might be stuck. > > Yes, especially with mixed stack traces, where part of the stack has > CFI and part of it doesn't, we quickly run into guesswork. I've > regenerated the binaries as suggested, with the result being that > raise() from libc actually has CFI, but doesn't set a frame pointer. > So, the frame pointer unwinder can find raise() in the link register, > but it sets up the FP register with the wrong value. Then raise() is > unwound using CFI, which mixes up the registers some more. At that > point we're lost. I don't see an easy way out of this. That might just mean that the testcase is slightly unrealistic. Getting a reliable backtrace through signal handlers when not having full CFI is probably not something we can expect to work. That doesn't mean having a frame pointer based fallback is a bad thing. We probably should find a more realistic testcase. And maybe in the future add an interface to allow people to unwind through "pure CFI" or mixed mode with frame markers that tell the caller whether the registers can be trusted or not. > I will keep a version of the frame unwinding for perfparser as it's > still better than not unwinding at all, but I do understand that it's > not really suitable for mainline elfutils. Really I do think it would be nice to have. I certainly didn't mean it isn't suitable for mainline. We just should be realistic about the expectations. IMHO if at all possible we should get this upstream so you don't have to carry extra patches. Cheers, Mark ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: frame unwinding patches 2017-04-19 19:48 ` Mark Wielaard @ 2017-04-20 9:26 ` Ulf Hermann 2017-04-25 12:50 ` Mark Wielaard 0 siblings, 1 reply; 37+ messages in thread From: Ulf Hermann @ 2017-04-20 9:26 UTC (permalink / raw) To: Mark Wielaard; +Cc: elfutils-devel > That might just mean that the testcase is slightly unrealistic. > Getting a reliable backtrace through signal handlers when not having > full CFI is probably not something we can expect to work. That doesn't > mean having a frame pointer based fallback is a bad thing. We probably > should find a more realistic testcase. And maybe in the future add an > interface to allow people to unwind through "pure CFI" or mixed mode > with frame markers that tell the caller whether the registers can be > trusted or not. The x86_64 case already works with the test case I sent. Maybe we can accept that one before the others. The aarch64 case almost works, but seems to generally duplicate the first entry it unwinds by frame pointer after unwinding anything by CFI. That should be fixable. I will research it and post a follow up patch. The 32bit arm case is a horrible mess and we may indeed need to lower our expectations for that one. Or maybe I can find a raise() that follows the same frame conventions as the gcc I'm using ... br, Ulf ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: frame unwinding patches 2017-04-20 9:26 ` Ulf Hermann @ 2017-04-25 12:50 ` Mark Wielaard 2017-04-25 12:54 ` [PATCH 1/5] Revert "Optionally allow unknown symbols in the backtrace tests" Mark Wielaard 2017-04-26 15:20 ` frame unwinding patches Ulf Hermann 0 siblings, 2 replies; 37+ messages in thread From: Mark Wielaard @ 2017-04-25 12:50 UTC (permalink / raw) To: Ulf Hermann; +Cc: elfutils-devel On Thu, 2017-04-20 at 11:26 +0200, Ulf Hermann wrote: > The x86_64 case already works with the test case I sent. Maybe we can > accept that one before the others. The aarch64 case almost works, but > seems to generally duplicate the first entry it unwinds by frame > pointer after unwinding anything by CFI. That should be fixable. I > will research it and post a follow up patch. The 32bit arm case is a > horrible mess and we may indeed need to lower our expectations for > that one. Or maybe I can find a raise() that follows the same frame > conventions as the gcc I'm using ... I tweaked the testcases a little to test more things. I reverted the "Optionally allow unknown symbols in the backtrace tests" and added an explicit check for a frame in the middle, the backtracegen function. That showed the ppc32 core testfiles were generated without CFI for the main executable, so I regenerated those. Then I added your x86_64 frame pointer unwinder, testcases, my i386 frame pointer unwinder and your aarch64 frame pointer unwinder. I dropped the arm32 frame pointer unwinder for now (maybe we need a less demanding testcase for that or, more awesome, add code to translate the exidx section for that). I do have a question about the aarch64 frame pointer unwinder (and the initial frame), but I'll reply to the patch email for that. I'll post the patches as reply to this message (stripping the binaries) and have pushed them all to the (rebased) mjw/fpunwind branch: https://sourceware.org/git/?p=elfutils.git;a=shortlog;h=refs/heads/mjw/fp-unwind Could you take a look and see if that looks good? Thanks, Mark ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 1/5] Revert "Optionally allow unknown symbols in the backtrace tests" 2017-04-25 12:50 ` Mark Wielaard @ 2017-04-25 12:54 ` Mark Wielaard 2017-04-25 12:50 ` [PATCH 2/5] tests: Add core backtracegen chec and regenerate ppc32 backtrace test files Mark Wielaard ` (4 more replies) 2017-04-26 15:20 ` frame unwinding patches Ulf Hermann 1 sibling, 5 replies; 37+ messages in thread From: Mark Wielaard @ 2017-04-25 12:54 UTC (permalink / raw) To: elfutils-devel; +Cc: Ulf Hermann, Mark Wielaard This reverts commit f9971cb422df39adea7e8c7e22689b879e39c626. Allowing no symbol resolving at all makes it too hard to see whether the test actually tests anything. But do keep "address out of range" as allowed error in check_err. This can be interpreted as DWARF not available (if end of callstack marker is missing, which it unfortunately often is missing even if CFI is available.). Signed-off-by: Mark Wielaard <mark@klomp.org> --- tests/ChangeLog | 6 ++++++ tests/backtrace-subr.sh | 12 +++--------- tests/backtrace.c | 30 ++++++++---------------------- 3 files changed, 17 insertions(+), 31 deletions(-) diff --git a/tests/ChangeLog b/tests/ChangeLog index 5f7bcdd..a71db45 100644 --- a/tests/ChangeLog +++ b/tests/ChangeLog @@ -1,3 +1,9 @@ +2017-04-24 Mark Wielaard <mark@klomp.org> + + * backtrace.c: Remove option to allow unknown symbols in the trace. + * backtrace-substr.sh: Remove option to allow unknown symbols + to check_core() and allow failed symbol lookups in check_err(). + 2017-04-05 Mark Wielaard <mark@klomp.org> * test-subr.sh (testrun_on_self_compressed): New function. diff --git a/tests/backtrace-subr.sh b/tests/backtrace-subr.sh index 5d3937c..e746dc1 100644 --- a/tests/backtrace-subr.sh +++ b/tests/backtrace-subr.sh @@ -44,9 +44,6 @@ check_gsignal() # In some cases we cannot reliably find out we got behind _start as some # operating system do not properly terminate CFI by undefined PC. # Ignore it here as it is a bug of OS, not a bug of elfutils. -# If the CFI is not terminated correctly, we might find another frame by -# checking for frame pointers. This is still not our problem, but only -# gives an error message when trying to look up the function name. check_err() { if [ $(egrep -v <$1 'dwfl_thread_getframes: (No DWARF information found|no matching address range|address out of range)$' \ @@ -64,9 +61,7 @@ check_all() bt=$1 err=$2 testname=$3 - if [ "x$4" != "x--allow-unknown" ]; then - check_main $bt $testname - fi + check_main $bt $testname check_gsignal $bt $testname check_err $err $testname } @@ -103,14 +98,13 @@ check_native_unsupported() check_core() { arch=$1 - args=$2 testfiles backtrace.$arch.{exec,core} tempfiles backtrace.$arch.{bt,err} echo ./backtrace ./backtrace.$arch.{exec,core} - testrun ${abs_builddir}/backtrace $args -e ./backtrace.$arch.exec --core=./backtrace.$arch.core 1>backtrace.$arch.bt 2>backtrace.$arch.err || true + testrun ${abs_builddir}/backtrace -e ./backtrace.$arch.exec --core=./backtrace.$arch.core 1>backtrace.$arch.bt 2>backtrace.$arch.err || true cat backtrace.$arch.{bt,err} check_unsupported backtrace.$arch.err backtrace.$arch.core - check_all backtrace.$arch.{bt,err} backtrace.$arch.core $args + check_all backtrace.$arch.{bt,err} backtrace.$arch.core } # Backtrace live process. diff --git a/tests/backtrace.c b/tests/backtrace.c index 34a2ab0..1ff6353 100644 --- a/tests/backtrace.c +++ b/tests/backtrace.c @@ -64,7 +64,6 @@ dump_modules (Dwfl_Module *mod, void **userdata __attribute__ ((unused)), return DWARF_CB_OK; } -static bool allow_unknown; static bool use_raise_jmp_patching; static pid_t check_tid; @@ -79,8 +78,7 @@ callback_verify (pid_t tid, unsigned frameno, Dwarf_Addr pc, seen_main = true; if (pc == 0) { - if (!allow_unknown) - assert (seen_main); + assert (seen_main); return; } if (check_tid == 0) @@ -105,12 +103,11 @@ callback_verify (pid_t tid, unsigned frameno, Dwarf_Addr pc, && (strcmp (symname, "__kernel_vsyscall") == 0 || strcmp (symname, "__libc_do_syscall") == 0)) reduce_frameno = true; - else if (!allow_unknown || symname) + else assert (symname && strcmp (symname, "raise") == 0); break; case 1: - if (!allow_unknown || symname) - assert (symname != NULL && strcmp (symname, "sigusr2") == 0); + assert (symname != NULL && strcmp (symname, "sigusr2") == 0); break; case 2: // x86_64 only /* __restore_rt - glibc maybe does not have to have this symbol. */ @@ -119,24 +116,20 @@ callback_verify (pid_t tid, unsigned frameno, Dwarf_Addr pc, if (use_raise_jmp_patching) { /* Verify we trapped on the very first instruction of jmp. */ - if (!allow_unknown || symname) - assert (symname != NULL && strcmp (symname, "jmp") == 0); + assert (symname != NULL && strcmp (symname, "jmp") == 0); mod = dwfl_addrmodule (dwfl, pc - 1); if (mod) symname2 = dwfl_module_addrname (mod, pc - 1); - if (!allow_unknown || symname2) - assert (symname2 == NULL || strcmp (symname2, "jmp") != 0); + assert (symname2 == NULL || strcmp (symname2, "jmp") != 0); break; } /* FALLTHRU */ case 4: - if (!allow_unknown || symname) - assert (symname != NULL && strcmp (symname, "stdarg") == 0); + assert (symname != NULL && strcmp (symname, "stdarg") == 0); break; case 5: /* Verify we trapped on the very last instruction of child. */ - if (!allow_unknown || symname) - assert (symname != NULL && strcmp (symname, "backtracegen") == 0); + assert (symname != NULL && strcmp (symname, "backtracegen") == 0); mod = dwfl_addrmodule (dwfl, pc); if (mod) symname2 = dwfl_module_addrname (mod, pc); @@ -145,7 +138,7 @@ callback_verify (pid_t tid, unsigned frameno, Dwarf_Addr pc, // there is no guarantee that the compiler doesn't reorder the // instructions or even inserts some padding instructions at the end // (which apparently happens on ppc64). - if (use_raise_jmp_patching && (!allow_unknown || symname2)) + if (use_raise_jmp_patching) assert (symname2 == NULL || strcmp (symname2, "backtracegen") != 0); break; } @@ -431,12 +424,10 @@ exec_dump (const char *exec) } #define OPT_BACKTRACE_EXEC 0x100 -#define OPT_ALLOW_UNKNOWN 0x200 static const struct argp_option options[] = { { "backtrace-exec", OPT_BACKTRACE_EXEC, "EXEC", 0, N_("Run executable"), 0 }, - { "allow-unknown", OPT_ALLOW_UNKNOWN, 0, 0, N_("Allow unknown symbols"), 0 }, { NULL, 0, NULL, 0, NULL, 0 } }; @@ -454,10 +445,6 @@ parse_opt (int key, char *arg, struct argp_state *state) exec_dump (arg); exit (0); - case OPT_ALLOW_UNKNOWN: - allow_unknown = true; - break; - default: return ARGP_ERR_UNKNOWN; } @@ -476,7 +463,6 @@ main (int argc __attribute__ ((unused)), char **argv) (void) setlocale (LC_ALL, ""); elf_version (EV_CURRENT); - allow_unknown = false; Dwfl *dwfl = NULL; const struct argp_child argp_children[] = -- 1.8.3.1 ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 2/5] tests: Add core backtracegen chec and regenerate ppc32 backtrace test files. 2017-04-25 12:54 ` [PATCH 1/5] Revert "Optionally allow unknown symbols in the backtrace tests" Mark Wielaard @ 2017-04-25 12:50 ` Mark Wielaard 2017-04-25 13:04 ` Ulf Hermann 2017-04-25 12:55 ` [PATCH 3/5] Add frame pointer unwinding as fallback on x86_64 Mark Wielaard ` (3 subsequent siblings) 4 siblings, 1 reply; 37+ messages in thread From: Mark Wielaard @ 2017-04-25 12:50 UTC (permalink / raw) To: elfutils-devel; +Cc: Ulf Hermann, Mark Wielaard Add a check to check_core to make sure the backtracegen function is found in the backtrace. This function is in the middle of the backtrace in the main executable and if not found it means the backtrace was incomplete or the frame was skipped (which could happen on a bad frame pointer only unwind). This showed that the ppc32 backtrace test files were missing DWARF CFI for the main executable. Regenerated them to include full CFI. Signed-off-by: Mark Wielaard <mark@klomp.org> --- tests/ChangeLog | 7 +++++++ tests/backtrace-subr.sh | 14 ++++++++++++++ tests/backtrace.ppc.core.bz2 | Bin 46357 -> 44482 bytes tests/backtrace.ppc.exec.bz2 | Bin 352898 -> 352197 bytes tests/run-backtrace-core-ppc.sh | 9 +++++++++ 5 files changed, 30 insertions(+) diff --git a/tests/ChangeLog b/tests/ChangeLog index a71db45..6f5956b 100644 --- a/tests/ChangeLog +++ b/tests/ChangeLog @@ -1,3 +1,10 @@ +2017-04-25 Mark Wielaard <mark@klomp.org> + + * backtrace-subr.sh (check_backtracegen): New function. + (check_core): Add check_backtracegen call. + * backtrace.ppc.exec.bz2: Regenerated. + * backtrace.ppc.core.bz2: Likewise. + 2017-04-24 Mark Wielaard <mark@klomp.org> * backtrace.c: Remove option to allow unknown symbols in the trace. diff --git a/tests/backtrace-subr.sh b/tests/backtrace-subr.sh index e746dc1..a303e32 100644 --- a/tests/backtrace-subr.sh +++ b/tests/backtrace-subr.sh @@ -40,6 +40,19 @@ check_gsignal() false } + +# Makes sure we saw the function that initiated the backtrace +# when the core was generated through the tests backtrace --gencore. +# This might disappear when frame pointer chasing gone bad. +check_backtracegen() +{ + if grep -w backtracegen $1; then + return + fi + echo >&2 $2: no backtracegen + false +} + # Verify the STDERR output does not contain unexpected errors. # In some cases we cannot reliably find out we got behind _start as some # operating system do not properly terminate CFI by undefined PC. @@ -105,6 +118,7 @@ check_core() cat backtrace.$arch.{bt,err} check_unsupported backtrace.$arch.err backtrace.$arch.core check_all backtrace.$arch.{bt,err} backtrace.$arch.core + check_backtracegen backtrace.$arch.bt backtrace.$arch.core } # Backtrace live process. diff --git a/tests/run-backtrace-core-ppc.sh b/tests/run-backtrace-core-ppc.sh index 65c9279..555ac35 100755 --- a/tests/run-backtrace-core-ppc.sh +++ b/tests/run-backtrace-core-ppc.sh @@ -17,4 +17,13 @@ . $srcdir/backtrace-subr.sh +# executable generated by: +# +# gcc -D_GNU_SOURCE -I. -I.. -I../lib -m32 -pthread -static -g \ +# -o backtrace.ppc.exec backtrace-child.c +# +# core generated by: +# +# ./backtrace.ppc.exec --gencore + check_core ppc -- 1.8.3.1 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 2/5] tests: Add core backtracegen chec and regenerate ppc32 backtrace test files. 2017-04-25 12:50 ` [PATCH 2/5] tests: Add core backtracegen chec and regenerate ppc32 backtrace test files Mark Wielaard @ 2017-04-25 13:04 ` Ulf Hermann 0 siblings, 0 replies; 37+ messages in thread From: Ulf Hermann @ 2017-04-25 13:04 UTC (permalink / raw) To: Mark Wielaard, elfutils-devel On 04/25/2017 02:49 PM, Mark Wielaard wrote: > Add a check to check_core to make sure the backtracegen function is > found in the backtrace. This function is in the middle of the backtrace > in the main executable and if not found it means the backtrace was > incomplete or the frame was skipped (which could happen on a bad frame > pointer only unwind). > > This showed that the ppc32 backtrace test files were missing DWARF CFI > for the main executable. Regenerated them to include full CFI. Looks good to me. Ulf ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 3/5] Add frame pointer unwinding as fallback on x86_64 2017-04-25 12:54 ` [PATCH 1/5] Revert "Optionally allow unknown symbols in the backtrace tests" Mark Wielaard 2017-04-25 12:50 ` [PATCH 2/5] tests: Add core backtracegen chec and regenerate ppc32 backtrace test files Mark Wielaard @ 2017-04-25 12:55 ` Mark Wielaard 2017-04-25 13:05 ` Ulf Hermann 2017-04-25 12:56 ` [PATCH 4/5] Add i386 frame pointer unwinder Mark Wielaard ` (2 subsequent siblings) 4 siblings, 1 reply; 37+ messages in thread From: Mark Wielaard @ 2017-04-25 12:55 UTC (permalink / raw) To: elfutils-devel; +Cc: Ulf Hermann From: Ulf Hermann <ulf.hermann@qt.io> If we don't find any debug information for a given frame, we usually cannot unwind any further. However, the binary in question might have been compiled with frame pointers, in which case we can look up the well known frame pointer locations in the stack snapshot and use them to bridge the frames without debug information. The "unwind" hook is the right place for this as it is so far only used on s390 and called only after trying to unwind with debug information. Signed-off-by: Ulf Hermann <ulf.hermann@qt.io> --- backends/ChangeLog | 6 +++ backends/Makefile.am | 2 +- backends/x86_64_init.c | 1 + backends/x86_64_unwind.c | 86 ++++++++++++++++++++++++++++++++++ tests/ChangeLog | 7 +++ tests/Makefile.am | 3 ++ tests/backtrace.x86_64.fp.core.bz2 | Bin 0 -> 11072 bytes tests/backtrace.x86_64.fp.exec.bz2 | Bin 0 -> 434645 bytes tests/run-backtrace-fp-core-x86_64.sh | 29 ++++++++++++ 9 files changed, 133 insertions(+), 1 deletion(-) create mode 100644 backends/x86_64_unwind.c create mode 100644 tests/backtrace.x86_64.fp.core.bz2 create mode 100644 tests/backtrace.x86_64.fp.exec.bz2 create mode 100755 tests/run-backtrace-fp-core-x86_64.sh diff --git a/backends/ChangeLog b/backends/ChangeLog index 39390cb..eec9923 100644 --- a/backends/ChangeLog +++ b/backends/ChangeLog @@ -1,3 +1,9 @@ +2017-02-09 Ulf Hermann <ulf.hermann@qt.io> + + * x86_64_unwind.c: New file + * Makefile.am (x86_64_SRCS): Add x86_64_unwind.c + * x86_64_init.c (x86_64_init): Hook x86_64_unwind + 2017-02-15 Mark Wielaard <mark@klomp.org> * ppc64_init.c (ppc64_init): Add check_object_attribute HOOK. diff --git a/backends/Makefile.am b/backends/Makefile.am index b553ec3..60917b9 100644 --- a/backends/Makefile.am +++ b/backends/Makefile.am @@ -59,7 +59,7 @@ am_libebl_sh_pic_a_OBJECTS = $(sh_SRCS:.c=.os) x86_64_SRCS = x86_64_init.c x86_64_symbol.c x86_64_corenote.c x86_64_cfi.c \ x86_64_retval.c x86_64_regs.c i386_auxv.c x86_64_syscall.c \ - x86_64_initreg.c x32_corenote.c + x86_64_initreg.c x86_64_unwind.c x32_corenote.c cpu_x86_64 = ../libcpu/libcpu_x86_64.a libebl_x86_64_pic_a_SOURCES = $(x86_64_SRCS) am_libebl_x86_64_pic_a_OBJECTS = $(x86_64_SRCS:.c=.os) diff --git a/backends/x86_64_init.c b/backends/x86_64_init.c index cfd0158..adfa479 100644 --- a/backends/x86_64_init.c +++ b/backends/x86_64_init.c @@ -68,6 +68,7 @@ x86_64_init (Elf *elf __attribute__ ((unused)), /* gcc/config/ #define DWARF_FRAME_REGISTERS. */ eh->frame_nregs = 17; HOOK (eh, set_initial_registers_tid); + HOOK (eh, unwind); return MODVERSION; } diff --git a/backends/x86_64_unwind.c b/backends/x86_64_unwind.c new file mode 100644 index 0000000..ade64c0 --- /dev/null +++ b/backends/x86_64_unwind.c @@ -0,0 +1,86 @@ +/* Get previous frame state for an existing frame state. + Copyright (C) 2016 The Qt Company Ltd. + This file is part of elfutils. + + This file is free software; you can redistribute it and/or modify + it under the terms of either + + * the GNU Lesser General Public License as published by the Free + Software Foundation; either version 3 of the License, or (at + your option) any later version + + or + + * the GNU General Public License as published by the Free + Software Foundation; either version 2 of the License, or (at + your option) any later version + + or both in parallel, as here. + + elfutils is distributed in the hope that it will be useful, but + WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + General Public License for more details. + + You should have received copies of the GNU General Public License and + the GNU Lesser General Public License along with this program. If + not, see <http://www.gnu.org/licenses/>. */ + +#ifdef HAVE_CONFIG_H +# include <config.h> +#endif + +#include <stdlib.h> +#include <assert.h> + +#define BACKEND x86_64_ +#include "libebl_CPU.h" + +/* There was no CFI. Maybe we happen to have a frame pointer and can unwind from that? */ + +bool +x86_64_unwind (Ebl *ebl __attribute__ ((unused)), + Dwarf_Addr pc __attribute__ ((unused)), + ebl_tid_registers_t *setfunc, ebl_tid_registers_get_t *getfunc, + ebl_pid_memory_read_t *readfunc, void *arg, + bool *signal_framep __attribute__ ((unused))) +{ + // Register 6 is supposed to be rbp, thus the conventional frame pointer + const int fpReg = 6; + const int spReg = 7; + + Dwarf_Word fp; + if (!getfunc(fpReg, 1, &fp, arg) || fp == 0) + return false; + + // Try to read old sp, so that we can avoid infinite loops below + Dwarf_Word sp; + if (!getfunc(spReg, 1, &sp, arg)) + sp = 0; + + Dwarf_Word prev_fp; + if (!readfunc(fp, &prev_fp, arg)) + prev_fp = 0; + + Dwarf_Word ret; + if (!readfunc(fp + 8, &ret, arg)) + return false; + + if (!setfunc(fpReg, 1, &prev_fp, arg)) + return false; + + fp += 16; // Pop fp and return address and write result to sp + if (!setfunc(spReg, 1, &fp, arg)) + return false; + + if (!setfunc(-1, 1, &ret, arg)) + return false; + + // If the sp didn't move up we don't actually have a new stack + // frame but rather some random data that doesn't include frame + // pointers. Break the unwinding then. + if (sp >= fp) + return false; + + return true; +} diff --git a/tests/ChangeLog b/tests/ChangeLog index 6f5956b..49b6a3f 100644 --- a/tests/ChangeLog +++ b/tests/ChangeLog @@ -1,3 +1,10 @@ +2017-02-09 Ulf Hermann <ulf.hermann@qt.io> + + * Makefile.am: Add test for unwinding with frame pointers on x86_64 + * backtrace.x86_64.fp.core.bz2: New file + * backtrace.x86_64.fp.exec.bz2: New file + * run-backtrace-fp-core-x86_64.sh: New file + 2017-04-25 Mark Wielaard <mark@klomp.org> * backtrace-subr.sh (check_backtracegen): New function. diff --git a/tests/Makefile.am b/tests/Makefile.am index f6d8b0d..b0db19f 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -115,6 +115,7 @@ TESTS = run-arextract.sh run-arsymtest.sh newfile test-nlist \ run-backtrace-native.sh run-backtrace-data.sh run-backtrace-dwarf.sh \ run-backtrace-native-biarch.sh run-backtrace-native-core.sh \ run-backtrace-native-core-biarch.sh run-backtrace-core-x86_64.sh \ + run-backtrace-fp-core-x86_64.sh \ run-backtrace-core-x32.sh \ run-backtrace-core-i386.sh run-backtrace-core-ppc.sh \ run-backtrace-core-s390x.sh run-backtrace-core-s390.sh \ @@ -293,9 +294,11 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh \ run-backtrace-native.sh run-backtrace-native-biarch.sh \ run-backtrace-native-core.sh run-backtrace-native-core-biarch.sh \ run-backtrace-core-x86_64.sh run-backtrace-core-i386.sh \ + run-backtrace-fp-core-x86_64.sh \ run-backtrace-core-x32.sh \ backtrace-subr.sh backtrace.i386.core.bz2 backtrace.i386.exec.bz2 \ backtrace.x86_64.core.bz2 backtrace.x86_64.exec.bz2 \ + backtrace.x86_64.fp.core.bz2 backtrace.x86_64.fp.exec.bz2 \ backtrace.ppc.core.bz2 backtrace.ppc.exec.bz2 \ run-backtrace-core-ppc.sh testfile66.bz2 testfile66.core.bz2 \ backtrace.s390x.core.bz2 backtrace.s390x.exec.bz2 \ diff --git a/tests/run-backtrace-fp-core-x86_64.sh b/tests/run-backtrace-fp-core-x86_64.sh new file mode 100755 index 0000000..348eb18 --- /dev/null +++ b/tests/run-backtrace-fp-core-x86_64.sh @@ -0,0 +1,29 @@ +#! /bin/bash +# Copyright (C) 2017 The Qt Company +# This file is part of elfutils. +# +# This file is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# elfutils is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +. $srcdir/backtrace-subr.sh + +# The binary is generated by compiling with eh_frame CFI, but with frame +# pointers. +# +# gcc -static -O2 -fno-omit-frame-pointer -fno-asynchronous-unwind-tables \ +# -D_GNU_SOURCE -pthread -o tests/backtrace.x86_64.fp.exec -I. -Ilib \ +# tests/backtrace-child.c +# +# The core is generated by calling the binary with --gencore + +check_core x86_64.fp -- 1.8.3.1 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 3/5] Add frame pointer unwinding as fallback on x86_64 2017-04-25 12:55 ` [PATCH 3/5] Add frame pointer unwinding as fallback on x86_64 Mark Wielaard @ 2017-04-25 13:05 ` Ulf Hermann 0 siblings, 0 replies; 37+ messages in thread From: Ulf Hermann @ 2017-04-25 13:05 UTC (permalink / raw) To: Mark Wielaard, elfutils-devel On 04/25/2017 02:49 PM, Mark Wielaard wrote: > From: Ulf Hermann <ulf.hermann@qt.io> > > If we don't find any debug information for a given frame, we usually > cannot unwind any further. However, the binary in question might have > been compiled with frame pointers, in which case we can look up the > well known frame pointer locations in the stack snapshot and use them > to bridge the frames without debug information. > > The "unwind" hook is the right place for this as it is so far only > used on s390 and called only after trying to unwind with debug > information. Looks good to me. Ulf ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 4/5] Add i386 frame pointer unwinder. 2017-04-25 12:54 ` [PATCH 1/5] Revert "Optionally allow unknown symbols in the backtrace tests" Mark Wielaard 2017-04-25 12:50 ` [PATCH 2/5] tests: Add core backtracegen chec and regenerate ppc32 backtrace test files Mark Wielaard 2017-04-25 12:55 ` [PATCH 3/5] Add frame pointer unwinding as fallback on x86_64 Mark Wielaard @ 2017-04-25 12:56 ` Mark Wielaard 2017-04-25 13:38 ` Ulf Hermann 2017-04-25 12:56 ` [PATCH 1/5] Revert "Optionally allow unknown symbols in the backtrace tests" Ulf Hermann 2017-04-25 13:11 ` [PATCH 5/5] Add frame pointer unwinding for aarch64 Mark Wielaard 4 siblings, 1 reply; 37+ messages in thread From: Mark Wielaard @ 2017-04-25 12:56 UTC (permalink / raw) To: elfutils-devel; +Cc: Ulf Hermann, Mark Wielaard Add a simple i386_unwind.c frame pointer unwinder as fallback if DWARF/CFI unwinding fails. Signed-off-by: Mark Wielaard <mark@klomp.org> --- backends/ChangeLog | 6 +++ backends/Makefile.am | 2 +- backends/i386_init.c | 3 +- backends/i386_unwind.c | 84 ++++++++++++++++++++++++++++++++++++ tests/ChangeLog | 9 ++++ tests/Makefile.am | 5 ++- tests/backtrace.i386.fp.core.bz2 | Bin 0 -> 8532 bytes tests/backtrace.i386.fp.exec.bz2 | Bin 0 -> 357436 bytes tests/run-backtrace-fp-core-i386.sh | 29 +++++++++++++ 9 files changed, 135 insertions(+), 3 deletions(-) create mode 100644 backends/i386_unwind.c create mode 100644 tests/backtrace.i386.fp.core.bz2 create mode 100755 tests/backtrace.i386.fp.exec.bz2 create mode 100755 tests/run-backtrace-fp-core-i386.sh diff --git a/backends/ChangeLog b/backends/ChangeLog index eec9923..733e72c 100644 --- a/backends/ChangeLog +++ b/backends/ChangeLog @@ -1,3 +1,9 @@ +2017-04-06 Mark Wielaard <mark@klomp.org> + + * i386_unwind.c: New file. + * i386_init.c: Hook i386_unwind. + * Makefile.am (i386_SRCS): Add i386_unwind.c + 2017-02-09 Ulf Hermann <ulf.hermann@qt.io> * x86_64_unwind.c: New file diff --git a/backends/Makefile.am b/backends/Makefile.am index 60917b9..22eb6ac 100644 --- a/backends/Makefile.am +++ b/backends/Makefile.am @@ -48,7 +48,7 @@ libdw = ../libdw/libdw.so i386_SRCS = i386_init.c i386_symbol.c i386_corenote.c i386_cfi.c \ i386_retval.c i386_regs.c i386_auxv.c i386_syscall.c \ - i386_initreg.c + i386_initreg.c i386_unwind.c cpu_i386 = ../libcpu/libcpu_i386.a libebl_i386_pic_a_SOURCES = $(i386_SRCS) am_libebl_i386_pic_a_OBJECTS = $(i386_SRCS:.c=.os) diff --git a/backends/i386_init.c b/backends/i386_init.c index 515d5ac..fc1587a 100644 --- a/backends/i386_init.c +++ b/backends/i386_init.c @@ -1,5 +1,5 @@ /* Initialization of i386 specific backend library. - Copyright (C) 2000-2009, 2013 Red Hat, Inc. + Copyright (C) 2000-2009, 2013, 2017 Red Hat, Inc. This file is part of elfutils. Written by Ulrich Drepper <drepper@redhat.com>, 2000. @@ -65,6 +65,7 @@ i386_init (Elf *elf __attribute__ ((unused)), /* gcc/config/ #define DWARF_FRAME_REGISTERS. For i386 it is 17, why? */ eh->frame_nregs = 9; HOOK (eh, set_initial_registers_tid); + HOOK (eh, unwind); return MODVERSION; } diff --git a/backends/i386_unwind.c b/backends/i386_unwind.c new file mode 100644 index 0000000..5c9a5de --- /dev/null +++ b/backends/i386_unwind.c @@ -0,0 +1,84 @@ +/* Get previous frame state for an existing frame state using frame pointers. + Copyright (C) 2017 Red Hat, Inc. + This file is part of elfutils. + + This file is free software; you can redistribute it and/or modify + it under the terms of either + + * the GNU Lesser General Public License as published by the Free + Software Foundation; either version 3 of the License, or (at + your option) any later version + + or + + * the GNU General Public License as published by the Free + Software Foundation; either version 2 of the License, or (at + your option) any later version + + or both in parallel, as here. + + elfutils is distributed in the hope that it will be useful, but + WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + General Public License for more details. + + You should have received copies of the GNU General Public License and + the GNU Lesser General Public License along with this program. If + not, see <http://www.gnu.org/licenses/>. */ + +#ifdef HAVE_CONFIG_H +# include <config.h> +#endif + +#include <stdlib.h> +#include <assert.h> + +#define BACKEND i386_ +#include "libebl_CPU.h" + +/* Register numbers for frame and stack pointers. We take advantage of + them being next to each other when calling getfunc and setfunc. */ +#define ESP 4 +#define EBP (ESP + 1) + +/* Most basic frame pointer chasing with EBP as frame pointer. + PC = *(FP + 4), SP = FP + 8, FP = *FP. */ +bool +i386_unwind (Ebl *ebl __attribute__ ((unused)), + Dwarf_Addr pc __attribute__ ((unused)), + ebl_tid_registers_t *setfunc, ebl_tid_registers_get_t *getfunc, + ebl_pid_memory_read_t *readfunc, void *arg, + bool *signal_framep __attribute__ ((unused))) +{ + /* sp = 0, fp = 1 */ + Dwarf_Word regs[2]; + + /* Get current stack and frame pointers. */ + if (! getfunc (ESP, 2, regs, arg)) + return false; + + Dwarf_Word sp = regs[0]; + Dwarf_Word fp = regs[1]; + + /* Sanity check. We only support traditional stack frames. */ + if (fp == 0 || sp == 0 || fp < sp) + return false; + + /* Get the return address from the stack, it is our new pc. */ + Dwarf_Word ret_addr; + if (! readfunc (fp + 4, &ret_addr, arg) || ret_addr == 0) + return false; + + /* Get new sp and fp. Sanity check again. */ + sp = fp + 8; + if (! readfunc (fp, &fp, arg) || fp == 0 || sp >= fp) + return false; + + /* Set new sp, fp and pc. */ + regs[0] = sp; + regs[1] = fp; + if (! setfunc (ESP, 2, regs, arg) || ! setfunc (-1, 1, &ret_addr, arg)) + return false; + + return true; +} diff --git a/tests/ChangeLog b/tests/ChangeLog index 49b6a3f..67a44f7 100644 --- a/tests/ChangeLog +++ b/tests/ChangeLog @@ -1,3 +1,12 @@ +2017-04-06 Mark Wielaard <mark@klomp.org> + + * run-backtrace-fp-core-i386.sh: New test. + * backtrace.i386.fp.core.bz2: New test file. + * backtrace.i386.fp.exec.bz2: New testfile. + * Makefile.am (TESTS): Add run-backtrace-fp-core-i386.sh. + (EXTRA_DIST): Add run-backtrace-fp-core-i386.sh, + backtrace.i386.fp.core.bz2 and backtrace.i386.fp.exec.bz2. + 2017-02-09 Ulf Hermann <ulf.hermann@qt.io> * Makefile.am: Add test for unwinding with frame pointers on x86_64 diff --git a/tests/Makefile.am b/tests/Makefile.am index b0db19f..2c82bfd 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -117,7 +117,8 @@ TESTS = run-arextract.sh run-arsymtest.sh newfile test-nlist \ run-backtrace-native-core-biarch.sh run-backtrace-core-x86_64.sh \ run-backtrace-fp-core-x86_64.sh \ run-backtrace-core-x32.sh \ - run-backtrace-core-i386.sh run-backtrace-core-ppc.sh \ + run-backtrace-core-i386.sh run-backtrace-fp-core-i386.sh \ + run-backtrace-core-ppc.sh \ run-backtrace-core-s390x.sh run-backtrace-core-s390.sh \ run-backtrace-core-aarch64.sh run-backtrace-core-sparc.sh \ run-backtrace-demangle.sh run-stack-d-test.sh run-stack-i-test.sh \ @@ -297,6 +298,8 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh \ run-backtrace-fp-core-x86_64.sh \ run-backtrace-core-x32.sh \ backtrace-subr.sh backtrace.i386.core.bz2 backtrace.i386.exec.bz2 \ + run-backtrace-fp-core-i386.sh \ + backtrace.i386.fp.core.bz2 backtrace.i386.fp.exec.bz2 \ backtrace.x86_64.core.bz2 backtrace.x86_64.exec.bz2 \ backtrace.x86_64.fp.core.bz2 backtrace.x86_64.fp.exec.bz2 \ backtrace.ppc.core.bz2 backtrace.ppc.exec.bz2 \ diff --git a/tests/run-backtrace-fp-core-i386.sh b/tests/run-backtrace-fp-core-i386.sh new file mode 100755 index 0000000..c58ff53 --- /dev/null +++ b/tests/run-backtrace-fp-core-i386.sh @@ -0,0 +1,29 @@ +#! /bin/bash +# Copyright (C) 2017 Red Hat, Inc. +# This file is part of elfutils. +# +# This file is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# elfutils is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +. $srcdir/backtrace-subr.sh + +# The binary is generated by compiling backtrace-child without unwind +# information, but with -fno-omit-frame-pointer. +# +# gcc -static -O2 -fno-omit-frame-pointer -fno-asynchronous-unwind-tables \ +# -D_GNU_SOURCE -pthread -o tests/backtrace.i386.fp.exec -I. -Ilib \ +# tests/backtrace-child.c +# +# The core is generated by calling tests/backtrace.i386.fp.exec --gencore + +check_core i386.fp -- 1.8.3.1 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 4/5] Add i386 frame pointer unwinder. 2017-04-25 12:56 ` [PATCH 4/5] Add i386 frame pointer unwinder Mark Wielaard @ 2017-04-25 13:38 ` Ulf Hermann 0 siblings, 0 replies; 37+ messages in thread From: Ulf Hermann @ 2017-04-25 13:38 UTC (permalink / raw) To: Mark Wielaard, elfutils-devel On 04/25/2017 02:49 PM, Mark Wielaard wrote: > Add a simple i386_unwind.c frame pointer unwinder as fallback if DWARF/CFI > unwinding fails. Looks good to me. The logic could be relaxed a bit so that failure to e.g. write the new value for sp would not be fatal. Then we might be able to unwind even more frames. But refusing to unwind obviously broken frames is acceptable, too. Ulf ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 1/5] Revert "Optionally allow unknown symbols in the backtrace tests" 2017-04-25 12:54 ` [PATCH 1/5] Revert "Optionally allow unknown symbols in the backtrace tests" Mark Wielaard ` (2 preceding siblings ...) 2017-04-25 12:56 ` [PATCH 4/5] Add i386 frame pointer unwinder Mark Wielaard @ 2017-04-25 12:56 ` Ulf Hermann 2017-04-25 13:11 ` [PATCH 5/5] Add frame pointer unwinding for aarch64 Mark Wielaard 4 siblings, 0 replies; 37+ messages in thread From: Ulf Hermann @ 2017-04-25 12:56 UTC (permalink / raw) To: Mark Wielaard, elfutils-devel On 04/25/2017 02:49 PM, Mark Wielaard wrote: > This reverts commit f9971cb422df39adea7e8c7e22689b879e39c626. > > Allowing no symbol resolving at all makes it too hard to see > whether the test actually tests anything. > [...] Looks good to me. Ulf ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 5/5] Add frame pointer unwinding for aarch64 2017-04-25 12:54 ` [PATCH 1/5] Revert "Optionally allow unknown symbols in the backtrace tests" Mark Wielaard ` (3 preceding siblings ...) 2017-04-25 12:56 ` [PATCH 1/5] Revert "Optionally allow unknown symbols in the backtrace tests" Ulf Hermann @ 2017-04-25 13:11 ` Mark Wielaard 2017-04-25 21:55 ` Ulf Hermann 2017-04-25 22:13 ` Mark Wielaard 4 siblings, 2 replies; 37+ messages in thread From: Mark Wielaard @ 2017-04-25 13:11 UTC (permalink / raw) To: elfutils-devel; +Cc: Ulf Hermann From: Ulf Hermann <ulf.hermann@qt.io> If we don't find any debug information for a given frame, we usually cannot unwind any further. However, the binary in question might have been compiled with frame pointers, in which case we can look up the well known frame pointer locations in the stack snapshot and use them to bridge the frames without debug information. Signed-off-by: Ulf Hermann <ulf.hermann@qt.io> --- backends/ChangeLog | 6 +++ backends/Makefile.am | 2 +- backends/aarch64_init.c | 1 + backends/aarch64_unwind.c | 96 +++++++++++++++++++++++++++++++++ tests/ChangeLog | 7 +++ tests/Makefile.am | 3 ++ tests/backtrace.aarch64.fp.core.bz2 | Bin 0 -> 8437 bytes tests/backtrace.aarch64.fp.exec.bz2 | Bin 0 -> 394972 bytes tests/run-backtrace-fp-core-aarch64.sh | 28 ++++++++++ 9 files changed, 142 insertions(+), 1 deletion(-) create mode 100644 backends/aarch64_unwind.c create mode 100644 tests/backtrace.aarch64.fp.core.bz2 create mode 100644 tests/backtrace.aarch64.fp.exec.bz2 create mode 100755 tests/run-backtrace-fp-core-aarch64.sh diff --git a/backends/ChangeLog b/backends/ChangeLog index 733e72c..d742bca 100644 --- a/backends/ChangeLog +++ b/backends/ChangeLog @@ -6,6 +6,12 @@ 2017-02-09 Ulf Hermann <ulf.hermann@qt.io> + * aarch64_unwind.c: New file + * Makefile.am (aarch64_SRCS): Add aarch64_unwind.c + * aarch64_init.c (aarch64_init): Hook aarch64_unwind + +2017-02-09 Ulf Hermann <ulf.hermann@qt.io> + * x86_64_unwind.c: New file * Makefile.am (x86_64_SRCS): Add x86_64_unwind.c * x86_64_init.c (x86_64_init): Hook x86_64_unwind diff --git a/backends/Makefile.am b/backends/Makefile.am index 22eb6ac..ff80a82 100644 --- a/backends/Makefile.am +++ b/backends/Makefile.am @@ -80,7 +80,7 @@ am_libebl_arm_pic_a_OBJECTS = $(arm_SRCS:.c=.os) aarch64_SRCS = aarch64_init.c aarch64_regs.c aarch64_symbol.c \ aarch64_corenote.c aarch64_retval.c aarch64_cfi.c \ - aarch64_initreg.c + aarch64_initreg.c aarch64_unwind.c libebl_aarch64_pic_a_SOURCES = $(aarch64_SRCS) am_libebl_aarch64_pic_a_OBJECTS = $(aarch64_SRCS:.c=.os) diff --git a/backends/aarch64_init.c b/backends/aarch64_init.c index 6395f11..0866494 100644 --- a/backends/aarch64_init.c +++ b/backends/aarch64_init.c @@ -63,6 +63,7 @@ aarch64_init (Elf *elf __attribute__ ((unused)), + ALT_FRAME_RETURN_COLUMN (used when LR isn't used) = 97 DWARF regs. */ eh->frame_nregs = 97; HOOK (eh, set_initial_registers_tid); + HOOK (eh, unwind); return MODVERSION; } diff --git a/backends/aarch64_unwind.c b/backends/aarch64_unwind.c new file mode 100644 index 0000000..cac4ebd --- /dev/null +++ b/backends/aarch64_unwind.c @@ -0,0 +1,96 @@ +/* Get previous frame state for an existing frame state. + Copyright (C) 2016 The Qt Company Ltd. + This file is part of elfutils. + + This file is free software; you can redistribute it and/or modify + it under the terms of either + + * the GNU Lesser General Public License as published by the Free + Software Foundation; either version 3 of the License, or (at + your option) any later version + + or + + * the GNU General Public License as published by the Free + Software Foundation; either version 2 of the License, or (at + your option) any later version + + or both in parallel, as here. + + elfutils is distributed in the hope that it will be useful, but + WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + General Public License for more details. + + You should have received copies of the GNU General Public License and + the GNU Lesser General Public License along with this program. If + not, see <http://www.gnu.org/licenses/>. */ + +#ifdef HAVE_CONFIG_H +# include <config.h> +#endif + +#define BACKEND aarch64_ +#define FP_REG 29 +#define LR_REG 30 +#define SP_REG 31 +#define FP_OFFSET 0 +#define LR_OFFSET 8 +#define SP_OFFSET 16 + +#include "libebl_CPU.h" + +/* There was no CFI. Maybe we happen to have a frame pointer and can unwind from that? */ + +bool +EBLHOOK(unwind) (Ebl *ebl __attribute__ ((unused)), Dwarf_Addr pc __attribute__ ((unused)), + ebl_tid_registers_t *setfunc, ebl_tid_registers_get_t *getfunc, + ebl_pid_memory_read_t *readfunc, void *arg, + bool *signal_framep __attribute__ ((unused))) +{ + Dwarf_Word fp, lr, sp; + + if (!getfunc(LR_REG, 1, &lr, arg)) + return false; + + if (!getfunc(FP_REG, 1, &fp, arg)) + fp = 0; + + if (!getfunc(SP_REG, 1, &sp, arg)) + sp = 0; + + Dwarf_Word newPc, newLr, newFp, newSp; + + // The initial frame is special. We are expected to return lr directly in this case, and we'll + // come back to the same frame again in the next round. + if ((pc & 0x1) == 0) + { + newLr = lr; + newFp = fp; + newSp = sp; + } + else + { + if (!readfunc(fp + LR_OFFSET, &newLr, arg)) + newLr = 0; + + if (!readfunc(fp + FP_OFFSET, &newFp, arg)) + newFp = 0; + + newSp = fp + SP_OFFSET; + } + + newPc = newLr & (~0x1); + if (!setfunc(-1, 1, &newPc, arg)) + return false; + + // These are not fatal if they don't work. They will just prevent unwinding at the next frame. + setfunc(LR_REG, 1, &newLr, arg); + setfunc(FP_REG, 1, &newFp, arg); + setfunc(SP_REG, 1, &newSp, arg); + + // If the fp is invalid, we might still have a valid lr. + // But if the fp is valid, then the stack should be moving in the right direction. + // Except, if this is the initial frame. Then the stack doesn't move. + return newPc != 0 && (fp == 0 || newSp > sp || (pc & 0x1) == 0); +} diff --git a/tests/ChangeLog b/tests/ChangeLog index 67a44f7..b76d643 100644 --- a/tests/ChangeLog +++ b/tests/ChangeLog @@ -1,3 +1,10 @@ +2017-02-13 Ulf Hermann <ulf.hermann@qt.io> + + * Makefile.am: Add test for unwinding with frame pointers on aarch64 + * backtrace.aarch64.fp.core.bz2: New file + * backtrace.aarch64.fp.exec.bz2: New file + * run-backtrace-fp-core-aarch64.sh: New file + 2017-04-06 Mark Wielaard <mark@klomp.org> * run-backtrace-fp-core-i386.sh: New test. diff --git a/tests/Makefile.am b/tests/Makefile.am index 2c82bfd..3a12fe3 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -116,6 +116,7 @@ TESTS = run-arextract.sh run-arsymtest.sh newfile test-nlist \ run-backtrace-native-biarch.sh run-backtrace-native-core.sh \ run-backtrace-native-core-biarch.sh run-backtrace-core-x86_64.sh \ run-backtrace-fp-core-x86_64.sh \ + run-backtrace-fp-core-aarch64.sh \ run-backtrace-core-x32.sh \ run-backtrace-core-i386.sh run-backtrace-fp-core-i386.sh \ run-backtrace-core-ppc.sh \ @@ -297,6 +298,8 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh \ run-backtrace-core-x86_64.sh run-backtrace-core-i386.sh \ run-backtrace-fp-core-x86_64.sh \ run-backtrace-core-x32.sh \ + run-backtrace-fp-core-aarch64.sh \ + backtrace.aarch64.fp.core.bz2 backtrace.aarch64.fp.exec.bz2 \ backtrace-subr.sh backtrace.i386.core.bz2 backtrace.i386.exec.bz2 \ run-backtrace-fp-core-i386.sh \ backtrace.i386.fp.core.bz2 backtrace.i386.fp.exec.bz2 \ diff --git a/tests/run-backtrace-fp-core-aarch64.sh b/tests/run-backtrace-fp-core-aarch64.sh new file mode 100755 index 0000000..3b8d4ed --- /dev/null +++ b/tests/run-backtrace-fp-core-aarch64.sh @@ -0,0 +1,28 @@ +#! /bin/bash +# Copyright (C) 2017 The Qt Company +# This file is part of elfutils. +# +# This file is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# elfutils is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +. $srcdir/backtrace-subr.sh + +# The binary is generated by compiling with eh_frame CFI, but with frame +# pointers. +# +# gcc -static -O2 -fno-omit-frame-pointer -fno-asynchronous-unwind-tables \ +# -D_GNU_SOURCE -pthread -o tests/backtrace.aarch64.fp.exec -I. -Ilib \ +# tests/backtrace-child.c# +# The core is generated by calling the binary with --gencore + +check_core aarch64.fp -- 1.8.3.1 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 5/5] Add frame pointer unwinding for aarch64 2017-04-25 13:11 ` [PATCH 5/5] Add frame pointer unwinding for aarch64 Mark Wielaard @ 2017-04-25 21:55 ` Ulf Hermann 2017-04-25 22:13 ` Mark Wielaard 1 sibling, 0 replies; 37+ messages in thread From: Ulf Hermann @ 2017-04-25 21:55 UTC (permalink / raw) To: Mark Wielaard, elfutils-devel On 04/25/2017 02:49 PM, Mark Wielaard wrote: > From: Ulf Hermann <ulf.hermann@qt.io> > > If we don't find any debug information for a given frame, we usually > cannot unwind any further. However, the binary in question might have > been compiled with frame pointers, in which case we can look up the > well known frame pointer locations in the stack snapshot and use them > to bridge the frames without debug information. Looks good to me. > +# The binary is generated by compiling with eh_frame CFI, but with frame > +# pointers. > +# > +# gcc -static -O2 -fno-omit-frame-pointer -fno-asynchronous-unwind-tables \ > +# -D_GNU_SOURCE -pthread -o tests/backtrace.aarch64.fp.exec -I. -Ilib \ > +# tests/backtrace-child.c# Trailing '#', but that is insignificant. Ulf ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 5/5] Add frame pointer unwinding for aarch64 2017-04-25 13:11 ` [PATCH 5/5] Add frame pointer unwinding for aarch64 Mark Wielaard 2017-04-25 21:55 ` Ulf Hermann @ 2017-04-25 22:13 ` Mark Wielaard 2017-04-25 22:23 ` Ulf Hermann 1 sibling, 1 reply; 37+ messages in thread From: Mark Wielaard @ 2017-04-25 22:13 UTC (permalink / raw) To: elfutils-devel; +Cc: Ulf Hermann On Tue, 2017-04-25 at 14:49 +0200, Mark Wielaard wrote: > +bool > +EBLHOOK(unwind) (Ebl *ebl __attribute__ ((unused)), Dwarf_Addr pc __attribute__ ((unused)), > + ebl_tid_registers_t *setfunc, ebl_tid_registers_get_t *getfunc, > + ebl_pid_memory_read_t *readfunc, void *arg, > + bool *signal_framep __attribute__ ((unused))) > +{ > + Dwarf_Word fp, lr, sp; > + > + if (!getfunc(LR_REG, 1, &lr, arg)) > + return false; > + > + if (!getfunc(FP_REG, 1, &fp, arg)) > + fp = 0; > + > + if (!getfunc(SP_REG, 1, &sp, arg)) > + sp = 0; > + > + Dwarf_Word newPc, newLr, newFp, newSp; > + > + // The initial frame is special. We are expected to return lr directly in this case, and we'll > + // come back to the same frame again in the next round. > + if ((pc & 0x1) == 0) > + { > + newLr = lr; > + newFp = fp; > + newSp = sp; > + } > + else > + { > + if (!readfunc(fp + LR_OFFSET, &newLr, arg)) > + newLr = 0; > + > + if (!readfunc(fp + FP_OFFSET, &newFp, arg)) > + newFp = 0; > + > + newSp = fp + SP_OFFSET; > + } > + > + newPc = newLr & (~0x1); > + if (!setfunc(-1, 1, &newPc, arg)) > + return false; My question is about this "initial frame". In our testcase we don't have this case since the backtrace starts in a function that has some CFI. But I assume you have some tests that rely on this behavior. The first question is how/why the (pc & 0x1) == 0 check works? Why is that the correct check? Secondly, if it really is the initial (or signal frame) we are after, should we pass in into bool *signal_framep argument. Currently we don't Lastly could you instead of returning the frame itself with just the pc adjusted do that directly? if ((pc & 0x1) == 0) lr = lr & (~0x1); And then use the code in the else clause always? All the above might simply be me not understanding the significance of the initial frame. Cheers, Mark ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 5/5] Add frame pointer unwinding for aarch64 2017-04-25 22:13 ` Mark Wielaard @ 2017-04-25 22:23 ` Ulf Hermann 2017-04-26 15:24 ` Mark Wielaard 0 siblings, 1 reply; 37+ messages in thread From: Ulf Hermann @ 2017-04-25 22:23 UTC (permalink / raw) To: Mark Wielaard, elfutils-devel > My question is about this "initial frame". In our testcase we don't have > this case since the backtrace starts in a function that has some CFI. > But I assume you have some tests that rely on this behavior. Actually the test I provided does exercise this code. The initial __libc_do_syscall() frame does not have CFI. Only raise() has. You can check that by dropping the code for pc & 0x1. > The first question is how/why the (pc & 0x1) == 0 check works? > Why is that the correct check? > > Secondly, if it really is the initial (or signal frame) we are after, > should we pass in into bool *signal_framep argument. Currently we don't We have this piece of code in __libdwfl_frame_unwind, in frame_unwind.c: if (! state->initial_frame && ! state->signal_frame) pc--; AArch64 has a fixed instruction width of 32bit. So, normally the pc is aligned to 4 bytes. Except if we decrement it, then we are guaranteed to have an odd number, which we can then test to see if the frame in question is the initial or a signal frame. Of course it would be nicer to pass this information directly, but the signal_frame parameter is supposed to be an output parameter. After all we do the following after calling ebl_unwind(): state->unwound->signal_frame = signal_frame; > Lastly could you instead of returning the frame itself with just the pc > adjusted do that directly? > > if ((pc & 0x1) == 0) > lr = lr & (~0x1); If I dig up the first frame after the initial one from the stack, then we drop whatever we initially had in LR. Apparently, on aarch64 PC is always one frame "ahead" of the other registers. To establish that, we have to set PC to the value of LR on the initial frame, without actually unwinding. br, Ulf ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 5/5] Add frame pointer unwinding for aarch64 2017-04-25 22:23 ` Ulf Hermann @ 2017-04-26 15:24 ` Mark Wielaard 2017-04-27 14:02 ` Ulf Hermann 0 siblings, 1 reply; 37+ messages in thread From: Mark Wielaard @ 2017-04-26 15:24 UTC (permalink / raw) To: Ulf Hermann; +Cc: elfutils-devel On Tue, 2017-04-25 at 15:38 +0200, Ulf Hermann wrote: > > My question is about this "initial frame". In our testcase we don't have > > this case since the backtrace starts in a function that has some CFI. > > But I assume you have some tests that rely on this behavior. > > Actually the test I provided does exercise this code. The initial > __libc_do_syscall() frame does not have CFI. Only raise() has. You can > check that by dropping the code for pc & 0x1. Maybe I am using the wrong binaries (exec and core), but for me there is no difference. With or with commenting out the adjustments: diff --git a/backends/aarch64_unwind.c b/backends/aarch64_unwind.c index cac4ebd..36cd0e1 100644 --- a/backends/aarch64_unwind.c +++ b/backends/aarch64_unwind.c @@ -63,6 +63,7 @@ EBLHOOK(unwind) (Ebl *ebl __attribute__ ((unused)), Dwarf_Addr pc __attribute__ // The initial frame is special. We are expected to return lr directly in this case, and we'll // come back to the same frame again in the next round. +/* if ((pc & 0x1) == 0) { newLr = lr; @@ -70,6 +71,7 @@ EBLHOOK(unwind) (Ebl *ebl __attribute__ ((unused)), Dwarf_Addr pc __attribute__ newSp = sp; } else +*/ { if (!readfunc(fp + LR_OFFSET, &newLr, arg)) newLr = 0; @@ -80,7 +82,7 @@ EBLHOOK(unwind) (Ebl *ebl __attribute__ ((unused)), Dwarf_Addr pc __attribute__ newSp = fp + SP_OFFSET; } - newPc = newLr & (~0x1); + newPc = newLr /* & (~0x1) */; if (!setfunc(-1, 1, &newPc, arg)) return false; @@ -92,5 +94,5 @@ EBLHOOK(unwind) (Ebl *ebl __attribute__ ((unused)), Dwarf_Addr pc __attribute__ // If the fp is invalid, we might still have a valid lr. // But if the fp is valid, then the stack should be moving in the right direction. // Except, if this is the initial frame. Then the stack doesn't move. - return newPc != 0 && (fp == 0 || newSp > sp || (pc & 0x1) == 0); + return newPc != 0 && (fp == 0 || newSp > sp /* || (pc & 0x1) == 0 */); } The testcase (run-backtrace-fp-core-aarch64.sh) PASSes and produces the same output for: LD_LIBRARY_PATH=backends:libelf:libdw src/stack -v --exec backtrace.aarch64.fp.exec --core backtrace.aarch64.fp.core PID 349 - core TID 350: #0 0x000000000040583c raise - /home/ulf/backtrace.aarch64.fp.exec ../nptl/sysdeps/unix/sysv/linux/pt-raise.c:37 #1 0x0000000000401aac - 1 sigusr2 - /home/ulf/backtrace.aarch64.fp.exec #2 0x0000000000401ba8 - 1 stdarg - /home/ulf/backtrace.aarch64.fp.exec #3 0x0000000000401c04 - 1 backtracegen - /home/ulf/backtrace.aarch64.fp.exec #4 0x0000000000401c10 - 1 start - /home/ulf/backtrace.aarch64.fp.exec #5 0x0000000000402f44 - 1 start_thread - /home/ulf/backtrace.aarch64.fp.exec /build/glibc-MsMi75/glibc-2.19/nptl/pthread_create.c:311 #6 0x000000000041dc70 - 1 __clone - /home/ulf/backtrace.aarch64.fp.exec TID 349: #0 0x0000000000403fcc pthread_join - /home/ulf/backtrace.aarch64.fp.exec /build/glibc-MsMi75/glibc-2.19/nptl/pthread_join.c:92 #1 0x0000000000401810 - 1 main - /home/ulf/backtrace.aarch64.fp.exec #2 0x0000000000406544 - 1 __libc_start_main - /home/ulf/backtrace.aarch64.fp.exec #3 0x0000000000401918 - 1 $x - /home/ulf/backtrace.aarch64.fp.exec src/stack: dwfl_thread_getframes tid 349 at 0x401917 in /home/ulf/backtrace.aarch64.fp.exec: address out of range Since I cannot find the __libc_do_syscall I assume I am not using the right executable & core? Could you double check them on the mjw/fp-unwind branch? > > The first question is how/why the (pc & 0x1) == 0 check works? > > Why is that the correct check? > > > > Secondly, if it really is the initial (or signal frame) we are after, > > should we pass in into bool *signal_framep argument. Currently we don't > > We have this piece of code in __libdwfl_frame_unwind, in frame_unwind.c: > > if (! state->initial_frame && ! state->signal_frame) > pc--; > > AArch64 has a fixed instruction width of 32bit. So, normally the pc is > aligned to 4 bytes. Except if we decrement it, then we are guaranteed > to have an odd number, which we can then test to see if the frame in > question is the initial or a signal frame. Aha, OK. I forgot we explicitly decrement the pc for the frame before doing the actual unwind. That makes sense. > Of course it would be nicer to pass this information directly, but the > signal_frame parameter is supposed to be an output parameter. After > all we do the following after calling ebl_unwind(): > > state->unwound->signal_frame = signal_frame; Right, but that doesn't mean we couldn't also provide it as input if we know that it is a signal or initial frame already. It just means that unwinders would have to explicitly set it to false if cannot determine it for the unwound frame (which is for all of them except the s390x unwinder). It would really be just one line change in the call to and in the unwinder functions. This isn't a public API, so we can change it to be smarter. Cheers, Mark ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 5/5] Add frame pointer unwinding for aarch64 2017-04-26 15:24 ` Mark Wielaard @ 2017-04-27 14:02 ` Ulf Hermann 2017-04-27 14:29 ` Mark Wielaard 0 siblings, 1 reply; 37+ messages in thread From: Ulf Hermann @ 2017-04-27 14:02 UTC (permalink / raw) To: Mark Wielaard; +Cc: elfutils-devel On 04/26/2017 04:33 PM, Mark Wielaard wrote: > On Tue, 2017-04-25 at 15:38 +0200, Ulf Hermann wrote: >>> My question is about this "initial frame". In our testcase we don't have >>> this case since the backtrace starts in a function that has some CFI. >>> But I assume you have some tests that rely on this behavior. >> >> Actually the test I provided does exercise this code. The initial >> __libc_do_syscall() frame does not have CFI. Only raise() has. You can >> check that by dropping the code for pc & 0x1. > > Maybe I am using the wrong binaries (exec and core), but for me there is > no difference. In fact, with the new binaries there is no difference. I was confused, sorry. However, if you strip .eh_frame and .eh_frame_hdr from the exe (thus triggering the fp unwinding on the first frame), you will see that it skips sigusr2. At the same time it invents another frame 0x403f40 on the main thread. Apparently pthread_join creates two stack frames. As it correctly unwinds the rest, the latter seemed harmless to me. With .eh_frame and .eh_frame_hdr: ulf@zebra:~/dev/build-elfutils/tests$ ./backtrace --core=backtrace.aarch64.fp.core -e backtrace.aarch64.fp.exec 0x400000 0x4a3000 /home/ulf/backtrace.aarch64.fp.exec 0x7fb6380000 0x7fb6381000 linux-vdso.so.1 TID 350: # 0 0x40583c raise # 1 0x401aac - 1 sigusr2 # 2 0x401ba8 - 1 stdarg # 3 0x401c04 - 1 backtracegen # 4 0x401c10 - 1 start # 5 0x402f44 - 1 start_thread # 6 0x41dc70 - 1 __clone TID 349: # 0 0x403fcc pthread_join # 1 0x401810 - 1 main # 2 0x406544 - 1 __libc_start_main # 3 0x401918 - 1 $x ./backtrace: dwfl_thread_getframes: address out of range Without .eh_frame and .eh_frame_hdr, code from PATCH V2: ulf@zebra:~/dev/build-elfutils/tests$ ./backtrace --core=backtrace.aarch64.fp.core -e backtrace.aarch64.fp.stripped 0x400000 0x4a3000 /home/ulf/backtrace.aarch64.fp.exec 0x7fb6380000 0x7fb6381000 linux-vdso.so.1 TID 350: # 0 0x40583c (null) # 1 0x401aac - 1 (null) # 2 0x401ba8 - 1 (null) # 3 0x401c04 - 1 (null) # 4 0x401c10 - 1 (null) # 5 0x402f44 - 1 (null) # 6 0x41dc70 - 1 (null) ./backtrace: dwfl_thread_getframes: address out of range TID 349: # 0 0x403fcc (null) # 1 0x403f40 - 1 (null) # 2 0x401810 - 1 (null) # 3 0x406544 - 1 (null) # 4 0x401918 - 1 (null) ./backtrace: dwfl_thread_getframes: address out of range Without .eh_frame and .eh_frame_hdr, without initial frame adjustment: ulf@zebra:~/dev/build-elfutils/tests$ ./backtrace --core=backtrace.aarch64.fp.core -e backtrace.aarch64.fp.stripped 0x400000 0x4a3000 /home/ulf/backtrace.aarch64.fp.exec 0x7fb6380000 0x7fb6381000 linux-vdso.so.1 TID 350: # 0 0x40583c (null) # 1 0x401ba8 - 1 (null) # 2 0x401c04 - 1 (null) # 3 0x401c10 - 1 (null) # 4 0x402f44 - 1 (null) # 5 0x41dc70 - 1 (null) ./backtrace: dwfl_thread_getframes: address out of range TID 349: # 0 0x403fcc (null) # 1 0x401810 - 1 (null) # 2 0x406544 - 1 (null) # 3 0x401918 - 1 (null) ./backtrace: dwfl_thread_getframes: address out of range You have to drop all the asserts from backtrace.c to actually test this: diff --git a/tests/backtrace.c b/tests/backtrace.c index 1ff6353..a910a77 100644 --- a/tests/backtrace.c +++ b/tests/backtrace.c @@ -71,14 +71,14 @@ static void callback_verify (pid_t tid, unsigned frameno, Dwarf_Addr pc, const char *symname, Dwfl *dwfl) { - static bool seen_main = false; +// static bool seen_main = false; if (symname && *symname == '.') symname++; - if (symname && strcmp (symname, "main") == 0) - seen_main = true; +// if (symname && strcmp (symname, "main") == 0) +// seen_main = true; if (pc == 0) { - assert (seen_main); +// assert (seen_main); return; } if (check_tid == 0) @@ -103,11 +103,11 @@ callback_verify (pid_t tid, unsigned frameno, Dwarf_Addr pc, && (strcmp (symname, "__kernel_vsyscall") == 0 || strcmp (symname, "__libc_do_syscall") == 0)) reduce_frameno = true; - else - assert (symname && strcmp (symname, "raise") == 0); +// else +// assert (symname && strcmp (symname, "raise") == 0); break; case 1: - assert (symname != NULL && strcmp (symname, "sigusr2") == 0); +// assert (symname != NULL && strcmp (symname, "sigusr2") == 0); break; case 2: // x86_64 only /* __restore_rt - glibc maybe does not have to have this symbol. */ @@ -125,11 +125,11 @@ callback_verify (pid_t tid, unsigned frameno, Dwarf_Addr pc, } /* FALLTHRU */ case 4: - assert (symname != NULL && strcmp (symname, "stdarg") == 0); +// assert (symname != NULL && strcmp (symname, "stdarg") == 0); break; case 5: /* Verify we trapped on the very last instruction of child. */ - assert (symname != NULL && strcmp (symname, "backtracegen") == 0); +// assert (symname != NULL && strcmp (symname, "backtracegen") == 0); mod = dwfl_addrmodule (dwfl, pc); if (mod) symname2 = dwfl_module_addrname (mod, pc); ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 5/5] Add frame pointer unwinding for aarch64 2017-04-27 14:02 ` Ulf Hermann @ 2017-04-27 14:29 ` Mark Wielaard 2017-04-27 14:35 ` Ulf Hermann 0 siblings, 1 reply; 37+ messages in thread From: Mark Wielaard @ 2017-04-27 14:29 UTC (permalink / raw) To: Ulf Hermann; +Cc: elfutils-devel [-- Attachment #1: Type: text/plain, Size: 2661 bytes --] On Wed, 2017-04-26 at 17:27 +0200, Ulf Hermann wrote: > However, if you strip .eh_frame and .eh_frame_hdr from the exe (thus > triggering the fp unwinding on the first frame), you will see that it > skips sigusr2. At the same time it invents another frame 0x403f40 on > the main thread. Apparently pthread_join creates two stack frames. As > it correctly unwinds the rest, the latter seemed harmless to me. I am a little concerned about testing against an exec where .eh_frame is forcibly removed since that is an allocated section you are messing up the binary (which shows because the symbol table doesn't match anymore). It seems nicer to do the checks instead with a hacked up libdwfl/frame_unwind.c that simply doesn't handle cfi and so always uses the frame pointer unwinder: diff --git a/libdwfl/frame_unwind.c b/libdwfl/frame_unwind.c index fb42c1a..6de64e5 100644 --- a/libdwfl/frame_unwind.c +++ b/libdwfl/frame_unwind.c @@ -539,6 +539,7 @@ new_unwound (Dwfl_Frame *state) static void handle_cfi (Dwfl_Frame *state, Dwarf_Addr pc, Dwarf_CFI *cfi, Dwarf_Addr bias) { + return; Dwarf_Frame *frame; if (INTUSE(dwarf_cfi_addrframe) (cfi, pc, &frame) != 0) { You are right that in that case we loose/skip over sigusr2 from raise and end up at stdarg directly if we remove the pc & 0x1 check. But... that really is because we deliberately skip it. Proper/simple link-register/frame unwinding should say: - newPc = newLr & (~0x1); + newPc = lr; The newPc is the current link register, not the new one. With that we get the backtrace as expected. But... I now realize why you needed something like that in the case of mixed CFI/no-framepointer/no-CFI/framepointer code. Like we have in our testcase. In that case there is no good way to determine whether or not there really were proper frame pointers and/or how the previous frame was unwound. Our testcase is somewhat mean by using some signal/no-return code which, which is hard to properly unwind without full frame pointers or full CFI. And with the simpler code that doesn't try to guess whether or not to skip a frame you do end up with an extra siguser2 and/or main frame. We could try to be clever and realize the link register and pc are the same and then use the newLR instead as newPC. That however might just mean that it is a recursive call to the same function. So maybe the proper "fix" for that is to make our testcase a little less strict and allow the occasional extra frame instead of trying to make the frame pointer unwinder "extra smart". Maybe something like the attached patch? Cheers, Mark [-- Attachment #2: simple_fp_unwind.patch --] [-- Type: text/x-patch, Size: 3030 bytes --] diff --git a/backends/aarch64_unwind.c b/backends/aarch64_unwind.c index cac4ebd..18aaf9a 100644 --- a/backends/aarch64_unwind.c +++ b/backends/aarch64_unwind.c @@ -61,26 +61,15 @@ EBLHOOK(unwind) (Ebl *ebl __attribute__ ((unused)), Dwarf_Addr pc __attribute__ Dwarf_Word newPc, newLr, newFp, newSp; - // The initial frame is special. We are expected to return lr directly in this case, and we'll - // come back to the same frame again in the next round. - if ((pc & 0x1) == 0) - { - newLr = lr; - newFp = fp; - newSp = sp; - } - else - { - if (!readfunc(fp + LR_OFFSET, &newLr, arg)) - newLr = 0; - - if (!readfunc(fp + FP_OFFSET, &newFp, arg)) - newFp = 0; - - newSp = fp + SP_OFFSET; - } - - newPc = newLr & (~0x1); + if (!readfunc(fp + LR_OFFSET, &newLr, arg)) + newLr = 0; + + if (!readfunc(fp + FP_OFFSET, &newFp, arg)) + newFp = 0; + + newSp = fp + SP_OFFSET; + + newPc = lr; if (!setfunc(-1, 1, &newPc, arg)) return false; @@ -91,6 +80,5 @@ EBLHOOK(unwind) (Ebl *ebl __attribute__ ((unused)), Dwarf_Addr pc __attribute__ // If the fp is invalid, we might still have a valid lr. // But if the fp is valid, then the stack should be moving in the right direction. - // Except, if this is the initial frame. Then the stack doesn't move. - return newPc != 0 && (fp == 0 || newSp > sp || (pc & 0x1) == 0); + return newPc != 0 && (fp == 0 || newSp > sp); } diff --git a/tests/backtrace-subr.sh b/tests/backtrace-subr.sh index a303e32..9731c43 100644 --- a/tests/backtrace-subr.sh +++ b/tests/backtrace-subr.sh @@ -59,7 +59,7 @@ check_backtracegen() # Ignore it here as it is a bug of OS, not a bug of elfutils. check_err() { - if [ $(egrep -v <$1 'dwfl_thread_getframes: (No DWARF information found|no matching address range|address out of range)$' \ + if [ $(egrep -v <$1 'dwfl_thread_getframes: (No DWARF information found|no matching address range|address out of range|Invalid register)$' \ | wc -c) \ -eq 0 ] then diff --git a/tests/backtrace.c b/tests/backtrace.c index 1ff6353..21abe8a 100644 --- a/tests/backtrace.c +++ b/tests/backtrace.c @@ -90,6 +90,10 @@ callback_verify (pid_t tid, unsigned frameno, Dwarf_Addr pc, return; } Dwfl_Module *mod; + /* See case 4. Special case to help out simple frame pointer unwinders. */ + static bool duplicate_sigusr2 = false; + if (duplicate_sigusr2) + frameno--; static bool reduce_frameno = false; if (reduce_frameno) frameno--; @@ -125,6 +129,14 @@ callback_verify (pid_t tid, unsigned frameno, Dwarf_Addr pc, } /* FALLTHRU */ case 4: + /* Some simple frame unwinders get this wrong and think sigusr2 + is calling itself again. Allow it and just pretend there is + an extra sigusr2 frame. */ + if (symname != NULL && strcmp (symname, "sigusr2") == 0) + { + duplicate_sigusr2 = true; + break; + } assert (symname != NULL && strcmp (symname, "stdarg") == 0); break; case 5: ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 5/5] Add frame pointer unwinding for aarch64 2017-04-27 14:29 ` Mark Wielaard @ 2017-04-27 14:35 ` Ulf Hermann 2017-04-27 15:09 ` Mark Wielaard 2017-05-03 8:46 ` Mark Wielaard 0 siblings, 2 replies; 37+ messages in thread From: Ulf Hermann @ 2017-04-27 14:35 UTC (permalink / raw) To: Mark Wielaard; +Cc: elfutils-devel > Maybe something like the attached patch? Well that's actually the original patch (as opposed to V2) with relaxed test conditions. You can write that a bit nicer by setting the new PC directly after retrieving LR and returning early if it doesn't work. See "[PATCH 2/3] Add frame pointer unwinding as fallback on arm" from February 16th. That's the original algorithm; for aarch64 I just added a few defines and included arm_unwind.c. It's in fact a bit annoying for my use case as the non-CFI stack sections are mostly in between CFI-enabled stack sections here. However, I can accept this. cheers, Ulf ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 5/5] Add frame pointer unwinding for aarch64 2017-04-27 14:35 ` Ulf Hermann @ 2017-04-27 15:09 ` Mark Wielaard 2017-04-27 15:42 ` Ulf Hermann 2017-05-03 8:46 ` Mark Wielaard 1 sibling, 1 reply; 37+ messages in thread From: Mark Wielaard @ 2017-04-27 15:09 UTC (permalink / raw) To: Ulf Hermann; +Cc: elfutils-devel On Thu, Apr 27, 2017 at 10:31:55AM +0200, Ulf Hermann wrote: > > Maybe something like the attached patch? > > Well that's actually the original patch (as opposed to V2) with relaxed test > conditions. You can write that a bit nicer by setting the new PC directly > after retrieving LR and returning early if it doesn't work. See "[PATCH 2/3] > Add frame pointer unwinding as fallback on arm" from February 16th. That's > the original algorithm; for aarch64 I just added a few defines and included > arm_unwind.c. I think the simplier implementation with relaxed test is better. I'll adjust the patch to look more like your original. > It's in fact a bit annoying for my use case as the non-CFI stack sections > are mostly in between CFI-enabled stack sections here. However, I can accept > this. Does every fp-only frame gets duplicated after a DWARF CFI frame? I'll look if I can better understand why that is. Cheers, Mark ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 5/5] Add frame pointer unwinding for aarch64 2017-04-27 15:09 ` Mark Wielaard @ 2017-04-27 15:42 ` Ulf Hermann 0 siblings, 0 replies; 37+ messages in thread From: Ulf Hermann @ 2017-04-27 15:42 UTC (permalink / raw) To: Mark Wielaard; +Cc: elfutils-devel > Does every fp-only frame gets duplicated after a DWARF CFI frame? > I'll look if I can better understand why that is. The last thing I've tested on an actual aarch64 setup is what I'm removing in this change: https://codereview.qt-project.org/#/c/191650/5/3rdparty/elfutils/backends/aarch64_unwind.c As you can see, I'm setting PC to the thing I read from memory, not the value of the LR register. So I must have had the same problem when I wrote that. Ulf ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 5/5] Add frame pointer unwinding for aarch64 2017-04-27 14:35 ` Ulf Hermann 2017-04-27 15:09 ` Mark Wielaard @ 2017-05-03 8:46 ` Mark Wielaard 1 sibling, 0 replies; 37+ messages in thread From: Mark Wielaard @ 2017-05-03 8:46 UTC (permalink / raw) To: Ulf Hermann; +Cc: elfutils-devel On Thu, 2017-04-27 at 10:31 +0200, Ulf Hermann wrote: > > Maybe something like the attached patch? > > Well that's actually the original patch (as opposed to V2) with relaxed > test conditions. You can write that a bit nicer by setting the new PC > directly after retrieving LR and returning early if it doesn't work. See > "[PATCH 2/3] Add frame pointer unwinding as fallback on arm" from > February 16th. That's the original algorithm; for aarch64 I just added a > few defines and included arm_unwind.c. OK, I made that cleanup and rebased the mjw/fp-unwind branch to master. All patches on that branch are now pushed so for 0.169 we should have a frame pointer unwinder backtrace fallback for at least x86_64, i686 and aarch64. Thanks for all your work and sorry this took a while to land. Cheers, Mark ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: frame unwinding patches 2017-04-25 12:50 ` Mark Wielaard 2017-04-25 12:54 ` [PATCH 1/5] Revert "Optionally allow unknown symbols in the backtrace tests" Mark Wielaard @ 2017-04-26 15:20 ` Ulf Hermann 1 sibling, 0 replies; 37+ messages in thread From: Ulf Hermann @ 2017-04-26 15:20 UTC (permalink / raw) To: Mark Wielaard; +Cc: elfutils-devel > I dropped the arm32 frame pointer unwinder for now (maybe we need a less > demanding testcase for that or, more awesome, add code to translate the > exidx section for that). Another problem is that QV4-generated code on a new frame pushes LR first and then FP. Code generated by gcc with "-arm -mapcs-frame -fno-omit-frame-pointer" pushes FP first and then LR. The libc raise() I have here miraculously does the same as QV4. Also, QV4 can alternatively use either r11 or r7 for LR, depending on if we're in ARM or THUMB mode (which I cannot detect in the unwind hook). As that is written somewhere in AAPCS, I guess you can coax gcc to do the same thing (but just leaving out the "-arm" above simply leads to no frame pointers at all). Well, let's forget about this for now. I'll keep something that works with QV4 in ARM mode and ignore everything else. Ulf ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: frame unwinding patches 2017-04-03 9:00 ` Milian Wolff 2017-04-03 9:03 ` Ulf Hermann @ 2017-04-03 21:23 ` Jan Kratochvil 2017-04-04 7:40 ` Milian Wolff 1 sibling, 1 reply; 37+ messages in thread From: Jan Kratochvil @ 2017-04-03 21:23 UTC (permalink / raw) To: Milian Wolff; +Cc: elfutils-devel, Mark Wielaard, Ulf Hermann On Mon, 03 Apr 2017 11:00:03 +0200, Milian Wolff wrote: > I just got a report from a colleague. As-is, elfutils would fail to unwind > from the following location in his application: > > 0x1137ca4 > > With the x86_64 patch applied, he got a proper backtrace: S/he has something wrong with the compiler. With -fasynchronous-unwind-tables frame pointer unwinding is never needed and gcc defaults to -fasynchronous-unwind-tables on x86_64. This is why I haven't implemented it originally as it only paper overs the real problem and it leads to unreliable backtraces in longterm. Jan ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: frame unwinding patches 2017-04-03 21:23 ` Jan Kratochvil @ 2017-04-04 7:40 ` Milian Wolff 2017-04-04 7:55 ` Jan Kratochvil 0 siblings, 1 reply; 37+ messages in thread From: Milian Wolff @ 2017-04-04 7:40 UTC (permalink / raw) To: elfutils-devel; +Cc: Jan Kratochvil, Mark Wielaard, Ulf Hermann On Monday, April 3, 2017 11:23:25 PM CEST Jan Kratochvil wrote: > On Mon, 03 Apr 2017 11:00:03 +0200, Milian Wolff wrote: > > I just got a report from a colleague. As-is, elfutils would fail to unwind > > from the following location in his application: > > > > 0x1137ca4 > > > With the x86_64 patch applied, he got a proper backtrace: > > S/he has something wrong with the compiler. With > -fasynchronous-unwind-tables frame pointer unwinding is never needed > and gcc defaults to -fasynchronous-unwind-tables on x86_64. > > This is why I haven't implemented it originally as it only paper overs the > real problem and it leads to unreliable backtraces in longterm. Please reconsider: - In the example above, the address points into libnvidia-glcore.so and as such not compiled by my colleague but rather provided by NVidia as a binary blob. When you only got a binary blob and have to make do with it, you cannot tell people to "just fix the compiler invocation". - Some JIT compilers, like QV4, actually embed frame pointers into their dynamic code, but do not go the extra mile for generating DWARF data or asynchronous unwind tables. That is another case where the patches by Ulf excel and make elfutils much more useful. Please, as a user of elfutils I strongly hope that this patch set gets accepted. In general, similar patches that make it more resilient in the face of broken setups should also at least get considered instead of right-out rejected because "something is broken". Things break all the time, but developers often have to live with the brokenness. Cheers -- Milian Wolff mail@milianw.de http://milianw.de ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: frame unwinding patches 2017-04-04 7:40 ` Milian Wolff @ 2017-04-04 7:55 ` Jan Kratochvil 2017-04-04 8:25 ` Ulf Hermann 0 siblings, 1 reply; 37+ messages in thread From: Jan Kratochvil @ 2017-04-04 7:55 UTC (permalink / raw) To: Milian Wolff; +Cc: elfutils-devel, Mark Wielaard, Ulf Hermann On Tue, 04 Apr 2017 09:40:06 +0200, Milian Wolff wrote: > - In the example above, the address points into libnvidia-glcore.so and as > such not compiled by my colleague but rather provided by NVidia as a binary > blob. When you only got a binary blob and have to make do with it, you cannot > tell people to "just fix the compiler invocation". This is their problem they support a vendor who cripples usage of their products. There is also Intel and AMD. > - Some JIT compilers, like QV4, actually embed frame pointers into their > dynamic code, but do not go the extra mile for generating DWARF data or > asynchronous unwind tables. That is another case where the patches by Ulf > excel and make elfutils much more useful. In such case elfutils could provide some workaround with a new eu-stack option: --please-workaround-a-completely-broken-compiler-i-still-have-not-fixed :-) Jan ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: frame unwinding patches 2017-04-04 7:55 ` Jan Kratochvil @ 2017-04-04 8:25 ` Ulf Hermann 0 siblings, 0 replies; 37+ messages in thread From: Ulf Hermann @ 2017-04-04 8:25 UTC (permalink / raw) To: Jan Kratochvil, Milian Wolff; +Cc: elfutils-devel, Mark Wielaard >> - In the example above, the address points into libnvidia-glcore.so and as >> such not compiled by my colleague but rather provided by NVidia as a binary >> blob. When you only got a binary blob and have to make do with it, you cannot >> tell people to "just fix the compiler invocation". > > This is their problem they support a vendor who cripples usage of their > products. There is also Intel and AMD. Sorry, but I cannot tell everybody with binary-only graphics drivers that they cannot use perfparser. That's probably the majority of embedded devices and a large number of desktops. >> - Some JIT compilers, like QV4, actually embed frame pointers into their >> dynamic code, but do not go the extra mile for generating DWARF data or >> asynchronous unwind tables. That is another case where the patches by Ulf >> excel and make elfutils much more useful. > > In such case elfutils could provide some workaround with a new eu-stack option: > --please-workaround-a-completely-broken-compiler-i-still-have-not-fixed Frame pointers are the easiest way to include unwinding information into a binary and they require less work from the compiler than other methods. With JIT compilers, compile time matters much more than with ahead of time compilers. Also, adding extra code to the compiler has to be justified in that case as the compiler is shipped inside the libQt5Qml binary and loaded into memory whenever you run some QML. So, I think frame pointers are a perfectly valid option for unwinding and should be supported. ^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2017-05-02 14:43 UTC | newest] Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-15 23:34 frame unwinding patches Mark Wielaard 2017-02-15 23:34 ` [PATCH 3/3] Add frame pointer unwinding for aarch64 Mark Wielaard 2017-02-15 23:34 ` [PATCH 2/3] Add frame pointer unwinding as fallback on arm Mark Wielaard 2017-02-15 23:34 ` [PATCH 1/3] Add frame pointer unwinding as fallback on x86_64 Mark Wielaard 2017-02-16 9:13 ` frame unwinding patches Ulf Hermann 2017-04-03 9:00 ` Milian Wolff 2017-04-03 9:03 ` Ulf Hermann 2017-04-03 21:14 ` Mark Wielaard 2017-04-07 10:27 ` Mark Wielaard 2017-04-11 10:16 ` Ulf Hermann 2017-04-19 19:48 ` Mark Wielaard 2017-04-20 9:26 ` Ulf Hermann 2017-04-25 12:50 ` Mark Wielaard 2017-04-25 12:54 ` [PATCH 1/5] Revert "Optionally allow unknown symbols in the backtrace tests" Mark Wielaard 2017-04-25 12:50 ` [PATCH 2/5] tests: Add core backtracegen chec and regenerate ppc32 backtrace test files Mark Wielaard 2017-04-25 13:04 ` Ulf Hermann 2017-04-25 12:55 ` [PATCH 3/5] Add frame pointer unwinding as fallback on x86_64 Mark Wielaard 2017-04-25 13:05 ` Ulf Hermann 2017-04-25 12:56 ` [PATCH 4/5] Add i386 frame pointer unwinder Mark Wielaard 2017-04-25 13:38 ` Ulf Hermann 2017-04-25 12:56 ` [PATCH 1/5] Revert "Optionally allow unknown symbols in the backtrace tests" Ulf Hermann 2017-04-25 13:11 ` [PATCH 5/5] Add frame pointer unwinding for aarch64 Mark Wielaard 2017-04-25 21:55 ` Ulf Hermann 2017-04-25 22:13 ` Mark Wielaard 2017-04-25 22:23 ` Ulf Hermann 2017-04-26 15:24 ` Mark Wielaard 2017-04-27 14:02 ` Ulf Hermann 2017-04-27 14:29 ` Mark Wielaard 2017-04-27 14:35 ` Ulf Hermann 2017-04-27 15:09 ` Mark Wielaard 2017-04-27 15:42 ` Ulf Hermann 2017-05-03 8:46 ` Mark Wielaard 2017-04-26 15:20 ` frame unwinding patches Ulf Hermann 2017-04-03 21:23 ` Jan Kratochvil 2017-04-04 7:40 ` Milian Wolff 2017-04-04 7:55 ` Jan Kratochvil 2017-04-04 8:25 ` Ulf Hermann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).