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ümelin<vr_q...@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 _______________________________________________ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org