Re: [PATCH 2/4] msvcrt: Use trampoline for _beginthreadex().

[Piotr Caban]

Hi Arek,

On 5/3/21 3:44 PM, Arkadiusz Hiler wrote:
> +/*********************************************************************
> -
> -
> _beginthreadex_trampoline
> - */
>
> +static DWORD CALLBACK _beginthreadex_trampoline(LPVOID arg)
> +{
> - unsigned int retval;
> - _beginthread_trampoline_t local_trampoline;
> - thread_data_t *data = msvcrt_get_thread_data();
> -
> - memcpy(&local_trampoline,arg,sizeof(local_trampoline));

Please add spaces between arguments.
> /*********************************************************************
> -
> _beginthreadex (MSVCRT.@)
>
> */
> @@ -157,12 +177,30 @@ uintptr_t CDECL _beginthreadex(
>     unsigned int initflag,   /* [in] Initial state of new thread (0 for running or CREATE_SUSPEND for suspended) */
>     unsigned int *thrdaddr)  /* [out] Points to a 32-bit variable that receives the thread identifier */
>   {
> - _beginthread_trampoline_t* trampoline;
> - HANDLE thread;
> - TRACE("(%p, %d, %p, %p, %d, %p)\n", security, stack_size, start_address, arglist, initflag, thrdaddr);
> - /* FIXME */
> - return (uintptr_t)CreateThread(security, stack_size,
> -
> 		     start_address, arglist,
> -
> 		     initflag, thrdaddr);
> - trampoline = malloc(sizeof(*trampoline));
> - if(!trampoline) {
> -
>  *_errno() = EAGAIN;
> -
>  return -1;

malloc already sets errno. Resetting it to EAGAIN looks questionable. 
The function should also return 0 on error.
> - trampoline->thread = INVALID_HANDLE_VALUE;
> - trampoline->start_address_ex = start_address;
> - trampoline->arglist = arglist;
> -
> - thread = CreateThread(security, stack_size, _beginthreadex_trampoline,
> -
>      trampoline, initflag, thrdaddr);
> - if(!thread) {
> -
>  free(trampoline);
> -
>  *_errno() = EAGAIN;
> -
>  return -1;

It should probably do something like:
msvcrt_set_errno(GetLastError());
return 0;

It would be nice to do similar changes in _beginthread() as well.

Thanks,
Piotr