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

[Jose Fonseca]

On 19/08/14 18:44, Emil Velikov wrote:
> 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

I'm sure it overflows on Linux too. valgrind might complain.

But MSVC emits code to check for stack overflow -- some sort of canary. 
  There is no "crash" per se -- but an error dialog.

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

Right, it will be truncated. There should be no overflow here though.

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

You're welcome!

Jose