From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [Patch v4 1/4] x86/stack: Refactor show_trace()
Date: Wed, 20 Nov 2013 14:11:18 +0000
Message-ID: <528CC306.1090108@citrix.com>
References: <1384952982-1490-1-git-send-email-andrew.cooper3@citrix.com>
	<1384952982-1490-2-git-send-email-andrew.cooper3@citrix.com>
	<528CC9C902000078001050CF@nat28.tlf.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta5.messagelabs.com ([195.245.231.135])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <Andrew.Cooper3@citrix.com>) id 1Vj8V0-0003D5-QM
	for xen-devel@lists.xenproject.org; Wed, 20 Nov 2013 14:11:23 +0000
In-Reply-To: <528CC9C902000078001050CF@nat28.tlf.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>, Tim Deegan <tim@xen.org>
List-Id: xen-devel@lists.xenproject.org

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