Re: [kvm-unit-tests v2 5/8] lib: backtrace printing

[Andrew Jones <drjones@redhat.com>]

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