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."

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)
         : "esi", "edi", "cc", "memory");
     return;
 
-- 
2.26.2

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

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_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.

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-leave@seabios.org

> 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

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org