| Message ID | 1456967378-6367-6-git-send-email-pfeiner@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 02, 2016 at 05:09:35PM -0800, Peter Feiner wrote: > Functions to walk stack and print backtrace. The stack's unadorned as > > STACK: addr addr addr ... > > A follow-up patch post-processes the output to pretty-print the stack. > > Stack walker is just a stub on arm and ppc. > > Signed-off-by: Peter Feiner <pfeiner@google.com> > --- > Makefile | 3 ++- > arm/Makefile.common | 1 + > lib/arm/dump_stack.c | 6 ++++++ > lib/libcflat.h | 4 ++++ > lib/powerpc/dump_stack.c | 6 ++++++ > lib/printf.c | 37 +++++++++++++++++++++++++++++++++++++ > lib/x86/dump_stack.c | 24 ++++++++++++++++++++++++ > powerpc/Makefile.common | 1 + > x86/Makefile.common | 4 ++++ > 9 files changed, 85 insertions(+), 1 deletion(-) > create mode 100644 lib/arm/dump_stack.c > create mode 100644 lib/powerpc/dump_stack.c > create mode 100644 lib/x86/dump_stack.c > > diff --git a/Makefile b/Makefile > index ddba941..40ea4ec 100644 > --- a/Makefile > +++ b/Makefile > @@ -42,7 +42,8 @@ cc-option = $(shell if $(CC) $(1) -S -o /dev/null -xc /dev/null \ > > CFLAGS += -g > CFLAGS += $(autodepend-flags) -Wall > -CFLAGS += $(call cc-option, -fomit-frame-pointer, "") > +frame-pointer-flag=-f$(if $(KEEP_FRAME_POINTER),no-,)omit-frame-pointer > +CFLAGS += $(call cc-option, $(frame-pointer-flag), "") > CFLAGS += $(call cc-option, -fno-stack-protector, "") > CFLAGS += $(call cc-option, -fno-stack-protector-all, "") > > diff --git a/arm/Makefile.common b/arm/Makefile.common > index dd3a0ca..054bdee 100644 > --- a/arm/Makefile.common > +++ b/arm/Makefile.common > @@ -39,6 +39,7 @@ cflatobjs += lib/arm/mmu.o > cflatobjs += lib/arm/bitops.o > cflatobjs += lib/arm/psci.o > cflatobjs += lib/arm/smp.o > +cflatobjs += lib/arm/dump_stack.o > > libeabi = lib/arm/libeabi.a > eabiobjs = lib/arm/eabi_compat.o > diff --git a/lib/arm/dump_stack.c b/lib/arm/dump_stack.c > new file mode 100644 > index 0000000..528ba63 > --- /dev/null > +++ b/lib/arm/dump_stack.c > @@ -0,0 +1,6 @@ > +#include "libcflat.h" > + > +int walk_stack(unsigned long bp, unsigned long *stack, int max_depth) > +{ > + return 0; > +} > diff --git a/lib/libcflat.h b/lib/libcflat.h > index 1f0049c..42c94df 100644 > --- a/lib/libcflat.h > +++ b/lib/libcflat.h > @@ -65,6 +65,10 @@ extern void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...); > extern void report_abort(const char *msg_fmt, ...); > extern int report_summary(void); > > +int walk_stack(unsigned long bp, unsigned long *stack, int max_depth); > +void dump_stack(unsigned long ip, unsigned long bp); > +void dump_current_stack(void); The inputs are a bit x86ish. On ARM, for example, we call them pc and fp. Acutally, how about we implement backtrace(3) instead? And then wrap print_current_stack(void) around that. Also, instead of building on a single arch function, walk_stack, how about two void *arch_frame_address(unsigned int level); void *arch_return_address(void *frame, unsigned int level); The architecture may be able to easily implement these with gcc builtins, or not. I've added *frame to arch_return_address' parameters in case __builtin_return_address(level) doesn't work. And, let's just require the architectures to have the functions, not that they have a dump_stack.c file. If we include lib/asm/<ARCH>/stack.h, or whatever you want to call it, then the arch could just supply some inlines there. > + > #define ARRAY_SIZE(_a) (sizeof(_a)/sizeof((_a)[0])) > > #define container_of(ptr, type, member) ({ \ > diff --git a/lib/powerpc/dump_stack.c b/lib/powerpc/dump_stack.c > new file mode 100644 > index 0000000..528ba63 > --- /dev/null > +++ b/lib/powerpc/dump_stack.c > @@ -0,0 +1,6 @@ > +#include "libcflat.h" > + > +int walk_stack(unsigned long bp, unsigned long *stack, int max_depth) > +{ > + return 0; > +} > diff --git a/lib/printf.c b/lib/printf.c > index 2aec59a..e97fca9 100644 > --- a/lib/printf.c > +++ b/lib/printf.c > @@ -259,3 +259,40 @@ int printf(const char *fmt, ...) > puts(buf); > return r; > } > + > +static void print_stack(unsigned long *stack, int depth, > + bool top_is_return_address) > +{ > + int i; > + > + printf("\tSTACK: " ); > + for (i = 0; i < depth; i++) { > + int offset = -1; > + if (i == 0 && !top_is_return_address) > + offset = 0; > + printf(" %lx", stack[i] + offset); I'm afraid I don't understand the use of offset here. I fear it may be x86-ness leaked into the arch-neutral print_stack(). > + } > + printf("\n"); > +} > + > +#define MAX_DEPTH 10 > + > +void dump_stack(unsigned long ip, unsigned long bp) > +{ > + unsigned long stack[MAX_DEPTH]; > + int depth; > + > + stack[0] = ip; > + depth = walk_stack(bp, &stack[1], MAX_DEPTH - 1); > + print_stack(stack, depth + 1, false); > +} When would dump_stack() be useful, as opposed to dump_current_stack? > + > +void dump_current_stack(void) > +{ > + unsigned long stack[MAX_DEPTH]; > + int depth; > + > + depth = walk_stack((unsigned long)__builtin_frame_address(1), stack, > + MAX_DEPTH); > + print_stack(stack, depth, true); > +} > diff --git a/lib/x86/dump_stack.c b/lib/x86/dump_stack.c > new file mode 100644 > index 0000000..6e9d126 > --- /dev/null > +++ b/lib/x86/dump_stack.c > @@ -0,0 +1,24 @@ > +#include "libcflat.h" > + > +int walk_stack(unsigned long bp, unsigned long *stack, int max_depth) > +{ > + static int walking; > + int depth = 0; > + unsigned long *frame = (unsigned long *) bp; > + > + if (walking) { > + printf("RECURSIVE STACK WALK!!!\n"); > + return 0; > + } > + walking = 1; > + > + for (depth = 0; depth < max_depth; depth++) { > + stack[depth] = frame[1]; > + if (stack[depth] == 0) > + break; > + frame = (unsigned long *) frame[0]; > + } > + > + walking = 0; > + return depth; > +} > diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common > index 2ce6494..694dea0 100644 > --- a/powerpc/Makefile.common > +++ b/powerpc/Makefile.common > @@ -30,6 +30,7 @@ cflatobjs += lib/powerpc/io.o > cflatobjs += lib/powerpc/hcall.o > cflatobjs += lib/powerpc/setup.o > cflatobjs += lib/powerpc/rtas.o > +cflatobjs += lib/powerpc/dump_stack.o > > FLATLIBS = $(libcflat) $(LIBFDT_archive) > %.elf: CFLAGS += $(arch_CFLAGS) > diff --git a/x86/Makefile.common b/x86/Makefile.common > index 3a14fea..d7c7eab 100644 > --- a/x86/Makefile.common > +++ b/x86/Makefile.common > @@ -12,6 +12,7 @@ cflatobjs += lib/x86/atomic.o > cflatobjs += lib/x86/desc.o > cflatobjs += lib/x86/isr.o > cflatobjs += lib/x86/acpi.o > +cflatobjs += lib/x86/dump_stack.o > > $(libcflat): LDFLAGS += -nostdlib > $(libcflat): CFLAGS += -ffreestanding -I lib > @@ -19,6 +20,9 @@ $(libcflat): CFLAGS += -ffreestanding -I lib > CFLAGS += -m$(bits) > CFLAGS += -O1 > > +# dump_stack.o relies on frame pointers. > +KEEP_FRAME_POINTER := y > + > libgcc := $(shell $(CC) -m$(bits) --print-libgcc-file-name) > > FLATLIBS = lib/libcflat.a $(libgcc) > -- > 2.7.0.rc3.207.g0ac5344 Thanks, drew -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 3, 2016 at 1:17 AM, Andrew Jones <drjones@redhat.com> wrote: > On Wed, Mar 02, 2016 at 05:09:35PM -0800, Peter Feiner wrote: >> Functions to walk stack and print backtrace. The stack's unadorned as >> >> STACK: addr addr addr ... >> >> A follow-up patch post-processes the output to pretty-print the stack. >> >> Stack walker is just a stub on arm and ppc. >> >> Signed-off-by: Peter Feiner <pfeiner@google.com> >> --- >> Makefile | 3 ++- >> arm/Makefile.common | 1 + >> lib/arm/dump_stack.c | 6 ++++++ >> lib/libcflat.h | 4 ++++ >> lib/powerpc/dump_stack.c | 6 ++++++ >> lib/printf.c | 37 +++++++++++++++++++++++++++++++++++++ >> lib/x86/dump_stack.c | 24 ++++++++++++++++++++++++ >> powerpc/Makefile.common | 1 + >> x86/Makefile.common | 4 ++++ >> 9 files changed, 85 insertions(+), 1 deletion(-) >> create mode 100644 lib/arm/dump_stack.c >> create mode 100644 lib/powerpc/dump_stack.c >> create mode 100644 lib/x86/dump_stack.c >> >> diff --git a/Makefile b/Makefile >> index ddba941..40ea4ec 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -42,7 +42,8 @@ cc-option = $(shell if $(CC) $(1) -S -o /dev/null -xc /dev/null \ >> >> CFLAGS += -g >> CFLAGS += $(autodepend-flags) -Wall >> -CFLAGS += $(call cc-option, -fomit-frame-pointer, "") >> +frame-pointer-flag=-f$(if $(KEEP_FRAME_POINTER),no-,)omit-frame-pointer >> +CFLAGS += $(call cc-option, $(frame-pointer-flag), "") >> CFLAGS += $(call cc-option, -fno-stack-protector, "") >> CFLAGS += $(call cc-option, -fno-stack-protector-all, "") >> >> diff --git a/arm/Makefile.common b/arm/Makefile.common >> index dd3a0ca..054bdee 100644 >> --- a/arm/Makefile.common >> +++ b/arm/Makefile.common >> @@ -39,6 +39,7 @@ cflatobjs += lib/arm/mmu.o >> cflatobjs += lib/arm/bitops.o >> cflatobjs += lib/arm/psci.o >> cflatobjs += lib/arm/smp.o >> +cflatobjs += lib/arm/dump_stack.o >> >> libeabi = lib/arm/libeabi.a >> eabiobjs = lib/arm/eabi_compat.o >> diff --git a/lib/arm/dump_stack.c b/lib/arm/dump_stack.c >> new file mode 100644 >> index 0000000..528ba63 >> --- /dev/null >> +++ b/lib/arm/dump_stack.c >> @@ -0,0 +1,6 @@ >> +#include "libcflat.h" >> + >> +int walk_stack(unsigned long bp, unsigned long *stack, int max_depth) >> +{ >> + return 0; >> +} >> diff --git a/lib/libcflat.h b/lib/libcflat.h >> index 1f0049c..42c94df 100644 >> --- a/lib/libcflat.h >> +++ b/lib/libcflat.h >> @@ -65,6 +65,10 @@ extern void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...); >> extern void report_abort(const char *msg_fmt, ...); >> extern int report_summary(void); >> >> +int walk_stack(unsigned long bp, unsigned long *stack, int max_depth); >> +void dump_stack(unsigned long ip, unsigned long bp); >> +void dump_current_stack(void); > > The inputs are a bit x86ish. On ARM, for example, we call them pc and > fp. Acutally, how about we implement backtrace(3) instead? And then wrap > print_current_stack(void) around that. I'm indifferent w.r.t. input names. I can change them to void *instruction and void *frame. I didn't go with backtrace() because I wanted to give the frame pointer as an argument. The frame pointer argument is essential for dumping stacks of unhandled exceptions. See the 6th patch in the series, " x86: lib: debug dump on unhandled exceptions". > Also, instead of building on a single arch function, walk_stack, how about > two > > void *arch_frame_address(unsigned int level); > void *arch_return_address(void *frame, unsigned int level); This would entail O(level^2) frame pointer traversals to print a stack trace. The extra CPU overhead doesn't bother me *too much*, but the inevitable debugging burden does. Every once and a while, I have to stick a printf() in walk_stack to see where a frame pointer is causing a #PF :-) So I'd like to stick with a function that returns an array of pointers. I can call it int arch_return_addresses(const void *frame, void **addresses, int max_depth); (is the arch_ prefix necessary given that there's no non-arch_ variant?) Here's the best of both worlds: /* return addresses starting from the given frame. needs arch-specific code. */ int backtrace_frame(const void *frame, void **addresses, int max_depth); /* return addresses from the current frame. Can use repeated calls to __builtin_return_address. */ int backtrace(void **addresses, int max_depth); I'll use backtrace() in dump_current_stack() and backtrace_frame in dump_stack(). I'll also rename dump_current_stack to dump_stack and dump_stack to dump_frame_stack for some consistency. Wow, I should've just written the patch instead of writing all of this crap out :-) > The architecture may be able to easily implement these with gcc builtins, > or not. I've added *frame to arch_return_address' parameters in case > __builtin_return_address(level) doesn't work. > > And, let's just require the architectures to have the functions, not that > they have a dump_stack.c file. If we include lib/asm/<ARCH>/stack.h, or > whatever you want to call it, then the arch could just supply some inlines > there. That's a good suggestion. I'll change things. >> + >> #define ARRAY_SIZE(_a) (sizeof(_a)/sizeof((_a)[0])) >> >> #define container_of(ptr, type, member) ({ \ >> diff --git a/lib/powerpc/dump_stack.c b/lib/powerpc/dump_stack.c >> new file mode 100644 >> index 0000000..528ba63 >> --- /dev/null >> +++ b/lib/powerpc/dump_stack.c >> @@ -0,0 +1,6 @@ >> +#include "libcflat.h" >> + >> +int walk_stack(unsigned long bp, unsigned long *stack, int max_depth) >> +{ >> + return 0; >> +} >> diff --git a/lib/printf.c b/lib/printf.c >> index 2aec59a..e97fca9 100644 >> --- a/lib/printf.c >> +++ b/lib/printf.c >> @@ -259,3 +259,40 @@ int printf(const char *fmt, ...) >> puts(buf); >> return r; >> } >> + >> +static void print_stack(unsigned long *stack, int depth, >> + bool top_is_return_address) >> +{ >> + int i; >> + >> + printf("\tSTACK: " ); >> + for (i = 0; i < depth; i++) { >> + int offset = -1; >> + if (i == 0 && !top_is_return_address) >> + offset = 0; >> + printf(" %lx", stack[i] + offset); > > I'm afraid I don't understand the use of offset here. I fear it may > be x86-ness leaked into the arch-neutral print_stack(). It's not x86-ness, but it's certainly not self explanatory :-) Return addresses are stored on the stack. Thus walk_stack returns an array of pointers to instructions that we'll *return to* when the stack unwinds. If you give those return addresses to addr2line, you'll get the line numbers of the code following function calls. For example, 1: void foo(void) { 2: dump_current_stack(); 3: return; 4: } 5: int main(void) { 6: foo(); 7: return 0; 8: } addr2line would print lines 3 & 7, i.e., the return addresses! With offset = -1, the addresses of some code within call instructions (or some other code generated to effect the function calls) are printed, and add2line prints lines 6 & 2. The special case of offset = 0 is for when the first address isn't the return address but the address of the faulting instruction, which is the case for unhandled exceptions. I.e., iret re-tries the interrupted / faulting instruction. I suppose I could've just done stack[0] = ip + 1 in dump_stack then unconditionally subtracted 1 in print_stack and gotten rid of the top_is_return_address argument. Yes, I'll do that since it'll be cleaner on all accounts. I'll also change the name of the argument from "stack" to "return_addrs". One disadvantage of all of this address twiddling is that the stack will look like nonsense for people not using the pretty printer because the'll see addresses in the middle of call instructions. I could do the subtraction in the pretty printer, but then I'd have to annotate the stack[0] = ip case somehow. Perhaps something like: STACK: @ip ret ret ret That's pretty simple and most useful everybody. Let me know what you think Drew. > >> + } >> + printf("\n"); >> +} >> + >> +#define MAX_DEPTH 10 >> + >> +void dump_stack(unsigned long ip, unsigned long bp) >> +{ >> + unsigned long stack[MAX_DEPTH]; >> + int depth; >> + >> + stack[0] = ip; >> + depth = walk_stack(bp, &stack[1], MAX_DEPTH - 1); >> + print_stack(stack, depth + 1, false); >> +} > > When would dump_stack() be useful, as opposed to dump_current_stack? Exception handler. > >> + >> +void dump_current_stack(void) >> +{ >> + unsigned long stack[MAX_DEPTH]; >> + int depth; >> + >> + depth = walk_stack((unsigned long)__builtin_frame_address(1), stack, >> + MAX_DEPTH); >> + print_stack(stack, depth, true); >> +} >> diff --git a/lib/x86/dump_stack.c b/lib/x86/dump_stack.c >> new file mode 100644 >> index 0000000..6e9d126 >> --- /dev/null >> +++ b/lib/x86/dump_stack.c >> @@ -0,0 +1,24 @@ >> +#include "libcflat.h" >> + >> +int walk_stack(unsigned long bp, unsigned long *stack, int max_depth) >> +{ >> + static int walking; >> + int depth = 0; >> + unsigned long *frame = (unsigned long *) bp; >> + >> + if (walking) { >> + printf("RECURSIVE STACK WALK!!!\n"); >> + return 0; >> + } >> + walking = 1; >> + >> + for (depth = 0; depth < max_depth; depth++) { >> + stack[depth] = frame[1]; >> + if (stack[depth] == 0) >> + break; >> + frame = (unsigned long *) frame[0]; >> + } >> + >> + walking = 0; >> + return depth; >> +} >> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common >> index 2ce6494..694dea0 100644 >> --- a/powerpc/Makefile.common >> +++ b/powerpc/Makefile.common >> @@ -30,6 +30,7 @@ cflatobjs += lib/powerpc/io.o >> cflatobjs += lib/powerpc/hcall.o >> cflatobjs += lib/powerpc/setup.o >> cflatobjs += lib/powerpc/rtas.o >> +cflatobjs += lib/powerpc/dump_stack.o >> >> FLATLIBS = $(libcflat) $(LIBFDT_archive) >> %.elf: CFLAGS += $(arch_CFLAGS) >> diff --git a/x86/Makefile.common b/x86/Makefile.common >> index 3a14fea..d7c7eab 100644 >> --- a/x86/Makefile.common >> +++ b/x86/Makefile.common >> @@ -12,6 +12,7 @@ cflatobjs += lib/x86/atomic.o >> cflatobjs += lib/x86/desc.o >> cflatobjs += lib/x86/isr.o >> cflatobjs += lib/x86/acpi.o >> +cflatobjs += lib/x86/dump_stack.o >> >> $(libcflat): LDFLAGS += -nostdlib >> $(libcflat): CFLAGS += -ffreestanding -I lib >> @@ -19,6 +20,9 @@ $(libcflat): CFLAGS += -ffreestanding -I lib >> CFLAGS += -m$(bits) >> CFLAGS += -O1 >> >> +# dump_stack.o relies on frame pointers. >> +KEEP_FRAME_POINTER := y >> + >> libgcc := $(shell $(CC) -m$(bits) --print-libgcc-file-name) >> >> FLATLIBS = lib/libcflat.a $(libgcc) >> -- >> 2.7.0.rc3.207.g0ac5344 > > Thanks, > drew Thanks for the review! -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 03, 2016 at 09:01:03AM -0800, Peter Feiner wrote: > On Thu, Mar 3, 2016 at 1:17 AM, Andrew Jones <drjones@redhat.com> wrote: > > On Wed, Mar 02, 2016 at 05:09:35PM -0800, Peter Feiner wrote: > >> Functions to walk stack and print backtrace. The stack's unadorned as > >> > >> STACK: addr addr addr ... > >> > >> A follow-up patch post-processes the output to pretty-print the stack. > >> > >> Stack walker is just a stub on arm and ppc. > >> > >> Signed-off-by: Peter Feiner <pfeiner@google.com> > >> --- > >> Makefile | 3 ++- > >> arm/Makefile.common | 1 + > >> lib/arm/dump_stack.c | 6 ++++++ > >> lib/libcflat.h | 4 ++++ > >> lib/powerpc/dump_stack.c | 6 ++++++ > >> lib/printf.c | 37 +++++++++++++++++++++++++++++++++++++ > >> lib/x86/dump_stack.c | 24 ++++++++++++++++++++++++ > >> powerpc/Makefile.common | 1 + > >> x86/Makefile.common | 4 ++++ > >> 9 files changed, 85 insertions(+), 1 deletion(-) > >> create mode 100644 lib/arm/dump_stack.c > >> create mode 100644 lib/powerpc/dump_stack.c > >> create mode 100644 lib/x86/dump_stack.c > >> > >> diff --git a/Makefile b/Makefile > >> index ddba941..40ea4ec 100644 > >> --- a/Makefile > >> +++ b/Makefile > >> @@ -42,7 +42,8 @@ cc-option = $(shell if $(CC) $(1) -S -o /dev/null -xc /dev/null \ > >> > >> CFLAGS += -g > >> CFLAGS += $(autodepend-flags) -Wall > >> -CFLAGS += $(call cc-option, -fomit-frame-pointer, "") > >> +frame-pointer-flag=-f$(if $(KEEP_FRAME_POINTER),no-,)omit-frame-pointer > >> +CFLAGS += $(call cc-option, $(frame-pointer-flag), "") > >> CFLAGS += $(call cc-option, -fno-stack-protector, "") > >> CFLAGS += $(call cc-option, -fno-stack-protector-all, "") > >> > >> diff --git a/arm/Makefile.common b/arm/Makefile.common > >> index dd3a0ca..054bdee 100644 > >> --- a/arm/Makefile.common > >> +++ b/arm/Makefile.common > >> @@ -39,6 +39,7 @@ cflatobjs += lib/arm/mmu.o > >> cflatobjs += lib/arm/bitops.o > >> cflatobjs += lib/arm/psci.o > >> cflatobjs += lib/arm/smp.o > >> +cflatobjs += lib/arm/dump_stack.o > >> > >> libeabi = lib/arm/libeabi.a > >> eabiobjs = lib/arm/eabi_compat.o > >> diff --git a/lib/arm/dump_stack.c b/lib/arm/dump_stack.c > >> new file mode 100644 > >> index 0000000..528ba63 > >> --- /dev/null > >> +++ b/lib/arm/dump_stack.c > >> @@ -0,0 +1,6 @@ > >> +#include "libcflat.h" > >> + > >> +int walk_stack(unsigned long bp, unsigned long *stack, int max_depth) > >> +{ > >> + return 0; > >> +} > >> diff --git a/lib/libcflat.h b/lib/libcflat.h > >> index 1f0049c..42c94df 100644 > >> --- a/lib/libcflat.h > >> +++ b/lib/libcflat.h > >> @@ -65,6 +65,10 @@ extern void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...); > >> extern void report_abort(const char *msg_fmt, ...); > >> extern int report_summary(void); > >> > >> +int walk_stack(unsigned long bp, unsigned long *stack, int max_depth); > >> +void dump_stack(unsigned long ip, unsigned long bp); > >> +void dump_current_stack(void); > > > > The inputs are a bit x86ish. On ARM, for example, we call them pc and > > fp. Acutally, how about we implement backtrace(3) instead? And then wrap > > print_current_stack(void) around that. > > I'm indifferent w.r.t. input names. I can change them to void > *instruction and void *frame. > > I didn't go with backtrace() because I wanted to give the frame > pointer as an argument. The frame pointer argument is essential for > dumping stacks of unhandled exceptions. See the 6th patch in the > series, " > x86: lib: debug dump on unhandled exceptions". > > > Also, instead of building on a single arch function, walk_stack, how about > > two > > > > void *arch_frame_address(unsigned int level); > > void *arch_return_address(void *frame, unsigned int level); > > This would entail O(level^2) frame pointer traversals to print a stack > trace. The extra CPU overhead doesn't bother me *too much*, but the > inevitable debugging burden does. Every once and a while, I have to > stick a printf() in walk_stack to see where a frame pointer is causing > a #PF :-) So I'd like to stick with a function that returns an array > of pointers. > > I can call it > > int arch_return_addresses(const void *frame, void **addresses, int max_depth); > > (is the arch_ prefix necessary given that there's no non-arch_ variant?) yeah, you're right, arch_ isn't really necessary here. > > Here's the best of both worlds: > > /* return addresses starting from the given frame. needs arch-specific code. */ > int backtrace_frame(const void *frame, void **addresses, int max_depth); > > /* return addresses from the current frame. Can use repeated calls to > __builtin_return_address. */ > int backtrace(void **addresses, int max_depth); > > I'll use backtrace() in dump_current_stack() and backtrace_frame in > dump_stack(). I'll also rename dump_current_stack to dump_stack and > dump_stack to dump_frame_stack for some consistency. > > Wow, I should've just written the patch instead of writing all of this > crap out :-) :-) > > > The architecture may be able to easily implement these with gcc builtins, > > or not. I've added *frame to arch_return_address' parameters in case > > __builtin_return_address(level) doesn't work. > > > > And, let's just require the architectures to have the functions, not that > > they have a dump_stack.c file. If we include lib/asm/<ARCH>/stack.h, or > > whatever you want to call it, then the arch could just supply some inlines > > there. > > That's a good suggestion. I'll change things. > > >> + > >> #define ARRAY_SIZE(_a) (sizeof(_a)/sizeof((_a)[0])) > >> > >> #define container_of(ptr, type, member) ({ \ > >> diff --git a/lib/powerpc/dump_stack.c b/lib/powerpc/dump_stack.c > >> new file mode 100644 > >> index 0000000..528ba63 > >> --- /dev/null > >> +++ b/lib/powerpc/dump_stack.c > >> @@ -0,0 +1,6 @@ > >> +#include "libcflat.h" > >> + > >> +int walk_stack(unsigned long bp, unsigned long *stack, int max_depth) > >> +{ > >> + return 0; > >> +} > >> diff --git a/lib/printf.c b/lib/printf.c > >> index 2aec59a..e97fca9 100644 > >> --- a/lib/printf.c > >> +++ b/lib/printf.c > >> @@ -259,3 +259,40 @@ int printf(const char *fmt, ...) > >> puts(buf); > >> return r; > >> } > >> + > >> +static void print_stack(unsigned long *stack, int depth, > >> + bool top_is_return_address) > >> +{ > >> + int i; > >> + > >> + printf("\tSTACK: " ); > >> + for (i = 0; i < depth; i++) { > >> + int offset = -1; > >> + if (i == 0 && !top_is_return_address) > >> + offset = 0; > >> + printf(" %lx", stack[i] + offset); > > > > I'm afraid I don't understand the use of offset here. I fear it may > > be x86-ness leaked into the arch-neutral print_stack(). > > It's not x86-ness, but it's certainly not self explanatory :-) Return > addresses are stored on the stack. Thus walk_stack returns an array of > pointers to instructions that we'll *return to* when the stack > unwinds. If you give those return addresses to addr2line, you'll get > the line numbers of the code following function calls. For example, > > 1: void foo(void) { > 2: dump_current_stack(); > 3: return; > 4: } > 5: int main(void) { > 6: foo(); > 7: return 0; > 8: } > > addr2line would print lines 3 & 7, i.e., the return addresses! With > offset = -1, the addresses of some code within call instructions (or > some other code generated to effect the function calls) are printed, > and add2line prints lines 6 & 2. > > The special case of offset = 0 is for when the first address isn't the > return address but the address of the faulting instruction, which is > the case for unhandled exceptions. I.e., iret re-tries the interrupted > / faulting instruction. I suppose I could've just done stack[0] = ip + > 1 in dump_stack then unconditionally subtracted 1 in print_stack and > gotten rid of the top_is_return_address argument. Yes, I'll do that > since it'll be cleaner on all accounts. I'll also change the name of > the argument from "stack" to "return_addrs". > > One disadvantage of all of this address twiddling is that the stack > will look like nonsense for people not using the pretty printer > because the'll see addresses in the middle of call instructions. I > could do the subtraction in the pretty printer, but then I'd have to > annotate the stack[0] = ip case somehow. Perhaps something like: > > STACK: @ip ret ret ret > > That's pretty simple and most useful everybody. Let me know what you think Drew. Sounds good to me. Thanks! > > > > >> + } > >> + printf("\n"); > >> +} > >> + > >> +#define MAX_DEPTH 10 > >> + > >> +void dump_stack(unsigned long ip, unsigned long bp) > >> +{ > >> + unsigned long stack[MAX_DEPTH]; > >> + int depth; > >> + > >> + stack[0] = ip; > >> + depth = walk_stack(bp, &stack[1], MAX_DEPTH - 1); > >> + print_stack(stack, depth + 1, false); > >> +} > > > > When would dump_stack() be useful, as opposed to dump_current_stack? > > Exception handler. > > > > >> + > >> +void dump_current_stack(void) > >> +{ > >> + unsigned long stack[MAX_DEPTH]; > >> + int depth; > >> + > >> + depth = walk_stack((unsigned long)__builtin_frame_address(1), stack, > >> + MAX_DEPTH); > >> + print_stack(stack, depth, true); > >> +} > >> diff --git a/lib/x86/dump_stack.c b/lib/x86/dump_stack.c > >> new file mode 100644 > >> index 0000000..6e9d126 > >> --- /dev/null > >> +++ b/lib/x86/dump_stack.c > >> @@ -0,0 +1,24 @@ > >> +#include "libcflat.h" > >> + > >> +int walk_stack(unsigned long bp, unsigned long *stack, int max_depth) > >> +{ > >> + static int walking; > >> + int depth = 0; > >> + unsigned long *frame = (unsigned long *) bp; > >> + > >> + if (walking) { > >> + printf("RECURSIVE STACK WALK!!!\n"); > >> + return 0; > >> + } > >> + walking = 1; > >> + > >> + for (depth = 0; depth < max_depth; depth++) { > >> + stack[depth] = frame[1]; > >> + if (stack[depth] == 0) > >> + break; > >> + frame = (unsigned long *) frame[0]; > >> + } > >> + > >> + walking = 0; > >> + return depth; > >> +} > >> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common > >> index 2ce6494..694dea0 100644 > >> --- a/powerpc/Makefile.common > >> +++ b/powerpc/Makefile.common > >> @@ -30,6 +30,7 @@ cflatobjs += lib/powerpc/io.o > >> cflatobjs += lib/powerpc/hcall.o > >> cflatobjs += lib/powerpc/setup.o > >> cflatobjs += lib/powerpc/rtas.o > >> +cflatobjs += lib/powerpc/dump_stack.o > >> > >> FLATLIBS = $(libcflat) $(LIBFDT_archive) > >> %.elf: CFLAGS += $(arch_CFLAGS) > >> diff --git a/x86/Makefile.common b/x86/Makefile.common > >> index 3a14fea..d7c7eab 100644 > >> --- a/x86/Makefile.common > >> +++ b/x86/Makefile.common > >> @@ -12,6 +12,7 @@ cflatobjs += lib/x86/atomic.o > >> cflatobjs += lib/x86/desc.o > >> cflatobjs += lib/x86/isr.o > >> cflatobjs += lib/x86/acpi.o > >> +cflatobjs += lib/x86/dump_stack.o > >> > >> $(libcflat): LDFLAGS += -nostdlib > >> $(libcflat): CFLAGS += -ffreestanding -I lib > >> @@ -19,6 +20,9 @@ $(libcflat): CFLAGS += -ffreestanding -I lib > >> CFLAGS += -m$(bits) > >> CFLAGS += -O1 > >> > >> +# dump_stack.o relies on frame pointers. > >> +KEEP_FRAME_POINTER := y > >> + > >> libgcc := $(shell $(CC) -m$(bits) --print-libgcc-file-name) > >> > >> FLATLIBS = lib/libcflat.a $(libgcc) > >> -- > >> 2.7.0.rc3.207.g0ac5344 > > > > Thanks, > > drew > > Thanks for the review! > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Makefile b/Makefile index ddba941..40ea4ec 100644 --- a/Makefile +++ b/Makefile @@ -42,7 +42,8 @@ cc-option = $(shell if $(CC) $(1) -S -o /dev/null -xc /dev/null \ CFLAGS += -g CFLAGS += $(autodepend-flags) -Wall -CFLAGS += $(call cc-option, -fomit-frame-pointer, "") +frame-pointer-flag=-f$(if $(KEEP_FRAME_POINTER),no-,)omit-frame-pointer +CFLAGS += $(call cc-option, $(frame-pointer-flag), "") CFLAGS += $(call cc-option, -fno-stack-protector, "") CFLAGS += $(call cc-option, -fno-stack-protector-all, "") diff --git a/arm/Makefile.common b/arm/Makefile.common index dd3a0ca..054bdee 100644 --- a/arm/Makefile.common +++ b/arm/Makefile.common @@ -39,6 +39,7 @@ cflatobjs += lib/arm/mmu.o cflatobjs += lib/arm/bitops.o cflatobjs += lib/arm/psci.o cflatobjs += lib/arm/smp.o +cflatobjs += lib/arm/dump_stack.o libeabi = lib/arm/libeabi.a eabiobjs = lib/arm/eabi_compat.o diff --git a/lib/arm/dump_stack.c b/lib/arm/dump_stack.c new file mode 100644 index 0000000..528ba63 --- /dev/null +++ b/lib/arm/dump_stack.c @@ -0,0 +1,6 @@ +#include "libcflat.h" + +int walk_stack(unsigned long bp, unsigned long *stack, int max_depth) +{ + return 0; +} diff --git a/lib/libcflat.h b/lib/libcflat.h index 1f0049c..42c94df 100644 --- a/lib/libcflat.h +++ b/lib/libcflat.h @@ -65,6 +65,10 @@ extern void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...); extern void report_abort(const char *msg_fmt, ...); extern int report_summary(void); +int walk_stack(unsigned long bp, unsigned long *stack, int max_depth); +void dump_stack(unsigned long ip, unsigned long bp); +void dump_current_stack(void); + #define ARRAY_SIZE(_a) (sizeof(_a)/sizeof((_a)[0])) #define container_of(ptr, type, member) ({ \ diff --git a/lib/powerpc/dump_stack.c b/lib/powerpc/dump_stack.c new file mode 100644 index 0000000..528ba63 --- /dev/null +++ b/lib/powerpc/dump_stack.c @@ -0,0 +1,6 @@ +#include "libcflat.h" + +int walk_stack(unsigned long bp, unsigned long *stack, int max_depth) +{ + return 0; +} diff --git a/lib/printf.c b/lib/printf.c index 2aec59a..e97fca9 100644 --- a/lib/printf.c +++ b/lib/printf.c @@ -259,3 +259,40 @@ int printf(const char *fmt, ...) puts(buf); return r; } + +static void print_stack(unsigned long *stack, int depth, + bool top_is_return_address) +{ + int i; + + printf("\tSTACK: " ); + for (i = 0; i < depth; i++) { + int offset = -1; + if (i == 0 && !top_is_return_address) + offset = 0; + printf(" %lx", stack[i] + offset); + } + printf("\n"); +} + +#define MAX_DEPTH 10 + +void dump_stack(unsigned long ip, unsigned long bp) +{ + unsigned long stack[MAX_DEPTH]; + int depth; + + stack[0] = ip; + depth = walk_stack(bp, &stack[1], MAX_DEPTH - 1); + print_stack(stack, depth + 1, false); +} + +void dump_current_stack(void) +{ + unsigned long stack[MAX_DEPTH]; + int depth; + + depth = walk_stack((unsigned long)__builtin_frame_address(1), stack, + MAX_DEPTH); + print_stack(stack, depth, true); +} diff --git a/lib/x86/dump_stack.c b/lib/x86/dump_stack.c new file mode 100644 index 0000000..6e9d126 --- /dev/null +++ b/lib/x86/dump_stack.c @@ -0,0 +1,24 @@ +#include "libcflat.h" + +int walk_stack(unsigned long bp, unsigned long *stack, int max_depth) +{ + static int walking; + int depth = 0; + unsigned long *frame = (unsigned long *) bp; + + if (walking) { + printf("RECURSIVE STACK WALK!!!\n"); + return 0; + } + walking = 1; + + for (depth = 0; depth < max_depth; depth++) { + stack[depth] = frame[1]; + if (stack[depth] == 0) + break; + frame = (unsigned long *) frame[0]; + } + + walking = 0; + return depth; +} diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common index 2ce6494..694dea0 100644 --- a/powerpc/Makefile.common +++ b/powerpc/Makefile.common @@ -30,6 +30,7 @@ cflatobjs += lib/powerpc/io.o cflatobjs += lib/powerpc/hcall.o cflatobjs += lib/powerpc/setup.o cflatobjs += lib/powerpc/rtas.o +cflatobjs += lib/powerpc/dump_stack.o FLATLIBS = $(libcflat) $(LIBFDT_archive) %.elf: CFLAGS += $(arch_CFLAGS) diff --git a/x86/Makefile.common b/x86/Makefile.common index 3a14fea..d7c7eab 100644 --- a/x86/Makefile.common +++ b/x86/Makefile.common @@ -12,6 +12,7 @@ cflatobjs += lib/x86/atomic.o cflatobjs += lib/x86/desc.o cflatobjs += lib/x86/isr.o cflatobjs += lib/x86/acpi.o +cflatobjs += lib/x86/dump_stack.o $(libcflat): LDFLAGS += -nostdlib $(libcflat): CFLAGS += -ffreestanding -I lib @@ -19,6 +20,9 @@ $(libcflat): CFLAGS += -ffreestanding -I lib CFLAGS += -m$(bits) CFLAGS += -O1 +# dump_stack.o relies on frame pointers. +KEEP_FRAME_POINTER := y + libgcc := $(shell $(CC) -m$(bits) --print-libgcc-file-name) FLATLIBS = lib/libcflat.a $(libgcc)
Functions to walk stack and print backtrace. The stack's unadorned as STACK: addr addr addr ... A follow-up patch post-processes the output to pretty-print the stack. Stack walker is just a stub on arm and ppc. Signed-off-by: Peter Feiner <pfeiner@google.com> --- Makefile | 3 ++- arm/Makefile.common | 1 + lib/arm/dump_stack.c | 6 ++++++ lib/libcflat.h | 4 ++++ lib/powerpc/dump_stack.c | 6 ++++++ lib/printf.c | 37 +++++++++++++++++++++++++++++++++++++ lib/x86/dump_stack.c | 24 ++++++++++++++++++++++++ powerpc/Makefile.common | 1 + x86/Makefile.common | 4 ++++ 9 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 lib/arm/dump_stack.c create mode 100644 lib/powerpc/dump_stack.c create mode 100644 lib/x86/dump_stack.c