Hi, Franck. Thank you for detailed review.
On Thu, 27 Jul 2006 16:33:08 +0200, Franck Bui-Huu <vagabon.xyz@xxxxxxxxx> wrote:
> > + if (info->pc_offset < 0 || info->frame_size == 0) {
> > + if (info->func == schedule)
>
> This can't happen since "schedule" is not a leaf function. Something I'm
> missing here but I would have said:
>
> if (func != schedule)
>
> instead, no ?
This "if (info->func == schedule)" condition is originally in current
get_frame_info(). And it was added to report "Can't analyze" message
only for schedule() function. This is because we can get at least
somewhat worth results for thread_saved_pc() and get_wchan() if the
frame information for the schedule() could be analyzed. The frame
information for other functions just make get_wchan()'s result better.
> > + if (info.pc_offset < 0 || !info.frame_size) {
> > + /* leaf? */
>
> for leaf case, can't we simply do this test:
>
> if (info.pc_offset < 0) {
>
> IOW, can a leaf function move sp ? I would say yes...
Normally, we can omit "!info.frame_size". But as I wrote in other
mail, I think it is hard to make perfect get_frame_info(). We should
handle any wired result from get_frame_info().
> BTW why not let this logic inside get_frame_info() ? Hence this function
> could return:
>
> if (info.frame_size && info.pc_offset > 0) /* nested */
> return 0;
> if (info.pc_offset < 0) /* leaf */
> return 1;
> /* prologue seems boggus... */
> printk("Can't analyze prologue code at %p\n", info->func);
> return -1;
Indeed. I'll make new patch. But I think put printk() in
get_frame_info() not good because now I want to use it for
show_trace(). I don't want to see the "Can't analyze" message in oops
log.
> > + *sp += info.frame_size / sizeof(long);
> > + return 0;
>
> why not returning:
> return regs->regs[31];
>
> and removes the leaf detection logic in show_frametrace() ?
Because unwind_stack() does not have "regs" argument. The RA
information is only needed for leaf (i.e. top on stack trace) and
unwind_stack() is used for any level of stack, so I think it is better
to handle the leaf case in show_frametrace().
> > + pc = (*sp)[info.pc_offset];
> > + *sp += info.frame_size / sizeof(long);
> > + return pc;
>
> why not directly doing:
>
> return (*sp)[info.pc_offset];
>
> and remove:
>
> pc = (*sp)[info.pc_offset];
This is wrong. The *sp must be modified before return.
> > + unsigned long *stack = (long *)regs->regs[29];
>
> why not calling that "sp" ?
Just because show_trace() named it "stack" :-)
---
Atsushi Nemoto
