| Message ID | 20230617014930.2070-4-namit@vmware.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: improve debuggability | expand |
On Sat, Jun 17, 2023 at 01:49:27AM +0000, Nadav Amit wrote: > From: Nadav Amit <namit@vmware.com> > > Enable frame pointers for arm64 and perform stack unwinding based on > arm64 convention. > > Signed-off-by: Nadav Amit <namit@vmware.com> > --- > arm/Makefile.arm | 3 --- > arm/Makefile.arm64 | 1 + > arm/Makefile.common | 3 +++ > lib/arm64/asm/stack.h | 3 +++ > lib/arm64/stack.c | 37 +++++++++++++++++++++++++++++++++++++ > 5 files changed, 44 insertions(+), 3 deletions(-) > create mode 100644 lib/arm64/stack.c > > diff --git a/arm/Makefile.arm b/arm/Makefile.arm > index 2ce00f5..7fd39f3 100644 > --- a/arm/Makefile.arm > +++ b/arm/Makefile.arm > @@ -11,9 +11,6 @@ ifeq ($(CONFIG_EFI),y) > $(error Cannot build arm32 tests as EFI apps) > endif > > -# stack.o relies on frame pointers. > -KEEP_FRAME_POINTER := y > - > CFLAGS += $(machine) > CFLAGS += -mcpu=$(PROCESSOR) > CFLAGS += -mno-unaligned-access > diff --git a/arm/Makefile.arm64 b/arm/Makefile.arm64 > index eada7f9..60385e2 100644 > --- a/arm/Makefile.arm64 > +++ b/arm/Makefile.arm64 > @@ -21,6 +21,7 @@ define arch_elf_check = > endef > > cstart.o = $(TEST_DIR)/cstart64.o > +cflatobjs += lib/arm64/stack.o > cflatobjs += lib/arm64/processor.o > cflatobjs += lib/arm64/spinlock.o > cflatobjs += lib/arm64/gic-v3-its.o lib/arm64/gic-v3-its-cmd.o > diff --git a/arm/Makefile.common b/arm/Makefile.common > index f904702..7fecfb3 100644 > --- a/arm/Makefile.common > +++ b/arm/Makefile.common > @@ -22,6 +22,9 @@ $(TEST_DIR)/sieve.elf: AUXFLAGS = 0x1 > ################################################################## > AUXFLAGS ?= 0x0 > > +# stack.o relies on frame pointers. > +KEEP_FRAME_POINTER := y > + > CFLAGS += -std=gnu99 > CFLAGS += -ffreestanding > CFLAGS += -O2 > diff --git a/lib/arm64/asm/stack.h b/lib/arm64/asm/stack.h > index d000624..be486cf 100644 > --- a/lib/arm64/asm/stack.h > +++ b/lib/arm64/asm/stack.h > @@ -5,4 +5,7 @@ > #error Do not directly include <asm/stack.h>. Just use <stack.h>. > #endif > > +#define HAVE_ARCH_BACKTRACE_FRAME > +#define HAVE_ARCH_BACKTRACE > + > #endif > diff --git a/lib/arm64/stack.c b/lib/arm64/stack.c > new file mode 100644 > index 0000000..1e2568a > --- /dev/null > +++ b/lib/arm64/stack.c > @@ -0,0 +1,37 @@ > +/* > + * backtrace support (this is a modified lib/x86/stack.c) > + * > + * This work is licensed under the terms of the GNU LGPL, version 2. > + */ > +#include <libcflat.h> > +#include <stack.h> > + > +extern char vector_stub_start, vector_stub_end; These aren't used until the next patch. > + > +int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) { '{' should be on its own line. I usually try to run the kernel's checkpatch since we use the same style (except we're even more forgiving for long lines). > + const void *fp = frame; > + void *lr; > + int depth; > + > + /* > + * ARM64 stack grows down. fp points to the previous fp on the stack, > + * and lr is just above it > + */ > + for (depth = 0; fp && depth < max_depth; ++depth) { > + > + asm volatile ("ldp %0, %1, [%2]" > + : "=r" (fp), "=r" (lr) > + : "r" (fp) > + : ); > + > + return_addrs[depth] = lr; > + } > + > + return depth; > +} > + > +int backtrace(const void **return_addrs, int max_depth) > +{ > + return backtrace_frame(__builtin_frame_address(0), > + return_addrs, max_depth); > +} > -- > 2.34.1 > Thanks, drew
> On Jun 24, 2023, at 3:13 AM, Andrew Jones <andrew.jones@linux.dev> wrote: > >> +extern char vector_stub_start, vector_stub_end; > > These aren't used until the next patch. > >> + >> +int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) { > > '{' should be on its own line. I usually try to run the kernel's > checkpatch since we use the same style (except we're even more forgiving > for long lines). I usually do use checkpatch. I guess I got sloppy. I will fix these 2 issues. BTW: I used the get_maintainer script to get who to send the patches to and it included the depreciated kvmarm@lists.cs.columbia.edu <mailto:kvmarm@lists.cs.columbia.edu> .. Ugh.
On Sun, Jun 25, 2023 at 12:22:15PM -0700, Nadav Amit wrote: > > > > On Jun 24, 2023, at 3:13 AM, Andrew Jones <andrew.jones@linux.dev> wrote: > > > >> +extern char vector_stub_start, vector_stub_end; > > > > These aren't used until the next patch. > > > >> + > >> +int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) { > > > > '{' should be on its own line. I usually try to run the kernel's > > checkpatch since we use the same style (except we're even more forgiving > > for long lines). > > I usually do use checkpatch. I guess I got sloppy. I will fix these 2 issues. > > BTW: I used the get_maintainer script to get who to send the patches to and it > included the depreciated kvmarm@lists.cs.columbia.edu <mailto:kvmarm@lists.cs.columbia.edu> .. Ugh. Ah, thanks for pointing that out. It's safe to declare the depreciation peroid over. I just pushed a patch dropping it now. drew
diff --git a/arm/Makefile.arm b/arm/Makefile.arm index 2ce00f5..7fd39f3 100644 --- a/arm/Makefile.arm +++ b/arm/Makefile.arm @@ -11,9 +11,6 @@ ifeq ($(CONFIG_EFI),y) $(error Cannot build arm32 tests as EFI apps) endif -# stack.o relies on frame pointers. -KEEP_FRAME_POINTER := y - CFLAGS += $(machine) CFLAGS += -mcpu=$(PROCESSOR) CFLAGS += -mno-unaligned-access diff --git a/arm/Makefile.arm64 b/arm/Makefile.arm64 index eada7f9..60385e2 100644 --- a/arm/Makefile.arm64 +++ b/arm/Makefile.arm64 @@ -21,6 +21,7 @@ define arch_elf_check = endef cstart.o = $(TEST_DIR)/cstart64.o +cflatobjs += lib/arm64/stack.o cflatobjs += lib/arm64/processor.o cflatobjs += lib/arm64/spinlock.o cflatobjs += lib/arm64/gic-v3-its.o lib/arm64/gic-v3-its-cmd.o diff --git a/arm/Makefile.common b/arm/Makefile.common index f904702..7fecfb3 100644 --- a/arm/Makefile.common +++ b/arm/Makefile.common @@ -22,6 +22,9 @@ $(TEST_DIR)/sieve.elf: AUXFLAGS = 0x1 ################################################################## AUXFLAGS ?= 0x0 +# stack.o relies on frame pointers. +KEEP_FRAME_POINTER := y + CFLAGS += -std=gnu99 CFLAGS += -ffreestanding CFLAGS += -O2 diff --git a/lib/arm64/asm/stack.h b/lib/arm64/asm/stack.h index d000624..be486cf 100644 --- a/lib/arm64/asm/stack.h +++ b/lib/arm64/asm/stack.h @@ -5,4 +5,7 @@ #error Do not directly include <asm/stack.h>. Just use <stack.h>. #endif +#define HAVE_ARCH_BACKTRACE_FRAME +#define HAVE_ARCH_BACKTRACE + #endif diff --git a/lib/arm64/stack.c b/lib/arm64/stack.c new file mode 100644 index 0000000..1e2568a --- /dev/null +++ b/lib/arm64/stack.c @@ -0,0 +1,37 @@ +/* + * backtrace support (this is a modified lib/x86/stack.c) + * + * This work is licensed under the terms of the GNU LGPL, version 2. + */ +#include <libcflat.h> +#include <stack.h> + +extern char vector_stub_start, vector_stub_end; + +int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) { + const void *fp = frame; + void *lr; + int depth; + + /* + * ARM64 stack grows down. fp points to the previous fp on the stack, + * and lr is just above it + */ + for (depth = 0; fp && depth < max_depth; ++depth) { + + asm volatile ("ldp %0, %1, [%2]" + : "=r" (fp), "=r" (lr) + : "r" (fp) + : ); + + return_addrs[depth] = lr; + } + + return depth; +} + +int backtrace(const void **return_addrs, int max_depth) +{ + return backtrace_frame(__builtin_frame_address(0), + return_addrs, max_depth); +}