Re: [kvm-unit-tests v3 1/4] lib: backtrace printing

[Andrew Jones <drjones@redhat.com>]

On Thu, Mar 03, 2016 at 12:48:33PM -0800, Peter Feiner wrote:
> Functions to walk stack and print backtrace. The stack's unadorned as
> 
> 	STACK: [@]addr addr addr ...
> 
> where the optional @ indicates that addr isn't a return address.
> 
> A follow-up patch post-processes the output to pretty-print the stack.
> 
> Frame 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/asm/stack.h     |  1 +
>  lib/arm64/asm/stack.h   |  1 +
>  lib/asm-generic/stack.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/asm-generic/stack.h | 14 +++++++++++++
>  lib/libcflat.h          |  3 +++
>  lib/powerpc/asm/stack.h |  1 +
>  lib/ppc64/asm/stack.h   |  1 +
>  lib/printf.c            | 42 +++++++++++++++++++++++++++++++++++++++
>  lib/x86/asm/stack.c     | 25 +++++++++++++++++++++++
>  lib/x86/asm/stack.h     | 12 +++++++++++
>  powerpc/Makefile.common |  1 +
>  x86/Makefile.common     |  4 ++++
>  14 files changed, 161 insertions(+), 1 deletion(-)
>  create mode 100644 lib/arm/asm/stack.h
>  create mode 100644 lib/arm64/asm/stack.h
>  create mode 100644 lib/asm-generic/stack.c
>  create mode 100644 lib/asm-generic/stack.h
>  create mode 100644 lib/powerpc/asm/stack.h
>  create mode 100644 lib/ppc64/asm/stack.h
>  create mode 100644 lib/x86/asm/stack.c
>  create mode 100644 lib/x86/asm/stack.h

It's not usual to put C files in the asm directories, at least Linux
doesn't do that. So I'd prefer

lib/stack.c: the common backtrace()
lib/<ARCH>/stack.c: arch specific backtrace* functions

I'm tempted to suggest that print_stack() and the dump*stack
functions also go in lib/stack.c, instead of lib/printf.c, but
then we need some

#ifndef backtrace
#define backtrace backtrace
int backtrace(...)
{
}
#endif

ugliness to allow the common backtrace() to be overridden. Although,
it might still be better to move those functions out of printf,
as I don't feel they really fit there.

> 
> diff --git a/Makefile b/Makefile
> index 72e6711..536f2e6 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..ede8851 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/asm-generic/stack.o
>  
>  libeabi = lib/arm/libeabi.a
>  eabiobjs = lib/arm/eabi_compat.o
> diff --git a/lib/arm/asm/stack.h b/lib/arm/asm/stack.h
> new file mode 100644
> index 0000000..f101d03
> --- /dev/null
> +++ b/lib/arm/asm/stack.h
> @@ -0,0 +1 @@
> +#include <asm-generic/stack.h>
> diff --git a/lib/arm64/asm/stack.h b/lib/arm64/asm/stack.h
> new file mode 100644
> index 0000000..f101d03
> --- /dev/null
> +++ b/lib/arm64/asm/stack.h
> @@ -0,0 +1 @@
> +#include <asm-generic/stack.h>
> diff --git a/lib/asm-generic/stack.c b/lib/asm-generic/stack.c
> new file mode 100644
> index 0000000..93219cb
> --- /dev/null
> +++ b/lib/asm-generic/stack.c
> @@ -0,0 +1,53 @@
> +#include <libcflat.h>
> +#include <asm-generic/stack.h>

nit: including asm-generic/stack.h doesn't appear to be necessary.

Actually, if there was something needed from stack.h, then we should
probably get it from asm/stack.h, in case the arch wanted to override
it.

