[Qemu-devel] [PATCH] coroutine: Fix win32 variant for older mingw32 compilers

[Qemu-devel] [PATCH] coroutine: Fix win32 variant for older mingw32 compilers

[Jan Kiszka]

mingw32 with gcc up to (at least) 4.4 has broken __thread support.
This means all __thread variables silently become global ones.

Address this by switching the coroutine implementation for win32 to
Get/SetTlsValue.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 coroutine-win32.c |   29 ++++++++++++++++++++++-------
 1 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/coroutine-win32.c b/coroutine-win32.c
index 4179609..bf0aeac 100644
--- a/coroutine-win32.c
+++ b/coroutine-win32.c
@@ -33,8 +33,7 @@ typedef struct
     CoroutineAction action;
 } CoroutineWin32;
 
-static __thread CoroutineWin32 leader;
-static __thread Coroutine *current;
+static int current_tls_index = TLS_OUT_OF_INDEXES;
 
 CoroutineAction qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
                                       CoroutineAction action)
@@ -42,7 +41,7 @@ CoroutineAction qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
     CoroutineWin32 *from = DO_UPCAST(CoroutineWin32, base, from_);
     CoroutineWin32 *to = DO_UPCAST(CoroutineWin32, base, to_);
 
-    current = to_;
+    TlsSetValue(current_tls_index, to_);
 
     to->action = action;
     SwitchToFiber(to->fiber);
@@ -79,14 +78,30 @@ void qemu_coroutine_delete(Coroutine *co_)
 
 Coroutine *qemu_coroutine_self(void)
 {
-    if (!current) {
-        current = &leader.base;
-        leader.fiber = ConvertThreadToFiber(NULL);
+    CoroutineWin32 *leader;
+
+    if (current_tls_index == TLS_OUT_OF_INDEXES) {
+        current_tls_index = TlsAlloc();
+        if (current_tls_index == TLS_OUT_OF_INDEXES) {
+            fprintf(stderr, "qemu: %s: out of TLS handles\n", __func__);
+            abort();
+        }
+    }
+    if (!TlsGetValue(current_tls_index)) {
+        leader = g_malloc0(sizeof(*leader));
+        leader->fiber = ConvertThreadToFiber(NULL);
+        TlsSetValue(current_tls_index, &leader->base);
     }
-    return current;
+    return TlsGetValue(current_tls_index);
 }
 
 bool qemu_in_coroutine(void)
 {
+    Coroutine *current;
+
+    if (current_tls_index == TLS_OUT_OF_INDEXES) {
+        return false;
+    }
+    current = TlsGetValue(current_tls_index);
     return current && current->caller;
 }
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH] coroutine: Fix win32 variant for older mingw32 compilers

[Paolo Bonzini]

Il 21/11/2012 15:00, Jan Kiszka ha scritto:
> +    if (!TlsGetValue(current_tls_index)) {
> +        leader = g_malloc0(sizeof(*leader));
> +        leader->fiber = ConvertThreadToFiber(NULL);
> +        TlsSetValue(current_tls_index, &leader->base);
>      }

Leaking leader is a bit bad, but it looks ok for 1.3.

For 1.4 we really should bring back a portable solution for TLS that
includes statically-allocated data.

Paolo

Re: [Qemu-devel] [PATCH] coroutine: Fix win32 variant for older mingw32 compilers

[Jan Kiszka]

On 2012-11-21 15:08, Paolo Bonzini wrote:
> Il 21/11/2012 15:00, Jan Kiszka ha scritto:
>> +    if (!TlsGetValue(current_tls_index)) {
>> +        leader = g_malloc0(sizeof(*leader));
>> +        leader->fiber = ConvertThreadToFiber(NULL);
>> +        TlsSetValue(current_tls_index, &leader->base);
>>      }
> 
> Leaking leader is a bit bad, but it looks ok for 1.3.

Hmm. A TLS destructor is apparently not available. Is there some "on
thread termination" callback mechanism on Windows? Didn't find one on
first glance.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] coroutine: Fix win32 variant for older mingw32 compilers

[malc]

On Wed, 21 Nov 2012, Jan Kiszka wrote:

> On 2012-11-21 15:08, Paolo Bonzini wrote:
> > Il 21/11/2012 15:00, Jan Kiszka ha scritto:
> >> +    if (!TlsGetValue(current_tls_index)) {
> >> +        leader = g_malloc0(sizeof(*leader));
> >> +        leader->fiber = ConvertThreadToFiber(NULL);
> >> +        TlsSetValue(current_tls_index, &leader->base);
> >>      }
> > 
> > Leaking leader is a bit bad, but it looks ok for 1.3.
> 
> Hmm. A TLS destructor is apparently not available. Is there some "on
> thread termination" callback mechanism on Windows? Didn't find one on
> first glance.
> 

Dlls receive something like THREAD_DETTACH in it's startup routine or
something like that if my memory serves me.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] coroutine: Fix win32 variant for older mingw32 compilers

[Paolo Bonzini]

