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). Did you observe a problem during runtime with the original code? > 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. Separately, the rest of the patch series looks good to me. -Kevin _______________________________________________ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