> +
> +int backtrace(const void **return_addrs, int max_depth)
> +{
> +	static int walking;
> +	int depth = 0;
> +	void *addr;
> +
> +	if (walking) {
> +		printf("RECURSIVE STACK WALK!!!\n");
> +		return 0;
> +	}
> +	walking = 1;
> +
> +	/* __builtin_return_address requires a compile-time constant argument */
> +#define GET_RETURN_ADDRESS(i)						\
> +	if (max_depth == i)						\
> +		goto done;						\
> +	addr = __builtin_return_address(i);				\
> +	if (!addr)							\
> +		goto done;						\
> +	return_addrs[i] = __builtin_extract_return_addr(addr);		\
> +	depth = i + 1;							\
> +
> +	GET_RETURN_ADDRESS(0)
> +	GET_RETURN_ADDRESS(1)
> +	GET_RETURN_ADDRESS(2)
> +	GET_RETURN_ADDRESS(3)
> +	GET_RETURN_ADDRESS(4)
> +	GET_RETURN_ADDRESS(5)
> +	GET_RETURN_ADDRESS(6)
> +	GET_RETURN_ADDRESS(7)
> +	GET_RETURN_ADDRESS(8)
> +	GET_RETURN_ADDRESS(9)
> +	GET_RETURN_ADDRESS(10)
> +	GET_RETURN_ADDRESS(11)
> +	GET_RETURN_ADDRESS(12)
> +	GET_RETURN_ADDRESS(13)
> +	GET_RETURN_ADDRESS(14)
> +	GET_RETURN_ADDRESS(15)
> +	GET_RETURN_ADDRESS(16)
> +	GET_RETURN_ADDRESS(17)
> +	GET_RETURN_ADDRESS(18)
> +	GET_RETURN_ADDRESS(19)
> +	GET_RETURN_ADDRESS(20)
> +
> +#undef GET_RETURN_ADDRESS
> +
> +done:
> +	walking = 0;
> +	return depth;
> +}
> diff --git a/lib/asm-generic/stack.h b/lib/asm-generic/stack.h
> new file mode 100644
> index 0000000..381fce4
> --- /dev/null
> +++ b/lib/asm-generic/stack.h
> @@ -0,0 +1,14 @@
> +#ifndef _ASMGENERIC_STACK_H_
> +#define _ASMGENERIC_STACK_H_
> +
> +static inline int
> +backtrace_frame(const void *frame __attribute__((unused)),
> +		const void **return_addrs __attribute__((unused)),
> +		int max_depth __attribute__((unused)))
> +{
> +	return 0;
> +}
> +
> +int backtrace(const void **return_addrs, int max_depth);
> +
> +#endif
> diff --git a/lib/libcflat.h b/lib/libcflat.h
> index b58a8a1..55bddca 100644
> --- a/lib/libcflat.h
> +++ b/lib/libcflat.h
> @@ -81,6 +81,9 @@ 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);
>  
> +extern void dump_stack(void);
> +extern void dump_frame_stack(const void *instruction, const void *frame);
> +
>  #define ARRAY_SIZE(_a) (sizeof(_a)/sizeof((_a)[0]))
>  
>  #define container_of(ptr, type, member) ({				\
> diff --git a/lib/powerpc/asm/stack.h b/lib/powerpc/asm/stack.h
> new file mode 100644
> index 0000000..f101d03
> --- /dev/null
> +++ b/lib/powerpc/asm/stack.h
> @@ -0,0 +1 @@
> +#include <asm-generic/stack.h>
> diff --git a/lib/ppc64/asm/stack.h b/lib/ppc64/asm/stack.h
> new file mode 100644
> index 0000000..f101d03
> --- /dev/null
> +++ b/lib/ppc64/asm/stack.h
> @@ -0,0 +1 @@
> +#include <asm-generic/stack.h>
> diff --git a/lib/printf.c b/lib/printf.c
> index 2aec59a..096e175 100644
> --- a/lib/printf.c
> +++ b/lib/printf.c
> @@ -1,4 +1,5 @@
>  #include "libcflat.h"
> +#include "asm/stack.h"
>  
>  #define BUFSZ 2000
>  
> @@ -259,3 +260,44 @@ int printf(const char *fmt, ...)
>      puts(buf);
>      return r;
>  }
> +
> +static void print_stack(const void **return_addrs, int depth,
> +			bool top_is_return_address)
> +{
> +	int i = 0;
> +
> +	printf("\tSTACK:");
> +
> +	/* @addr indicates a non-return address, as expected by the stack
> +	 * pretty printer script. */
> +	if (depth > 0 && !top_is_return_address) {
> +		printf(" @%lx", (unsigned long) return_addrs[0]);
> +		i++;
> +	}
> +
> +	for (; i < depth; i++) {
> +		printf(" %lx", (unsigned long) return_addrs[i]);
> +	}
> +	printf("\n");
> +}
> +
> +#define MAX_DEPTH 10