Il 21/11/2012 15:33, malc ha scritto:
>>> > > Leaking leader is a bit bad, but it looks ok for 1.3.
>> > 
>> > Hmm. A TLS destructor is apparently not available. Is there some "on
>> > thread termination" callback mechanism on Windows? Didn't find one on
>> > first glance.
>> > 
> Dlls receive something like THREAD_DETTACH in it's startup routine or
> something like that if my memory serves me.

Only DLLs.

But this sounds like deja-vu.  I'm pretty sure in the past we just
decided that this compiler is not supported (of course it's bad that
it's silent).  Stefan, do you remember the details?

Paolo

Re: [Qemu-devel] [PATCH] coroutine: Fix win32 variant for older mingw32 compilers

[Jan Kiszka]

On 2012-11-21 15:38, Paolo Bonzini wrote:
> Il 21/11/2012 15:33, malc ha scritto:
>>>>>> Leaking leader is a bit bad, but it looks ok for 1.3.
>>>>
>>>> Hmm. A TLS destructor is apparently not available. Is there some "on
>>>> thread termination" callback mechanism on Windows? Didn't find one on
>>>> first glance.
>>>>
>> Dlls receive something like THREAD_DETTACH in it's startup routine or
>> something like that if my memory serves me.
> 
> Only DLLs.
> 
> But this sounds like deja-vu.  I'm pretty sure in the past we just
> decided that this compiler is not supported (of course it's bad that
> it's silent).  Stefan, do you remember the details?

Current Debian delivers 4.4-based mingw unfortunately.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] coroutine: Fix win32 variant for older mingw32 compilers

[Jan Kiszka]

On 2012-11-21 15:49, Jan Kiszka wrote:
> On 2012-11-21 15:38, Paolo Bonzini wrote:
>> Il 21/11/2012 15:33, malc ha scritto:
>>>>>>> Leaking leader is a bit bad, but it looks ok for 1.3.
>>>>>
>>>>> Hmm. A TLS destructor is apparently not available. Is there some "on
>>>>> thread termination" callback mechanism on Windows? Didn't find one on
>>>>> first glance.
>>>>>
>>> Dlls receive something like THREAD_DETTACH in it's startup routine or
>>> something like that if my memory serves me.
>>
>> Only DLLs.
>>
>> But this sounds like deja-vu.  I'm pretty sure in the past we just
>> decided that this compiler is not supported (of course it's bad that
>> it's silent).  Stefan, do you remember the details?
> 
> Current Debian delivers 4.4-based mingw unfortunately.

I think we practically do not leak, at least as long as we continue to
use coroutines only over cpu and iothread context. Those threads stay as
long as qemu is running. And to my understanding, those contexts are the
only target of coroutines anyway. Anything that already uses its own
proper threads has no need for this problematic concept, no?

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] coroutine: Fix win32 variant for older mingw32 compilers

[Paolo Bonzini]

Il 21/11/2012 16:44, Jan Kiszka ha scritto:
>>>>>>>> Leaking leader is a bit bad, but it looks ok for 1.3.
>>>>>> >>>>>
>>>>>> >>>>> Hmm. A TLS destructor is apparently not available. Is there some "on
>>>>>> >>>>> thread termination" callback mechanism on Windows? Didn't find one on
>>>>>> >>>>> first glance.
>>>>>> >>>>>
>>>> >>> Dlls receive something like THREAD_DETTACH in it's startup routine or
>>>> >>> something like that if my memory serves me.
>>> >>
>>> >> Only DLLs.
>>> >>
>>> >> But this sounds like deja-vu.  I'm pretty sure in the past we just
>>> >> decided that this compiler is not supported (of course it's bad that
>>> >> it's silent).  Stefan, do you remember the details?
>> > 
>> > Current Debian delivers 4.4-based mingw unfortunately.
> I think we practically do not leak, at least as long as we continue to
> use coroutines only over cpu and iothread context. Those threads stay as
> long as qemu is running. And to my understanding, those contexts are the
> only target of coroutines anyway. Anything that already uses its own
> proper threads has no need for this problematic concept, no?

Kind of... when Stefan (Hajnoczi) finishes the full version of
virtio-blk dataplane, there will be one thread per device running
coroutines.

But it's still a minor leak, it's ok for 1.3 and we can get it right
later using the Windows run-time linker's TLS support, like on Linux.

Paolo

Re: [Qemu-devel] [PATCH] coroutine: Fix win32 variant for older mingw32 compilers

[Jan Kiszka]

On 2012-11-21 16:54, Paolo Bonzini wrote:
> Il 21/11/2012 16:44, Jan Kiszka ha scritto:
>>>>>>>>> Leaking leader is a bit bad, but it looks ok for 1.3.
>>>>>>>>>>>>
>>>>>>>>>>>> Hmm. A TLS destructor is apparently not available. Is there some "on
>>>>>>>>>>>> thread termination" callback mechanism on Windows? Didn't find one on
>>>>>>>>>>>> first glance.
>>>>>>>>>>>>
>>>>>>>> Dlls receive something like THREAD_DETTACH in it's startup routine or
>>>>>>>> something like that if my memory serves me.
>>>>>>
>>>>>> Only DLLs.
>>>>>>
>>>>>> But this sounds like deja-vu.  I'm pretty sure in the past we just
>>>>>> decided that this compiler is not supported (of course it's bad that
>>>>>> it's silent).  Stefan, do you remember the details?
>>>>
>>>> Current Debian delivers 4.4-based mingw unfortunately.
>> I think we practically do not leak, at least as long as we continue to
>> use coroutines only over cpu and iothread context. Those threads stay as
>> long as qemu is running. And to my understanding, those contexts are the
>> only target of coroutines anyway. Anything that already uses its own
>> proper threads has no need for this problematic concept, no?
> 
> Kind of... when Stefan (Hajnoczi) finishes the full version of
> virtio-blk dataplane, there will be one thread per device running
> coroutines.
> 
> But it's still a minor leak, it's ok for 1.3 and we can get it right
> later using the Windows run-time linker's TLS support, like on Linux.

So it's a non-leak for current QEMU. :)

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] coroutine: Fix win32 variant for older mingw32 compilers

[Stefan Weil]

Am 21.11.2012 15:38, schrieb Paolo Bonzini:
> Il 21/11/2012 15:33, malc ha scritto:
>>>>>> Leaking leader is a bit bad, but it looks ok for 1.3.
>>>> Hmm. A TLS destructor is apparently not available. Is there some "on
>>>> thread termination" callback mechanism on Windows? Didn't find one on
>>>> first glance.
>>>>
>> Dlls receive something like THREAD_DETTACH in it's startup routine or
>> something like that if my memory serves me.
> Only DLLs.
>
> But this sounds like deja-vu.  I'm pretty sure in the past we just
> decided that this compiler is not supported (of course it's bad that
> it's silent).  Stefan, do you remember the details?
>
> Paolo

Debian cross works with -mthread.The issue was discussed here:

https://bugs.launchpad.net/qemu/+bug/932487

Jan, I don't think your patch should be applied.

Current MinGW / MinGW-w64 compilers work, so those users
which compile and use QEMU on Windows won't have a problem.
With MinGW-w64, it is even possible to compile QEMU with nearly
no warnings :-)

Debian cross development is full of difficulties. Passing an extra
compiler option like -mthread is only one of these difficulties.
I updated http://wiki.qemu.org/Hosts/W32, so anybody who really
wants to run cross compilations on Debian can get more information
there.

We could add a check to configure and add -mthread automatically.
Up to now, there was no consensus whether this is wanted because
-mthread adds a library to QEMU's dependencies.

Stefan

Re: [Qemu-devel] [PATCH] coroutine: Fix win32 variant for older mingw32 compilers

[Paolo Bonzini]

Il 21/11/2012 20:11, Stefan Weil ha scritto:
> 
> Debian cross development is full of difficulties. Passing an extra
> compiler option like -mthread is only one of these difficulties.
> I updated http://wiki.qemu.org/Hosts/W32, so anybody who really
> wants to run cross compilations on Debian can get more information
> there.

Thanks very much!

Paolo

Re: [Qemu-devel] [PATCH] coroutine: Fix win32 variant for older mingw32 compilers

[Jan Kiszka]

On 2012-11-21 20:11, Stefan Weil wrote:
> Am 21.11.2012 15:38, schrieb Paolo Bonzini:
>> Il 21/11/2012 15:33, malc ha scritto:
>>>>>>> Leaking leader is a bit bad, but it looks ok for 1.3.
>>>>> Hmm. A TLS destructor is apparently not available. Is there some "on
>>>>> thread termination" callback mechanism on Windows? Didn't find one on
>>>>> first glance.
>>>>>
>>> Dlls receive something like THREAD_DETTACH in it's startup routine or
>>> something like that if my memory serves me.
>> Only DLLs.
>>
>> But this sounds like deja-vu.  I'm pretty sure in the past we just
>> decided that this compiler is not supported (of course it's bad that
>> it's silent).  Stefan, do you remember the details?
>>
>> Paolo
> 
> Debian cross works with -mthread.The issue was discussed here:

Yep, works here as well.

With this knowledge, why not convert the TLS usage in
qemu-thread-win32.c as well? That's what motivated me to go this path.

> 
> https://bugs.launchpad.net/qemu/+bug/932487
> 
> Jan, I don't think your patch should be applied.

I agree, but we need a different solution for this subtle failure.

> 
> Current MinGW / MinGW-w64 compilers work, so those users
> which compile and use QEMU on Windows won't have a problem.
> With MinGW-w64, it is even possible to compile QEMU with nearly
> no warnings :-)
> 
> Debian cross development is full of difficulties. Passing an extra
> compiler option like -mthread is only one of these difficulties.
> I updated http://wiki.qemu.org/Hosts/W32, so anybody who really
> wants to run cross compilations on Debian can get more information
> there.
> 
> We could add a check to configure and add -mthread automatically.
> Up to now, there was no consensus whether this is wanted because
> -mthread adds a library to QEMU's dependencies.

As the alternative to this is a crashing QEMU, I doubt there is much to
discuss.

Do we know which mingw version is fine without -mthreads? Or do we have
to test this during configure?

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux