mini-os: x86_64: crash passing double arguments

mini-os: x86_64: crash passing double arguments

[Thomas Leonard]

[-- Attachment #1: Type: text/plain, Size: 1141 bytes --]

I noticed that passing a double argument to a varargs function on
x86_64 causes Mini-OS to crash. This only happens when called from a
thread, not when using the boot stack.

Here's my test case:

static void foo(char *fmt, ...) { }

static void float_tester(void *p)
{
    printk("Running tests...\n");
    foo("%d", 3);
    printk("Pass int.\n");
    foo("%f", 3.1);
    printk("Pass float.\n");
}

When run from test.c's app_main directly, it passes, but when run from
a thread it crashes:

    (d22) Running tests...
    (d22) Pass int.
    (d22) GPF rip: 0000000000006b04, error_code=0
    (d22) Thread: float
    (d22) RIP: e030:[<0000000000006b04>]

Subtracting 8 bytes from thread->sp when creating a new thread fixes
it (patch attached), but I'm not sure whether this is the right
solution, or whether there's a problem elsewhere (I know very little
about x86).

My test-case and fix/work-around are here:

https://github.com/talex5/xen/commits/stack-fix-x86

Thanks,


-- 
Dr Thomas Leonard        http://0install.net/
GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

[-- Attachment #2: 0001-mini-os-x86_64-make-thread-stacks-16-byte-aligned.patch --]
[-- Type: text/x-patch, Size: 1136 bytes --]

From 2519db5ad5da187ff1c466c7175b8e02d8059ed2 Mon Sep 17 00:00:00 2001
From: Thomas Leonard <talex5@gmail.com>
Date: Wed, 2 Jul 2014 10:15:26 +0100
Subject: [PATCH] mini-os: x86_64: make thread stacks 16-byte aligned

Otherwise, passing doubles to varargs functions causes a crash.

Signed-off-by: Thomas Leonard <talex5@gmail.com>
---
 extras/mini-os/arch/x86/sched.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/extras/mini-os/arch/x86/sched.c b/extras/mini-os/arch/x86/sched.c
index 8a05b58..e4a3dc2 100644
--- a/extras/mini-os/arch/x86/sched.c
+++ b/extras/mini-os/arch/x86/sched.c
@@ -107,6 +107,9 @@ struct thread* arch_create_thread(char *name, void (*function)(void *),
     thread->sp = (unsigned long)thread->stack + STACK_SIZE;
     /* Save pointer to the thread on the stack, used by current macro */
     *((unsigned long *)thread->stack) = (unsigned long)thread;
+
+    /* Must ensure that (%rsp + 8) is 16-byte aligned at the start of thread_starter. */
+    thread->sp -= sizeof(unsigned long);
     
     stack_push(thread, (unsigned long) function);
     stack_push(thread, (unsigned long) data);
-- 
2.0.0


[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: mini-os: x86_64: crash passing double arguments

[Jan Beulich]

>>> On 02.07.14 at 12:17, <talex5@gmail.com> wrote:
> Subtracting 8 bytes from thread->sp when creating a new thread fixes
> it (patch attached), but I'm not sure whether this is the right
> solution, or whether there's a problem elsewhere (I know very little
> about x86).

Considering that this really is kernel code, passing
-mpreferred-stack-boundary=2 to gcc would seem like the better
option to me, or else someone might run into a similar issue again
trying to use %ymm or %zmm registers in their MiniOS incarnation.

Jan

Re: mini-os: x86_64: crash passing double arguments

[Thomas Leonard]

On 2 July 2014 11:36, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 02.07.14 at 12:17, <talex5@gmail.com> wrote:
>> Subtracting 8 bytes from thread->sp when creating a new thread fixes
>> it (patch attached), but I'm not sure whether this is the right
>> solution, or whether there's a problem elsewhere (I know very little
>> about x86).
>
> Considering that this really is kernel code, passing
> -mpreferred-stack-boundary=2 to gcc would seem like the better
> option to me, or else someone might run into a similar issue again
> trying to use %ymm or %zmm registers in their MiniOS incarnation.

Could you explain this a bit further? It looks like using
-mpreferred-stack-boundary=2 just means that the code won't compile
("error: -mpreferred-stack-boundary=2 is not between 4 and 12").

Although Mini-OS applications run in kernel mode, they can still be
full applications, and need to use all the processor's features.


-- 
Dr Thomas Leonard        http://0install.net/
GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

Re: mini-os: x86_64: crash passing double arguments

[Jan Beulich]

>>> On 02.07.14 at 13:16, <talex5@gmail.com> wrote:
> On 2 July 2014 11:36, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 02.07.14 at 12:17, <talex5@gmail.com> wrote:
>>> Subtracting 8 bytes from thread->sp when creating a new thread fixes
>>> it (patch attached), but I'm not sure whether this is the right
>>> solution, or whether there's a problem elsewhere (I know very little
>>> about x86).
>>
>> Considering that this really is kernel code, passing
>> -mpreferred-stack-boundary=2 to gcc would seem like the better
>> option to me, or else someone might run into a similar issue again
>> trying to use %ymm or %zmm registers in their MiniOS incarnation.
> 
> Could you explain this a bit further? It looks like using
> -mpreferred-stack-boundary=2 just means that the code won't compile
> ("error: -mpreferred-stack-boundary=2 is not between 4 and 12").

