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.

Add unused output variables and fix the asm constraints in
run_thread().

Signed-off-by: Volker Rümelin <vr_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.

With best regards,
Volker

Separately, the rest of the patch series looks good to me.

-Kevin