This closes #11. Support building the 'sim' with optimization levels higher than -O0. The higher optimization levels emit useful warnings that are masked at -O0.
The basic problem with the implementation of 'os_arch_task_stack_init()' is that we are switching stacks from underneath the compiler without any ability to influence how the compiler uses the stack. Thus when setjmp() returns the compiler doesn't know that the stack frame it allocated for local variables doesn't exist anymore on the new stack. This happens to work with the -O0 optimization level because the compiler addresses local variables using the frame pointer (%ebp). Higher optimization levels use offsets from the stack pointer and immediately expose the issue mentioned above. Fix this by switching stacks in 'os_arch_frame_init()' which is implemented in assembly so we can control the use of stack after setjmp() returns. Project: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-larva/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-larva/commit/6440a4aa Tree: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-larva/tree/6440a4aa Diff: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-larva/diff/6440a4aa Branch: refs/heads/master Commit: 6440a4aa3043a2c0985050dfb996d391f7beccc4 Parents: 9717c79 Author: Neel Natu <n...@nahannisys.com> Authored: Fri Mar 4 18:13:55 2016 -0800 Committer: wes3 <w...@micosa.io> Committed: Sun Mar 6 11:05:19 2016 -0800 ---------------------------------------------------------------------- compiler/sim/pkg.yml | 7 ++- libs/os/src/arch/sim/os_arch_sim.c | 84 ++++++++++++------------- libs/os/src/arch/sim/os_arch_stack_frame.s | 68 ++++++++++++++++++++ libs/util/include/util/util.h | 2 + 4 files changed, 113 insertions(+), 48 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-mynewt-larva/blob/6440a4aa/compiler/sim/pkg.yml ---------------------------------------------------------------------- diff --git a/compiler/sim/pkg.yml b/compiler/sim/pkg.yml index de62ed9..c9ace8f 100644 --- a/compiler/sim/pkg.yml +++ b/compiler/sim/pkg.yml @@ -29,15 +29,16 @@ pkg.keywords: - gcc compiler.path.cc: "/usr/local/bin/gcc-5" +compiler.path.as: "/usr/local/bin/gcc-5 -x assembler-with-cpp" compiler.path.archive: "ar" compiler.path.objdump: "gobjdump" compiler.path.objsize: "objsize" compiler.path.objcopy: "gobjcopy" compiler.flags.base: > - -m32 -Wall -Werror -ggdb -O0 -DMN_OSX + -m32 -Wall -Werror -ggdb -DMN_OSX -compiler.flags.default: [compiler.flags.base] -compiler.flags.debug: [compiler.flags.base, -ggdb -O0] +compiler.flags.default: [compiler.flags.base, -O1] +compiler.flags.debug: [compiler.flags.base, -O0] compiler.ld.mapfile: false http://git-wip-us.apache.org/repos/asf/incubator-mynewt-larva/blob/6440a4aa/libs/os/src/arch/sim/os_arch_sim.c ---------------------------------------------------------------------- diff --git a/libs/os/src/arch/sim/os_arch_sim.c b/libs/os/src/arch/sim/os_arch_sim.c index d22f8a2..03975ce 100644 --- a/libs/os/src/arch/sim/os_arch_sim.c +++ b/libs/os/src/arch/sim/os_arch_sim.c @@ -31,19 +31,24 @@ #include <signal.h> #include <sys/time.h> #include <assert.h> +#include <util/util.h> -/* XXX: - * Stack must be 16-byte aligned, any changes to this structure - * must keep that alignment. - */ struct stack_frame { + int sf_mainsp; /* stack on which main() is executing */ jmp_buf sf_jb; int sf_sigsblocked; - os_task_func_t sf_func; - void *sf_arg; - int _pad[3]; + struct os_task *sf_task; }; +/* + * Assert that 'sf_mainsp' and 'sf_jb' are at the specific offsets where + * os_arch_frame_init() expects them to be. + */ +CTASSERT(offsetof(struct stack_frame, sf_mainsp) == 0); +CTASSERT(offsetof(struct stack_frame, sf_jb) == 4); + +extern void os_arch_frame_init(struct stack_frame *sf); + #define CTX_SWITCH_ISR (1) #define CTX_SWITCH_TASK (2) @@ -86,54 +91,43 @@ sigs_block(void) sigprocmask(SIG_BLOCK, &g_sigset, NULL); } -os_stack_t * -os_arch_task_stack_init(struct os_task *t, os_stack_t *stack_top, int size) +/* + * Called from 'os_arch_frame_init()' when setjmp returns indirectly via + * longjmp. The return value of setjmp is passed to this function as 'rc'. + */ +void +os_arch_task_start(struct stack_frame *sf, int rc) { - struct os_task *cur_t; - struct stack_frame *sf; - os_stack_t *s; - void *s_cur; - volatile int block_isr_off; - int rc; + struct os_task *task; - s = (os_stack_t *) ((uint8_t *) stack_top - sizeof(*sf)); - sf = (struct stack_frame *) s; - sf->sf_sigsblocked = 0; - sf->sf_func = t->t_func; - sf->sf_arg = t->t_arg; + task = sf->sf_task; - block_isr_off = g_block_isr_off; + isr_state(&g_block_isr_off, NULL); - /* Save the current stack pointer, and then trick setjmp() - * to store the stackos stack pointer. - */ - asm("movl %%esp, %0" : "=r" (s_cur)); - asm("movl %0,%%esp" : /* no output */ : "r" (s)); + if (rc == CTX_SWITCH_ISR) { + sigs_unblock(); + } - isr_state(&g_block_isr_on, NULL); + task->t_func(task->t_arg); - rc = sim_setjmp(sf->sf_jb); - if (rc != 0) { - cur_t = os_sched_get_current_task(); - - isr_state(&block_isr_off, NULL); + /* This should never return */ + assert(0); +} - if (rc == CTX_SWITCH_ISR) { - sigs_unblock(); - } +os_stack_t * +os_arch_task_stack_init(struct os_task *t, os_stack_t *stack_top, int size) +{ + struct stack_frame *sf; - sf = (struct stack_frame *) cur_t->t_stackptr; - sf->sf_func(sf->sf_arg); + sf = (struct stack_frame *) ((uint8_t *) stack_top - sizeof(*sf)); + sf->sf_sigsblocked = 0; + sf->sf_task = t; - /* This should never return */ - assert(0); - } else { - /* restore current stack pointer */ - isr_state(&g_block_isr_off, NULL); - asm("movl %0, %%esp" : /* no output */ : "r" (s_cur)); - } + isr_state(&g_block_isr_on, NULL); + os_arch_frame_init(sf); + isr_state(&g_block_isr_off, NULL); - return (s); + return ((os_stack_t *)sf); } void http://git-wip-us.apache.org/repos/asf/incubator-mynewt-larva/blob/6440a4aa/libs/os/src/arch/sim/os_arch_stack_frame.s ---------------------------------------------------------------------- diff --git a/libs/os/src/arch/sim/os_arch_stack_frame.s b/libs/os/src/arch/sim/os_arch_stack_frame.s new file mode 100644 index 0000000..91c9338 --- /dev/null +++ b/libs/os/src/arch/sim/os_arch_stack_frame.s @@ -0,0 +1,68 @@ + .text + .code32 + .p2align 4, 0x90 /* align on 16-byte boundary and fill with NOPs */ + + .globl _os_arch_frame_init + /* + * void os_arch_frame_init(struct stack_frame *sf) + */ +_os_arch_frame_init: + push %ebp /* function prologue for backtrace */ + mov %esp,%ebp + push %esi /* save %esi before using it as a tmpreg */ + + /* + * At this point we are executing on the main() stack: + * ---------------- + * stack_frame ptr 0xc(%esp) + * ---------------- + * return address 0x8(%esp) + * ---------------- + * saved ebp 0x4(%esp) + * ---------------- + * saved esi 0x0(%esp) + * ---------------- + */ + movl 0xc(%esp),%esi /* %esi = 'sf' */ + movl %esp,0x0(%esi) /* sf->mainsp = %esp */ + + /* + * Switch the stack so the stack pointer stored in 'sf->sf_jb' points + * to the task stack. This is slightly complicated because OS X wants + * the incoming stack pointer to be 16-byte aligned. + * + * ---------------- + * sf (other fields) + * ---------------- + * sf (sf_jb) 0x4(%esi) + * ---------------- + * sf (sf_mainsp) 0x0(%esi) + * ---------------- + * alignment padding variable (0 to 12 bytes) + * ---------------- + * pointer to sf_jb %esp + * ---------------- + */ + movl %esi,%esp + subl $0x4,%esp /* make room for setjmp() argument */ + andl $0xfffffff0,%esp /* align %esp on 16-byte boundary */ + leal 0x4(%esi),%eax /* %eax = &sf->sf_jb */ + movl %eax,0x0(%esp) + call __setjmp /* _setjmp(sf->sf_jb) */ + test %eax,%eax + jne 1f + movl 0x0(%esi),%esp /* switch back to the main() stack */ + pop %esi + pop %ebp + ret /* return to os_arch_task_stack_init() */ +1: + lea 2f,%ecx + push %ecx /* retaddr */ + push $0 /* frame pointer */ + movl %esp,%ebp /* handcrafted prologue for backtrace */ + push %eax /* rc */ + push %esi /* sf */ + call _os_arch_task_start /* os_arch_task_start(sf, rc) */ + /* never returns */ +2: + nop http://git-wip-us.apache.org/repos/asf/incubator-mynewt-larva/blob/6440a4aa/libs/util/include/util/util.h ---------------------------------------------------------------------- diff --git a/libs/util/include/util/util.h b/libs/util/include/util/util.h index 6b3dd8b..1dcdab4 100644 --- a/libs/util/include/util/util.h +++ b/libs/util/include/util/util.h @@ -19,4 +19,6 @@ #ifndef __UTIL_H__ #define __UTIL_H__ +#define CTASSERT(x) typedef int __ctasssert ## __LINE__[(x) ? 1 : -1] + #endif /* __UTIL_H__ */
