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

[Arkadiusz Hiler]

On Mon, May 03, 2021 at 05:44:59PM +0200, Piotr Caban wrote:
> 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;

The return -1 here is my bad, because I've misread MSDN, but looks like
_beginthread() has a different behavior.

_beginthreadex() returns zero and is mentioned to set errno and
_doserrno (I'll include that in next revision) whithout any details.
> It would be nice to do similar changes in _beginthread() as well.

_beginthread() is documented to:
 * return -1L
 * set errno to EAGAIN in too many threads
 * EINVAL for invalid argument / stack size is incorrect
 * EACCESS if insufficient resources (e.g. memory)

I can clean it up a bit an make it more in line with what the spec says.