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<[email protected]>
> > > ---
> > > 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 -- [email protected]
To unsubscribe send an email to [email protected]
