Re: [Patch v4 1/4] x86/stack: Refactor show_trace()

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 20/11/13 13:40, Jan Beulich wrote:
>>>> On 20.11.13 at 14:09, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> +static void show_trace(const struct cpu_user_regs *regs)
>> +{
>> +    unsigned long *sp = ESP_BEFORE_EXCEPTION(regs);
> So you correctly made it a pointer here...
>
>> +
>> +    printk("Xen call trace:\n");
>> +
>> +    /*
>> +     * If RIP looks sensible, or the top of the stack doesn't, print RIP at
>> +     * the top of the stack trace.
>> +     */
>> +    if ( is_active_kernel_text(regs->rip) ||
>> +         !is_active_kernel_text(*sp) )
> ... and de-reference it here ...
>
>> +        printk("   [<%p>] %pS\n", _p(regs->rip), _p(regs->rip));
>> +    /*
>> +     * Else RIP looks bad but the top of the stack looks good.  Perhaps we
>> +     * followed a wild function pointer? Lets assume the top of the stack is a
>> +     * return address; print it and skip past so _show_trace() doesn't print
>> +     * it again.
>> +     */
>> +    else
>> +    {
>> +        printk("   [<%p>] %pS\n", _p(*sp), _p(*sp));
>> +        sp++;
>> +    }
>> +
>> +    _show_trace(*sp, regs->rbp);
> ... but then you now also de-reference it here? How did that end
> up producing sane stack dumps?
>
> Since one of the two _show_trace() variants wants a pointer here
> anyway, you would probably best switch its first argument and use
> the inverse casting in the other variant.
>
> Jan
>

Huh - I appear to have tested the debug build twice rather than 1 debug
and 1 non-debug build.  I clearly need to be rather more careful!

I am not so certain about changing the _show_trace() prototype.  The
naive algorithm wants everything as pointers, while the frame-pointer
algorithm wants everything as integers.  Switching to passing by pointer
would require an equal amount of ugly casting back to an integer for
get_printable_stack_bottom() etc.

Personally I would prefer to keep as integers to be in line with the
registers;  I think it looks more natural to take sp as a resister value
and cast it to a pointer to look at the stack, than passing around a
pointer and casting it to an integer for bounds.

~Andrew