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

[Andrew Jones <drjones@redhat.com>]

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