Messages in this thread | | | Date | Mon, 20 Sep 2010 12:44:15 +0200 | From | Robert Richter <> | Subject | Re: [PATCH] oprofile, x86: allow backtrace for 32bit apps under 64bit kernel |
| |
On 13.09.10 08:15:17, Jiri Olsa wrote: > hi, > > adding support for backtrace of ia32 applications under 64bit kernels. > > wbr, > jirka > > > > Signed-off-by: Jiri Olsa <jolsa@redhat.com> > Signed-off-by: Tom Marshall <tdm.rhbz@gmail.com> > --- > arch/x86/oprofile/backtrace.c | 42 +++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 42 insertions(+), 0 deletions(-)
Jirka,
see my comments below.
Please, provide a more meaningful commit message.
> > diff --git a/arch/x86/oprofile/backtrace.c b/arch/x86/oprofile/backtrace.c > index 3855096..3e443ad 100644 > --- a/arch/x86/oprofile/backtrace.c > +++ b/arch/x86/oprofile/backtrace.c > @@ -73,6 +73,38 @@ static struct frame_head *dump_user_backtrace(struct frame_head *head) > return bufhead[0].bp; > } > > +#ifdef CONFIG_X86_64 > +struct frame_head_32 { > + __u32 ebp; > + __u32 ret; > +} __attribute__((packed));
There exists already struct stack_frame_ia32.
Please, also change the code to reuse struct stack_frame for 64 bit processes.
> + > +static struct frame_head * > +dump_user_backtrace_32(struct frame_head *head) > +{ > + struct frame_head_32 bufhead_32[2]; > + unsigned long bp, ret; > + > + /* Also check accessibility of one struct frame_head beyond */ > + if (!access_ok(VERIFY_READ, head, sizeof(bufhead_32))) > + return NULL; > + if (__copy_from_user_inatomic(bufhead_32, head, sizeof(bufhead_32))) > + return NULL; > + > + bp = bufhead_32[0].ebp; > + ret = bufhead_32[0].ret; > + > + oprofile_add_trace(ret); > + > + /* frame pointers should strictly progress back up the stack > + * (towards higher addresses) */ > + if (head >= (struct frame_head *) bp)
You are mixing up struct frame_head and struct frame_head_32 here.
> + return NULL; > + > + return (struct frame_head *) bp; > +} > +#endif > + > void > x86_backtrace(struct pt_regs * const regs, unsigned int depth) > { > @@ -86,6 +118,16 @@ x86_backtrace(struct pt_regs * const regs, unsigned int depth) > return; > } > > +#ifdef CONFIG_X86_64
Rather use CONFIG_COMPAT here.
> + if (current && test_thread_flag(TIF_IA32)) { > + /* User process is 32-bit */ > + head = (struct frame_head *)(regs->bp & 0xffffffff);
Same is provided with compat_ptr().
> + while (depth-- && head) > + head = dump_user_backtrace_32(head); > + return; > + } > +#endif
Please avoid inline macros and make this an inline function (maybe x86_backtrace_32) with a function stub for the !CONFIG_COMPAT case.
If possible, try to avoid duplicate code for 32 and 64 bit backtraces.
Thanks,
-Robert
> + > while (depth-- && head) > head = dump_user_backtrace(head); > } > -- > 1.7.1 > >
-- Advanced Micro Devices, Inc. Operating System Research Center
| |