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

[Emil Velikov]

On 19 August 2014 16:51, Jose Fonseca <jfonseca at vmware.com> wrote:
> On 19/08/14 16:41, Jose Fonseca wrote:
>>
>> 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.
>
>
> This is the problem:
>
>         thrd_join(threads[i], (int *) &exit_codes[i]);
>
> a bool takes one byte, where as int takes four.
>
> If the cast wasn't here the compiler would have warned about the type
> mismtach.
>
Indeed that's the one. Which begs the question why we don't crash on
linux but I'll leave that one for some other time.
There seems to be another booger a few lines above

a->thread_id = i;

The former is int, while the latter is intptr_t.

Thank you Jose, I owe you one :)
-Emil

> Jose
>
> _______________________________________________
> waffle mailing list
> waffle at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/waffle