[Mesa-dev] [PATCH 1/2] c11/threads: implement thrd_current on Windows

[Jose Fonseca]

I reread http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf and 
is not explicit.

But I checked C11 threads.h implementation and thrd_current also uses 
DuplicateHandle.

So C11/threads_win32.h is correct.  And ISO C11 standard commited 
screwed up here.  Maybe they'll introduce thread ids  at some point 
(MSVC's implementation has a non-standard _Thrd_id() function), and we 
can use it, but until then, it's better not pretend that thrd_current 
meets our needs because it doesn't.


Jose


On 21/04/17 15:06, Jose Fonseca wrote:
> No, I'm afraid the problem is still there AFAICS.
>
> GetCurrentThread returns a magic constant (-2 if I'm not mistaken).  So
> it is (and always will be) useless for thrd_current.
>
> Imagine that:
>
> - thread #1 invokes thrd_current() and gets -2
> - thread #2 invokes thrd_current() and gets -2
> - a thread (#1, #2 or another thrad) receives invokes thread_equal(-2, -2)
>
>
> The only way this would work is if it would be forbidden to pass the
> value of thrd_current to other threads.  I'll need to re-read the specs,
> but my guess is that it would be pretty useless.
>
> I also doubt the way mapi uses would work.
>
>
> Furthermore map really wants a fast thread id.  It doesn't need actual
> thread handles, just some way of telling threads apart, any id.
>
>
> Jose
>
>
> On 20/04/17 15:15, Emil Velikov wrote:
>> From: Emil Velikov <emil.velikov at collabora.com>
>>
>> Earlier commit removed the implementation due to a subtle bug, in the
>> existing code. Namely:
>>
>> One was comparing directly the thread handle/IDs rather than using
>> thrd_equal(). As the pseudo-handles never matched things went south.
>>
>> That bug was resolved with commit 458c7490c29 "mapi: rewrite
>> u_current_init() function without u_thread_self()" thus we can bring the
>> thrd_current implementation back, whist preserving the correct
>> thrd_equal implementation as-is.
>>
>> With this in place, we can remove a handful of the remaining pthread vs
>> WIN32 specifics.
>>
>> Cc: Brian Paul <brianp at vmware.com>
>> Cc: José Fonseca <jfonseca at vmware.com>
>> Signed-off-by: Emil Velikov <emil.l.velikov at gmail.com>
>> ---
>>  include/c11/threads_win32.h | 35 ++++++-----------------------------
>>  1 file changed, 6 insertions(+), 29 deletions(-)
>>
>> diff --git a/include/c11/threads_win32.h b/include/c11/threads_win32.h
>> index d017c31c34e..f0eb5c77c60 100644
>> --- a/include/c11/threads_win32.h
>> +++ b/include/c11/threads_win32.h
>> @@ -494,42 +494,19 @@ thrd_create(thrd_t *thr, thrd_start_t func, void
>> *arg)
>>      return thrd_success;
>>  }
>>
>> -#if 0
>>  // 7.25.5.2
>>  static inline thrd_t
>>  thrd_current(void)
>>  {
>> -    HANDLE hCurrentThread;
>> -    BOOL bRet;
>> -
>> -    /* GetCurrentThread() returns a pseudo-handle, which is useless.
>> We need
>> -     * to call DuplicateHandle to get a real handle.  However the
>> handle value
>> -     * will not match the one returned by thread_create.
>> -     *
>> -     * Other potential solutions would be:
>> -     * - define thrd_t as a thread Ids, but this would mean we'd need
>> to OpenThread for many operations
>> -     * - use malloc'ed memory for thrd_t. This would imply using TLS
>> for current thread.
>> -     *
>> -     * Neither is particularly nice.
>> +    /* GetCurrentThread() returns a pseudo-handle, which will not
>> match the
>> +     * one returned by thread_create.
>>       *
>> -     * Life would be much easier if C11 threads had different
>> abstractions for
>> -     * threads and thread IDs, just like C++11 threads does...
>> +     * At the same time, the only way that one should compare thread
>> handle
>> +     * and/or IDs is via thrd_equal. That in itself attributes for
>> the above
>> +     * situation.
>>       */
>> -
>> -    bRet = DuplicateHandle(GetCurrentProcess(), // source process
>> (pseudo) handle
>> -                           GetCurrentThread(), // source (pseudo) handle
>> -                           GetCurrentProcess(), // target process
>> -                           &hCurrentThread, // target handle
>> -                           0,
>> -                           FALSE,
>> -                           DUPLICATE_SAME_ACCESS);
>> -    assert(bRet);
>> -    if (!bRet) {
>> -    hCurrentThread = GetCurrentThread();
>> -    }
>> -    return hCurrentThread;
>> +    return GetCurrentThread();
>>  }
>> -#endif
>>
>>  // 7.25.5.3
>>  static inline int
>>
>