[waffle] [PATCH 4/4] wgl: attempt to fix the final test

[Jose Fonseca]

On 19/08/14 14:43, Emil Velikov wrote:
> On 19/08/14 14:34, Emil Velikov wrote:
>> On 19/08/14 13:42, Jose Fonseca wrote:
>>> On 12/08/14 16:37, Emil Velikov wrote:
>>>> MSVC helps us out with the final test by undicating that we're
>>>> corrupting the stack, which begs the question - at which point are we
>>>> messing up with the calling conventions. This patch attempts to resolve
>>>> that yet the bug still persists :'(
>>>>
>>>> Signed-off-by: Emil Velikov <emil.l.velikov at gmail.com>
>>>> ---
>>>>    src/waffle/core/wcore_error_unittest.c | 4 ++++
>>>>    third_party/threads/threads.h          | 2 +-
>>>>    2 files changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/waffle/core/wcore_error_unittest.c
>>>> b/src/waffle/core/wcore_error_unittest.c
>>>> index 5176031..8b9b334 100644
>>>> --- a/src/waffle/core/wcore_error_unittest.c
>>>> +++ b/src/waffle/core/wcore_error_unittest.c
>>>> @@ -148,7 +148,11 @@ struct thread_arg {
>>>>    };
>>>>
>>>>    /// The start routine given to threads in test wcore_error.thread_local.
>>>> +#if !defined(_WIN32)
>>>>    static bool
>>>> +#else
>>>> +static bool __stdcall
>>>> +#endif
>>>>    thread_start(struct thread_arg *a)
>>>>    {
>>>>        static const enum waffle_error error_codes[NUM_THREADS] = {
>>>> diff --git a/third_party/threads/threads.h b/third_party/threads/threads.h
>>>> index 4e7dba2..eb024dd 100644
>>>> --- a/third_party/threads/threads.h
>>>> +++ b/third_party/threads/threads.h
>>>> @@ -117,7 +117,7 @@ typedef pthread_once_t  once_flag;
>>>>
>>>>    /*---------------------------- types ----------------------------*/
>>>>    typedef void (*tss_dtor_t)(void*);
>>>> -typedef int (*thrd_start_t)(void*);
>>>> +typedef int (__stdcall *thrd_start_t)(void*);
>>>>
>>>>    struct xtime {
>>>>        time_t sec;
>>>>
>>>
>>>
>>> Sorry, I've been on PTO and I haven't caught up with email.
>>>
>>> What is the problem here again?
>>>
>> We end up with stack corruption in test_wcore_error_thread_local().
>>
>> The documentation indicates [1] that the function pointer passed to
>> _beginthreadex (in our third_party/threads code) should use __stdcall calling
>> convention. I'm not entirely sure which/how many pieces we need to annotate
>> due to the amount of function pointers passed around :'(
>>
> Actually _beginthreadex() is correctly setup, yet pack.func(pack.arg) (from
> impl_thrd_routine) may not be. So many functions passing around func pointers
> is making my head spin.
>
> You should be able to reproduce by building waffle and giving 'make check'* a try.
>

I will try to repro.

But this patch doesn't look right, because we don't call beginthreadex 
with the thrd_start_t pointer.  Instead we call _beginthreadex( 
impl_thrd_routine), which does have the __stdcall already.


BTW, this code is being used in llvmpipe's multi-threaded rasterized and 
it is known to work.

I suspect the problem is elsewhere.


Jose