On Thu, Mar 3, 2016 at 1:17 AM, Andrew Jones <drjones@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
