On Wed, Jun 02, 2021 at 09:25:54PM +0200, Volker Rümelin wrote:
On Tue, Jun 01, 2021 at 09:06:54PM +0200, Volker Rümelin wrote:
The variables data, func, thread and cur are input operands for the asm statement in run_thread(). The assembly code clobbers these inputs.
From the gcc documentation chapter Extended-Asm, Input Operands:
"It is not possible to use clobbers to inform the compiler that the values in these inputs are changing. One common work-around is to tie the changing input variable to an output variable that never gets used."
Thanks. However, this change doesn't look correct to me. The variables data, func, thread and cur were all *output* operands. They were output operands that use "+" to indicate that they also have inputs associated with them. That is, unless I'm missing something, the asm already followed the gcc documentation (use an output operand and don't use the results of it).
Hi Kevin,
the "+" output constraint indicates that the assembly code uses the variable as input and updates the same variable.
The gcc manual does not say the compiler will not use the output operand result. It actually uses the updated variable.
Your code works because you never use the output variables.
Did you observe a problem during runtime with the original code?
My next patch does not work, because the compiler assumes edx is the updated variable cur at the end of the assembly code, but edx is actually clobbered. Just use a dprintf() to print cur before and after the asm statement to see I'm right. objdump -dS src/stacks.o will show the same.
Okay, I think I understand the issue. The asm constrains aren't wrong, but "cur" is clobbered by the asm. I think it's probably simpler to use this in the new code:
if (getCurThread() == &MainThread) check_irqs();
And do something similar in yield(). That is, not save/restore getCurThread() during stack switches. If it helps, we can also rename "cur" to "edx" to make it more clear that the C variable is clobbered.
Add unused output variables and fix the asm constraints in run_thread().
Signed-off-by: Volker Rümelinvr_qemu@t-online.de
src/stacks.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/stacks.c b/src/stacks.c index 2fe1bfb..ef0aba1 100644 --- a/src/stacks.c +++ b/src/stacks.c @@ -565,6 +565,7 @@ run_thread(void (*func)(void*), void *data) thread->stackpos = (void*)thread + THREADSTACKSIZE; struct thread_info *cur = getCurThread(); hlist_add_after(&thread->node, &cur->node);
- u32 unused_a, unused_b, unused_c, unused_d; asm volatile( // Start thread " pushl $1f\n" // store return pc
@@ -576,14 +577,15 @@ run_thread(void (*func)(void*), void *data) // End thread " movl %%ebx, %%eax\n" // %eax = thread " movl 4(%%ebx), %%ebx\n" // %ebx = thread->node.next
" movl (%5), %%esp\n" // %esp = MainThread.stackpos" calll %4\n" // call __end_thread(thread)
" movl (%9), %%esp\n" // %esp = MainThread.stackpos" calll %8\n" // call __end_thread(thread) " movl -4(%%ebx), %%esp\n" // %esp = next->stackpos " popl %%ebp\n" // restore %ebp " retl\n" // restore pc "1:\n"
: "+a"(data), "+c"(func), "+b"(thread), "+d"(cur): "m"(*(u8*)__end_thread), "m"(MainThread)
: "=a"(unused_a), "=c"(unused_c), "=b"(unused_b), "=d"(unused_d): "0"(data), "1"(func), "2"(thread), "3"(cur),"m"(*(u8*)__end_thread), "m"(MainThread)The original code made sure data was in eax, func in ecx, thread in ebx, and cur in edx. The altered code no longer enforces this association and I think that could introduce a bug.
The digit input constraints tie the variables to the correct register just like the old code. data, func, thread and cur are still in eax, ecx, ebx and edx at the beginning of the assembly code.
Sorry, you are correct, I missed the association. That said, I don't think we need to tell gcc to save/restore the original values during the stack switch as they can be easily recalculated.
Cheers, -Kevin