| Message ID | 1394705630-12384-7-git-send-email-takahiro.akashi@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 13, 2014 at 10:13:49AM +0000, AKASHI Takahiro wrote: > CALLER_ADDRx returns caller's address at specified level in call stacks. > They are used for several tracers like irqsoff and preemptoff. > Strange to say, however, they are refered even without FTRACE. > > Please note that this implementation assumes that we have frame pointers. > (which means kernel should be compiled with -fno-omit-frame-pointer.) How do you ensure that -fno-omit-frame-pointer is passed? > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > arch/arm64/include/asm/ftrace.h | 13 ++++++++- > arch/arm64/kernel/Makefile | 3 +- > arch/arm64/kernel/return_address.c | 55 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 69 insertions(+), 2 deletions(-) > create mode 100644 arch/arm64/kernel/return_address.c > > diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h > index ed5c448..c44c4b1 100644 > --- a/arch/arm64/include/asm/ftrace.h > +++ b/arch/arm64/include/asm/ftrace.h > @@ -18,6 +18,7 @@ > > #ifndef __ASSEMBLY__ > extern void _mcount(unsigned long); > +extern void *return_address(unsigned int); > > struct dyn_arch_ftrace { > /* No extra data needed for arm64 */ > @@ -33,6 +34,16 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr) > */ > return addr; > } > -#endif /* __ASSEMBLY__ */ > + > +#define HAVE_ARCH_CALLER_ADDR > + > +#define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) > +#define CALLER_ADDR1 ((unsigned long)return_address(1)) > +#define CALLER_ADDR2 ((unsigned long)return_address(2)) > +#define CALLER_ADDR3 ((unsigned long)return_address(3)) > +#define CALLER_ADDR4 ((unsigned long)return_address(4)) > +#define CALLER_ADDR5 ((unsigned long)return_address(5)) > +#define CALLER_ADDR6 ((unsigned long)return_address(6)) Could we change the core definitions of these macros (in linux/ftrace.h) to use return_address, then provide an overridable version of return_address that defaults to __builtin_return_address, instead of copy-pasting this sequence? > diff --git a/arch/arm64/kernel/return_address.c b/arch/arm64/kernel/return_address.c > new file mode 100644 > index 0000000..89102a6 > --- /dev/null > +++ b/arch/arm64/kernel/return_address.c > @@ -0,0 +1,55 @@ > +/* > + * arch/arm64/kernel/return_address.c > + * > + * Copyright (C) 2013 Linaro Limited > + * Author: AKASHI Takahiro <takahiro.akashi@linaro.org> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include <linux/export.h> > +#include <linux/ftrace.h> > + > +#include <asm/stacktrace.h> > + > +struct return_address_data { > + unsigned int level; > + void *addr; > +}; > + > +static int save_return_addr(struct stackframe *frame, void *d) > +{ > + struct return_address_data *data = d; > + > + if (!data->level) { > + data->addr = (void *)frame->pc; > + return 1; > + } else { > + --data->level; > + return 0; > + } > +} > + > +void *return_address(unsigned int level) > +{ > + struct return_address_data data; > + struct stackframe frame; > + register unsigned long current_sp asm ("sp"); > + > + data.level = level + 2; > + data.addr = NULL; > + > + frame.fp = (unsigned long)__builtin_frame_address(0); > + frame.sp = current_sp; > + frame.pc = (unsigned long)return_address; /* dummy */ > + > + walk_stackframe(&frame, save_return_addr, &data); > + > + if (!data.level) > + return data.addr; > + else > + return NULL; > +} > +EXPORT_SYMBOL_GPL(return_address); This whole file is basically copied from arch/arm/, but it's not too much code. Ideally the toolchain would have made use of the frame pointer, but it looks like it doesn't bother. Will
On Thu, 2014-03-13 at 15:54 +0000, Will Deacon wrote: > On Thu, Mar 13, 2014 at 10:13:49AM +0000, AKASHI Takahiro wrote: > > CALLER_ADDRx returns caller's address at specified level in call stacks. > > They are used for several tracers like irqsoff and preemptoff. > > Strange to say, however, they are refered even without FTRACE. > > > > Please note that this implementation assumes that we have frame pointers. > > (which means kernel should be compiled with -fno-omit-frame-pointer.) > > How do you ensure that -fno-omit-frame-pointer is passed? Perhaps -pg does the same thing? > > +#define HAVE_ARCH_CALLER_ADDR > > + > > +#define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) > > +#define CALLER_ADDR1 ((unsigned long)return_address(1)) > > +#define CALLER_ADDR2 ((unsigned long)return_address(2)) > > +#define CALLER_ADDR3 ((unsigned long)return_address(3)) > > +#define CALLER_ADDR4 ((unsigned long)return_address(4)) > > +#define CALLER_ADDR5 ((unsigned long)return_address(5)) > > +#define CALLER_ADDR6 ((unsigned long)return_address(6)) > > Could we change the core definitions of these macros (in linux/ftrace.h) to > use return_address, then provide an overridable version of return_address > that defaults to __builtin_return_address, instead of copy-pasting this > sequence? We could add a new macro: /* All archs should have this, but we define it for consistency */ #ifndef ftrace_return_address0 # define ftrace_return_address0 __builtin_return_address(0) #endif /* Archs may use other ways for ADDR1 and beyond */ #ifndef ftrace_return_address # define ftrace_return_address(n) __builtin_return_address(n) #endif And then have: #define CALLER_ADDR0 ((unsigned long)ftrace_return_address0) #define CALLER_ADDR1 ((unsigned long)ftrace_return_address(1)) [...] And then you would only need to redefine ftrace_return_address. -- Steve
On 03/14/2014 12:54 AM, Will Deacon wrote: > On Thu, Mar 13, 2014 at 10:13:49AM +0000, AKASHI Takahiro wrote: >> CALLER_ADDRx returns caller's address at specified level in call stacks. >> They are used for several tracers like irqsoff and preemptoff. >> Strange to say, however, they are refered even without FTRACE. >> >> Please note that this implementation assumes that we have frame pointers. >> (which means kernel should be compiled with -fno-omit-frame-pointer.) > > How do you ensure that -fno-omit-frame-pointer is passed? arm64 selects ARCH_WANT_FRAME_POINTERS, then FRAME_POINTER is on (lib/Kconfig.debug) and so -fno-omit-frame-pointer is appended (${TOP}/Makefile). (stacktrace.c also assumes FRAME_POINTER.) Do you think I should remove the comment above? >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >> --- >> arch/arm64/include/asm/ftrace.h | 13 ++++++++- >> arch/arm64/kernel/Makefile | 3 +- >> arch/arm64/kernel/return_address.c | 55 ++++++++++++++++++++++++++++++++++++ >> 3 files changed, 69 insertions(+), 2 deletions(-) >> create mode 100644 arch/arm64/kernel/return_address.c >> >> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h >> index ed5c448..c44c4b1 100644 >> --- a/arch/arm64/include/asm/ftrace.h >> +++ b/arch/arm64/include/asm/ftrace.h >> @@ -18,6 +18,7 @@ >> >> #ifndef __ASSEMBLY__ >> extern void _mcount(unsigned long); >> +extern void *return_address(unsigned int); >> >> struct dyn_arch_ftrace { >> /* No extra data needed for arm64 */ >> @@ -33,6 +34,16 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr) >> */ >> return addr; >> } >> -#endif /* __ASSEMBLY__ */ >> + >> +#define HAVE_ARCH_CALLER_ADDR >> + >> +#define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) >> +#define CALLER_ADDR1 ((unsigned long)return_address(1)) >> +#define CALLER_ADDR2 ((unsigned long)return_address(2)) >> +#define CALLER_ADDR3 ((unsigned long)return_address(3)) >> +#define CALLER_ADDR4 ((unsigned long)return_address(4)) >> +#define CALLER_ADDR5 ((unsigned long)return_address(5)) >> +#define CALLER_ADDR6 ((unsigned long)return_address(6)) > > Could we change the core definitions of these macros (in linux/ftrace.h) to > use return_address, then provide an overridable version of return_address > that defaults to __builtin_return_address, instead of copy-pasting this > sequence? I think I understand what you mean, and will try to post a separate RFC, but I also want to hold off this change on this patch since such a change may raise a small controversy from other archs' maintainers. >> diff --git a/arch/arm64/kernel/return_address.c b/arch/arm64/kernel/return_address.c >> new file mode 100644 >> index 0000000..89102a6 >> --- /dev/null >> +++ b/arch/arm64/kernel/return_address.c >> @@ -0,0 +1,55 @@ >> +/* >> + * arch/arm64/kernel/return_address.c >> + * >> + * Copyright (C) 2013 Linaro Limited >> + * Author: AKASHI Takahiro <takahiro.akashi@linaro.org> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> + >> +#include <linux/export.h> >> +#include <linux/ftrace.h> >> + >> +#include <asm/stacktrace.h> >> + >> +struct return_address_data { >> + unsigned int level; >> + void *addr; >> +}; >> + >> +static int save_return_addr(struct stackframe *frame, void *d) >> +{ >> + struct return_address_data *data = d; >> + >> + if (!data->level) { >> + data->addr = (void *)frame->pc; >> + return 1; >> + } else { >> + --data->level; >> + return 0; >> + } >> +} >> + >> +void *return_address(unsigned int level) >> +{ >> + struct return_address_data data; >> + struct stackframe frame; >> + register unsigned long current_sp asm ("sp"); >> + >> + data.level = level + 2; >> + data.addr = NULL; >> + >> + frame.fp = (unsigned long)__builtin_frame_address(0); >> + frame.sp = current_sp; >> + frame.pc = (unsigned long)return_address; /* dummy */ >> + >> + walk_stackframe(&frame, save_return_addr, &data); >> + >> + if (!data.level) >> + return data.addr; >> + else >> + return NULL; >> +} >> +EXPORT_SYMBOL_GPL(return_address); > > This whole file is basically copied from arch/arm/, but it's not too much > code. Ideally the toolchain would have made use of the frame pointer, but it > looks like it doesn't bother. I confirmed that __builtin_return_address([123456]) doesn't work even with -fno-omit-frame-pointer. Keep this as it is. -Takahiro AKASHI > Will >
On 03/14/2014 03:07 AM, Steven Rostedt wrote: > On Thu, 2014-03-13 at 15:54 +0000, Will Deacon wrote: >> On Thu, Mar 13, 2014 at 10:13:49AM +0000, AKASHI Takahiro wrote: >>> CALLER_ADDRx returns caller's address at specified level in call stacks. >>> They are used for several tracers like irqsoff and preemptoff. >>> Strange to say, however, they are refered even without FTRACE. >>> >>> Please note that this implementation assumes that we have frame pointers. >>> (which means kernel should be compiled with -fno-omit-frame-pointer.) >> >> How do you ensure that -fno-omit-frame-pointer is passed? > > Perhaps -pg does the same thing? > >>> +#define HAVE_ARCH_CALLER_ADDR >>> + >>> +#define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) >>> +#define CALLER_ADDR1 ((unsigned long)return_address(1)) >>> +#define CALLER_ADDR2 ((unsigned long)return_address(2)) >>> +#define CALLER_ADDR3 ((unsigned long)return_address(3)) >>> +#define CALLER_ADDR4 ((unsigned long)return_address(4)) >>> +#define CALLER_ADDR5 ((unsigned long)return_address(5)) >>> +#define CALLER_ADDR6 ((unsigned long)return_address(6)) >> >> Could we change the core definitions of these macros (in linux/ftrace.h) to >> use return_address, then provide an overridable version of return_address >> that defaults to __builtin_return_address, instead of copy-pasting this >> sequence? > > We could add a new macro: > > /* All archs should have this, but we define it for consistency */ > #ifndef ftrace_return_address0 > # define ftrace_return_address0 __builtin_return_address(0) > #endif > /* Archs may use other ways for ADDR1 and beyond */ > #ifndef ftrace_return_address > # define ftrace_return_address(n) __builtin_return_address(n) > #endif > > And then have: > > #define CALLER_ADDR0 ((unsigned long)ftrace_return_address0) > #define CALLER_ADDR1 ((unsigned long)ftrace_return_address(1)) > [...] > > And then you would only need to redefine ftrace_return_address. I'm going to create a separate RFC, including fixes for other archs. -Takahiro AKASHI > -- Steve > >
On Fri, Mar 14, 2014 at 03:00:14AM +0000, AKASHI Takahiro wrote: > On 03/14/2014 12:54 AM, Will Deacon wrote: > > On Thu, Mar 13, 2014 at 10:13:49AM +0000, AKASHI Takahiro wrote: > >> CALLER_ADDRx returns caller's address at specified level in call stacks. > >> They are used for several tracers like irqsoff and preemptoff. > >> Strange to say, however, they are refered even without FTRACE. > >> > >> Please note that this implementation assumes that we have frame pointers. > >> (which means kernel should be compiled with -fno-omit-frame-pointer.) > > > > How do you ensure that -fno-omit-frame-pointer is passed? > > arm64 selects ARCH_WANT_FRAME_POINTERS, then FRAME_POINTER is on (lib/Kconfig.debug) > and so -fno-omit-frame-pointer is appended (${TOP}/Makefile). > (stacktrace.c also assumes FRAME_POINTER.) > > Do you think I should remove the comment above? Yes please, it sounds like everything is taken care of automatically, so there's no need to scare people in the commit message! > >> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h > >> index ed5c448..c44c4b1 100644 > >> --- a/arch/arm64/include/asm/ftrace.h > >> +++ b/arch/arm64/include/asm/ftrace.h > >> @@ -18,6 +18,7 @@ > >> > >> #ifndef __ASSEMBLY__ > >> extern void _mcount(unsigned long); > >> +extern void *return_address(unsigned int); > >> > >> struct dyn_arch_ftrace { > >> /* No extra data needed for arm64 */ > >> @@ -33,6 +34,16 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr) > >> */ > >> return addr; > >> } > >> -#endif /* __ASSEMBLY__ */ > >> + > >> +#define HAVE_ARCH_CALLER_ADDR > >> + > >> +#define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) > >> +#define CALLER_ADDR1 ((unsigned long)return_address(1)) > >> +#define CALLER_ADDR2 ((unsigned long)return_address(2)) > >> +#define CALLER_ADDR3 ((unsigned long)return_address(3)) > >> +#define CALLER_ADDR4 ((unsigned long)return_address(4)) > >> +#define CALLER_ADDR5 ((unsigned long)return_address(5)) > >> +#define CALLER_ADDR6 ((unsigned long)return_address(6)) > > > > Could we change the core definitions of these macros (in linux/ftrace.h) to > > use return_address, then provide an overridable version of return_address > > that defaults to __builtin_return_address, instead of copy-pasting this > > sequence? > > I think I understand what you mean, and will try to post a separate RFC, > but I also want to hold off this change on this patch since such a change > may raise a small controversy from other archs' maintainers. I don't see anything controversial here, but ok. Steve already posted something you can get started with. Will
On 03/14/2014 07:09 PM, Will Deacon wrote: > On Fri, Mar 14, 2014 at 03:00:14AM +0000, AKASHI Takahiro wrote: >> On 03/14/2014 12:54 AM, Will Deacon wrote: >>> On Thu, Mar 13, 2014 at 10:13:49AM +0000, AKASHI Takahiro wrote: >>>> CALLER_ADDRx returns caller's address at specified level in call stacks. >>>> They are used for several tracers like irqsoff and preemptoff. >>>> Strange to say, however, they are refered even without FTRACE. >>>> >>>> Please note that this implementation assumes that we have frame pointers. >>>> (which means kernel should be compiled with -fno-omit-frame-pointer.) >>> >>> How do you ensure that -fno-omit-frame-pointer is passed? >> >> arm64 selects ARCH_WANT_FRAME_POINTERS, then FRAME_POINTER is on (lib/Kconfig.debug) >> and so -fno-omit-frame-pointer is appended (${TOP}/Makefile). >> (stacktrace.c also assumes FRAME_POINTER.) >> >> Do you think I should remove the comment above? > > Yes please, it sounds like everything is taken care of automatically, so > there's no need to scare people in the commit message! OK, I will do so. Thanks, -Takahiro AKASHI >>>> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h >>>> index ed5c448..c44c4b1 100644 >>>> --- a/arch/arm64/include/asm/ftrace.h >>>> +++ b/arch/arm64/include/asm/ftrace.h >>>> @@ -18,6 +18,7 @@ >>>> >>>> #ifndef __ASSEMBLY__ >>>> extern void _mcount(unsigned long); >>>> +extern void *return_address(unsigned int); >>>> >>>> struct dyn_arch_ftrace { >>>> /* No extra data needed for arm64 */ >>>> @@ -33,6 +34,16 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr) >>>> */ >>>> return addr; >>>> } >>>> -#endif /* __ASSEMBLY__ */ >>>> + >>>> +#define HAVE_ARCH_CALLER_ADDR >>>> + >>>> +#define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) >>>> +#define CALLER_ADDR1 ((unsigned long)return_address(1)) >>>> +#define CALLER_ADDR2 ((unsigned long)return_address(2)) >>>> +#define CALLER_ADDR3 ((unsigned long)return_address(3)) >>>> +#define CALLER_ADDR4 ((unsigned long)return_address(4)) >>>> +#define CALLER_ADDR5 ((unsigned long)return_address(5)) >>>> +#define CALLER_ADDR6 ((unsigned long)return_address(6)) >>> >>> Could we change the core definitions of these macros (in linux/ftrace.h) to >>> use return_address, then provide an overridable version of return_address >>> that defaults to __builtin_return_address, instead of copy-pasting this >>> sequence? >> >> I think I understand what you mean, and will try to post a separate RFC, >> but I also want to hold off this change on this patch since such a change >> may raise a small controversy from other archs' maintainers. > > I don't see anything controversial here, but ok. Steve already posted > something you can get started with. > > Will >
diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h index ed5c448..c44c4b1 100644 --- a/arch/arm64/include/asm/ftrace.h +++ b/arch/arm64/include/asm/ftrace.h @@ -18,6 +18,7 @@ #ifndef __ASSEMBLY__ extern void _mcount(unsigned long); +extern void *return_address(unsigned int); struct dyn_arch_ftrace { /* No extra data needed for arm64 */ @@ -33,6 +34,16 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr) */ return addr; } -#endif /* __ASSEMBLY__ */ + +#define HAVE_ARCH_CALLER_ADDR + +#define CALLER_ADDR0 ((unsigned long)__builtin_return_address(0)) +#define CALLER_ADDR1 ((unsigned long)return_address(1)) +#define CALLER_ADDR2 ((unsigned long)return_address(2)) +#define CALLER_ADDR3 ((unsigned long)return_address(3)) +#define CALLER_ADDR4 ((unsigned long)return_address(4)) +#define CALLER_ADDR5 ((unsigned long)return_address(5)) +#define CALLER_ADDR6 ((unsigned long)return_address(6)) +#endif /* ifndef __ASSEMBLY__ */ #endif /* __ASM_FTRACE_H */ diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile index ac67fd0..b5bfa7f 100644 --- a/arch/arm64/kernel/Makefile +++ b/arch/arm64/kernel/Makefile @@ -7,12 +7,13 @@ AFLAGS_head.o := -DTEXT_OFFSET=$(TEXT_OFFSET) CFLAGS_REMOVE_ftrace.o = -pg CFLAGS_REMOVE_insn.o = -pg +CFLAGS_REMOVE_return_address.o = -pg # Object file lists. arm64-obj-y := cputable.o debug-monitors.o entry.o irq.o fpsimd.o \ entry-fpsimd.o process.o ptrace.o setup.o signal.o \ sys.o stacktrace.o time.o traps.o io.o vdso.o \ - hyp-stub.o psci.o cpu_ops.o insn.o + hyp-stub.o psci.o cpu_ops.o insn.o return_address.o arm64-obj-$(CONFIG_COMPAT) += sys32.o kuser32.o signal32.o \ sys_compat.o diff --git a/arch/arm64/kernel/return_address.c b/arch/arm64/kernel/return_address.c new file mode 100644 index 0000000..89102a6 --- /dev/null +++ b/arch/arm64/kernel/return_address.c @@ -0,0 +1,55 @@ +/* + * arch/arm64/kernel/return_address.c + * + * Copyright (C) 2013 Linaro Limited + * Author: AKASHI Takahiro <takahiro.akashi@linaro.org> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/export.h> +#include <linux/ftrace.h> + +#include <asm/stacktrace.h> + +struct return_address_data { + unsigned int level; + void *addr; +}; + +static int save_return_addr(struct stackframe *frame, void *d) +{ + struct return_address_data *data = d; + + if (!data->level) { + data->addr = (void *)frame->pc; + return 1; + } else { + --data->level; + return 0; + } +} + +void *return_address(unsigned int level) +{ + struct return_address_data data; + struct stackframe frame; + register unsigned long current_sp asm ("sp"); + + data.level = level + 2; + data.addr = NULL; + + frame.fp = (unsigned long)__builtin_frame_address(0); + frame.sp = current_sp; + frame.pc = (unsigned long)return_address; /* dummy */ + + walk_stackframe(&frame, save_return_addr, &data); + + if (!data.level) + return data.addr; + else + return NULL; +} +EXPORT_SYMBOL_GPL(return_address);
CALLER_ADDRx returns caller's address at specified level in call stacks. They are used for several tracers like irqsoff and preemptoff. Strange to say, however, they are refered even without FTRACE. Please note that this implementation assumes that we have frame pointers. (which means kernel should be compiled with -fno-omit-frame-pointer.) Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- arch/arm64/include/asm/ftrace.h | 13 ++++++++- arch/arm64/kernel/Makefile | 3 +- arch/arm64/kernel/return_address.c | 55 ++++++++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 2 deletions(-) create mode 100644 arch/arm64/kernel/return_address.c