How about 20, like the common backtrace() supports?

> +
> +void dump_stack(void)
> +{
> +	const void *return_addrs[MAX_DEPTH];
> +	int depth;
> +
> +	depth = backtrace(return_addrs, MAX_DEPTH);
> +	print_stack(return_addrs, depth, true);
> +}
> +
> +void dump_frame_stack(const void *instruction, const void *frame)
> +{
> +	const void *return_addrs[MAX_DEPTH];
> +	int depth;
> +
> +	return_addrs[0] = instruction;
> +	depth = backtrace_frame(frame, &return_addrs[1], MAX_DEPTH - 1);
> +	print_stack(return_addrs, depth + 1, false);
> +}
> diff --git a/lib/x86/asm/stack.c b/lib/x86/asm/stack.c
> new file mode 100644
> index 0000000..b30b3b1
> --- /dev/null
> +++ b/lib/x86/asm/stack.c
> @@ -0,0 +1,25 @@
> +#include <libcflat.h>
> +#include <asm/stack.h>
> +
> +int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
> +{
> +	static int walking;
> +	int depth = 0;
> +	const unsigned long *bp = (unsigned long *) frame;
> +
> +	if (walking) {
> +		printf("RECURSIVE STACK WALK!!!\n");
> +		return 0;
> +	}
> +	walking = 1;
> +
> +	for (depth = 0; depth < max_depth; depth++) {
> +		return_addrs[depth] = (void *) bp[1];
> +		if (return_addrs[depth] == 0)
> +			break;
> +		bp = (unsigned long *) bp[0];
> +	}
> +
> +	walking = 0;
> +	return depth;
> +}
> diff --git a/lib/x86/asm/stack.h b/lib/x86/asm/stack.h
> new file mode 100644
> index 0000000..c43bb96
> --- /dev/null
> +++ b/lib/x86/asm/stack.h
> @@ -0,0 +1,12 @@
> +#ifndef _X86ASM_STACK_H_
> +#define _X86ASM_STACK_H_
> +
> +int backtrace_frame(const void *frame, const void **return_addrs, int max_depth);
> +
> +static inline int backtrace(const void **return_addrs, int max_depth)
> +{
> +	return backtrace_frame(__builtin_frame_address(0), return_addrs,
> +			       max_depth);
> +}
> +
> +#endif
> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> index 424983e..ea8f669 100644
> --- a/powerpc/Makefile.common
> +++ b/powerpc/Makefile.common
> @@ -31,6 +31,7 @@ cflatobjs += lib/powerpc/io.o
>  cflatobjs += lib/powerpc/hcall.o
>  cflatobjs += lib/powerpc/setup.o
>  cflatobjs += lib/powerpc/rtas.o
> +cflatobjs += lib/asm-generic/stack.o
>  
>  FLATLIBS = $(libcflat) $(LIBFDT_archive)
>  %.elf: CFLAGS += $(arch_CFLAGS)
> diff --git a/x86/Makefile.common b/x86/Makefile.common
> index 3a14fea..c4750f0 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/asm/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

Other than my desire to keep C out of asm directories, then this
looks good to me.

Thanks,
drew