Yeah, I should have written "3" (2 is for ix86). Linux uses 3, so it
certainly works (but without checking I can't immediately say
whether that's perhaps tied to the use of -mcmodel=kernel).

> Although Mini-OS applications run in kernel mode, they can still be
> full applications, and need to use all the processor's features.

Right, and that option doesn't preclude that use. It's just that
the compiler will take care to align the stack suitably in each
function when it's told that the to be expected alignment is less
strict than the one needed, or to use instructions not requiring
full alignment.

Jan

Re: mini-os: x86_64: crash passing double arguments

[Ian Campbell]

On Wed, 2014-07-02 at 12:32 +0100, Jan Beulich wrote:
> >>> On 02.07.14 at 13:16, <talex5@gmail.com> wrote:
> > On 2 July 2014 11:36, Jan Beulich <JBeulich@suse.com> wrote:
> >>>>> On 02.07.14 at 12:17, <talex5@gmail.com> wrote:
> >>> Subtracting 8 bytes from thread->sp when creating a new thread fixes
> >>> it (patch attached), but I'm not sure whether this is the right
> >>> solution, or whether there's a problem elsewhere (I know very little
> >>> about x86).
> >>
> >> Considering that this really is kernel code, passing
> >> -mpreferred-stack-boundary=2 to gcc would seem like the better
> >> option to me, or else someone might run into a similar issue again
> >> trying to use %ymm or %zmm registers in their MiniOS incarnation.
> > 
> > Could you explain this a bit further? It looks like using
> > -mpreferred-stack-boundary=2 just means that the code won't compile
> > ("error: -mpreferred-stack-boundary=2 is not between 4 and 12").
> 
> Yeah, I should have written "3" (2 is for ix86). Linux uses 3, so it
> certainly works (but without checking I can't immediately say
> whether that's perhaps tied to the use of -mcmodel=kernel).
> 
> > Although Mini-OS applications run in kernel mode, they can still be
> > full applications, and need to use all the processor's features.
> 
> Right, and that option doesn't preclude that use. It's just that
> the compiler will take care to align the stack suitably in each
> function when it's told that the to be expected alignment is less
> strict than the one needed, or to use instructions not requiring
> full alignment.

Wouldn't it be more efficient to simply arrange that the thread's
initial stack meets the necessary preconditions for the standard
alignments like Thomas has done though? Especially given the trivial
nature of the patch.

Ian.

Re: mini-os: x86_64: crash passing double arguments

[Jan Beulich]

>>> On 02.07.14 at 13:39, <Ian.Campbell@citrix.com> wrote:
> On Wed, 2014-07-02 at 12:32 +0100, Jan Beulich wrote:
>> >>> On 02.07.14 at 13:16, <talex5@gmail.com> wrote:
>> > On 2 July 2014 11:36, Jan Beulich <JBeulich@suse.com> wrote:
>> >>>>> On 02.07.14 at 12:17, <talex5@gmail.com> wrote:
>> >>> Subtracting 8 bytes from thread->sp when creating a new thread fixes
>> >>> it (patch attached), but I'm not sure whether this is the right
>> >>> solution, or whether there's a problem elsewhere (I know very little
>> >>> about x86).
>> >>
>> >> Considering that this really is kernel code, passing
>> >> -mpreferred-stack-boundary=2 to gcc would seem like the better
>> >> option to me, or else someone might run into a similar issue again
>> >> trying to use %ymm or %zmm registers in their MiniOS incarnation.
>> > 
>> > Could you explain this a bit further? It looks like using
>> > -mpreferred-stack-boundary=2 just means that the code won't compile
>> > ("error: -mpreferred-stack-boundary=2 is not between 4 and 12").
>> 
>> Yeah, I should have written "3" (2 is for ix86). Linux uses 3, so it
>> certainly works (but without checking I can't immediately say
>> whether that's perhaps tied to the use of -mcmodel=kernel).
>> 
>> > Although Mini-OS applications run in kernel mode, they can still be
>> > full applications, and need to use all the processor's features.
>> 
>> Right, and that option doesn't preclude that use. It's just that
>> the compiler will take care to align the stack suitably in each
>> function when it's told that the to be expected alignment is less
>> strict than the one needed, or to use instructions not requiring
>> full alignment.
> 
> Wouldn't it be more efficient to simply arrange that the thread's
> initial stack meets the necessary preconditions for the standard
> alignments like Thomas has done though? Especially given the trivial
> nature of the patch.

But that can't be done once and forever, as pointed out before: If
in the future someone wants his MiniOS incarnation to use AVX or
AVX512, the now enforced 16-byte alignment would still not be
enough, it would need growing to 32 or 64 bytes. By using the
named command line option one can make the compiler take care
of this.

Of course an option would be to use his patch to ensure 16-byte
alignment and -mpreferred-stack-boundary=4 to cover eventual
use of the wider registers.

Jan

Re: mini-os: x86_64: crash passing double arguments

[Ian Campbell]

On Wed, 2014-07-02 at 13:01 +0100, Jan Beulich wrote:
> >>> On 02.07.14 at 13:39, <Ian.Campbell@citrix.com> wrote:
> > On Wed, 2014-07-02 at 12:32 +0100, Jan Beulich wrote:
> >> >>> On 02.07.14 at 13:16, <talex5@gmail.com> wrote:
> >> > On 2 July 2014 11:36, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>>>> On 02.07.14 at 12:17, <talex5@gmail.com> wrote:
> >> >>> Subtracting 8 bytes from thread->sp when creating a new thread fixes
> >> >>> it (patch attached), but I'm not sure whether this is the right
> >> >>> solution, or whether there's a problem elsewhere (I know very little
> >> >>> about x86).
> >> >>
> >> >> Considering that this really is kernel code, passing
> >> >> -mpreferred-stack-boundary=2 to gcc would seem like the better
> >> >> option to me, or else someone might run into a similar issue again
> >> >> trying to use %ymm or %zmm registers in their MiniOS incarnation.
> >> > 
> >> > Could you explain this a bit further? It looks like using
> >> > -mpreferred-stack-boundary=2 just means that the code won't compile
> >> > ("error: -mpreferred-stack-boundary=2 is not between 4 and 12").
> >> 
> >> Yeah, I should have written "3" (2 is for ix86). Linux uses 3, so it
> >> certainly works (but without checking I can't immediately say
> >> whether that's perhaps tied to the use of -mcmodel=kernel).
> >> 
> >> > Although Mini-OS applications run in kernel mode, they can still be
> >> > full applications, and need to use all the processor's features.
> >> 
> >> Right, and that option doesn't preclude that use. It's just that
> >> the compiler will take care to align the stack suitably in each
> >> function when it's told that the to be expected alignment is less
> >> strict than the one needed, or to use instructions not requiring
> >> full alignment.
> > 
> > Wouldn't it be more efficient to simply arrange that the thread's
> > initial stack meets the necessary preconditions for the standard
> > alignments like Thomas has done though? Especially given the trivial
> > nature of the patch.
> 
> But that can't be done once and forever, as pointed out before: If
> in the future someone wants his MiniOS incarnation to use AVX or
> AVX512, the now enforced 16-byte alignment would still not be
> enough, it would need growing to 32 or 64 bytes. By using the
> named command line option one can make the compiler take care
> of this.
> 
> Of course an option would be to use his patch to ensure 16-byte
> alignment and -mpreferred-stack-boundary=4 to cover eventual
> use of the wider registers.

I was assuming that 16-bytes (-mpreferred-stack-boundary=4) was the
implied default by the architecture's ABI, hence the compiler would
already naturally deal with things requiring 32 or 64 byte alignment
(with a starting assumption of 16-byte alignment on function entry).

If that's not the case then we should certainly make it explicit on way
or another.

Ian,

Re: mini-os: x86_64: crash passing double arguments

[Jan Beulich]

>>> On 02.07.14 at 14:37, <Ian.Campbell@citrix.com> wrote:
> I was assuming that 16-bytes (-mpreferred-stack-boundary=4) was the
> implied default by the architecture's ABI, hence the compiler would
> already naturally deal with things requiring 32 or 64 byte alignment
> (with a starting assumption of 16-byte alignment on function entry).

That is the default in the absence of any other options. Someone
adding -mavx or some such to their compiler options would, however,
break this. One might argue that (s)he should then also add the
other option, but we can as well be on the safe side from the
beginning.

Anyway - the judgment here is with the MiniOS maintainers.

Jan

Re: mini-os: x86_64: crash passing double arguments

[Ian Campbell]

On Wed, 2014-07-02 at 13:44 +0100, Jan Beulich wrote:
> >>> On 02.07.14 at 14:37, <Ian.Campbell@citrix.com> wrote:
> > I was assuming that 16-bytes (-mpreferred-stack-boundary=4) was the
> > implied default by the architecture's ABI, hence the compiler would
> > already naturally deal with things requiring 32 or 64 byte alignment
> > (with a starting assumption of 16-byte alignment on function entry).
> 
> That is the default in the absence of any other options. Someone
> adding -mavx or some such to their compiler options would, however,
> break this. One might argue that (s)he should then also add the
> other option, but we can as well be on the safe side from the
> beginning.

OK.

I wonder how e.g. pthreads handles this? Is there some gcc provided
#define which exposes the result of all these options so pthreads can
DTRT with the stacks?

> 
> Anyway - the judgment here is with the MiniOS maintainers.
> 
> Jan
> 

Re: mini-os: x86_64: crash passing double arguments

[Jan Beulich]

>>> On 02.07.14 at 14:54, <Ian.Campbell@citrix.com> wrote:
> On Wed, 2014-07-02 at 13:44 +0100, Jan Beulich wrote:
>> >>> On 02.07.14 at 14:37, <Ian.Campbell@citrix.com> wrote:
>> > I was assuming that 16-bytes (-mpreferred-stack-boundary=4) was the
>> > implied default by the architecture's ABI, hence the compiler would
>> > already naturally deal with things requiring 32 or 64 byte alignment
>> > (with a starting assumption of 16-byte alignment on function entry).
>> 
>> That is the default in the absence of any other options. Someone
>> adding -mavx or some such to their compiler options would, however,
>> break this. One might argue that (s)he should then also add the
>> other option, but we can as well be on the safe side from the
>> beginning.
> 
> OK.
> 
> I wonder how e.g. pthreads handles this? Is there some gcc provided
> #define which exposes the result of all these options so pthreads can
> DTRT with the stacks?

I have no idea, but would kind of expect user land stacks to start at
page (or at least cache line) boundaries anyway.

Jan