Re: _lwp_create change

[christos%astron.com@localhost (Christos Zoulas)]

In article <20170420113134.GB23195%britannica.bec.de@localhost>,
Joerg Sonnenberger  <joerg%bec.de@localhost> wrote:
>On Thu, Apr 20, 2017 at 06:48:01AM +0200, Martin Husemann wrote:
>> On Thu, Apr 20, 2017 at 02:29:26AM +0200, Joerg Sonnenberger wrote:
>> > As discussed previously, I think the patch makes the behavior worse and
>> > the Go implementation is kind of stupid as well.
>> 
>> No comment on the patch of the go implementation, but I think that the
>> code should actually do what the documentation says:
>> 
>> 	The context argument specifies the
>>      initial execution context for the new LWP including signal mask, stack,
>>      and machine registers.
>> 
>> Specifying the signal stack in the create call does make sense to me.
>
>Note: stack here means the regular stack. It is not "signal mask, signal
>stack, stack, machine registers". Christos wants to overload the stack
>field to be polymorphic. I don't mind fixing the signal mask handling,
>that's a clear bug. Changing the stack to have a new double meaning is
>bad though.

It always has a double meaning... Check out the source in getucontext:

        /*
         * The (unsupplied) definition of the `current execution stack'
         * in the System V Interface Definition appears to allow returning
         * the main context stack.
         */
        if ((l->l_sigstk.ss_flags & SS_ONSTACK) == 0) { 
                ucp->uc_stack.ss_sp = (void *)l->l_proc->p_stackbase;
                ucp->uc_stack.ss_size = ctob(l->l_proc->p_vmspace->vm_ssize);
                ucp->uc_stack.ss_flags = 0;     /* XXX, def. is Very Fishy */
        } else {
                /* Simply copy alternate signal execution stack. */
                ucp->uc_stack = l->l_sigstk;
        }

ss_flags determines the meaning; note that XXX comment too: It means that
it is not SS_DISABLE, so it has *something*...

Now in the _lwp_create case the code is currently buggy; the stack supplied
by _lwp_makecontext is unused (and wrong as explained below). To take the
x86_64 case (_lwp_makecontext):

....
        void **sp;

        getcontext(u);
        u->uc_link = NULL;

	/*
         * BUG: the ss_flags are not being reset here. If we were using SS_ONSTACK
	 * this will be bogus, since it will be specifying the wrong stack.
         * Now this is completely unused currently (which is why it works :-);
	 * fixing it to use the proper stack here would probably mean to do
	 * something like:
	 *
	 *	if (uc->uc_stack.ss_flags & SS_ONSTACK) {
	 *		/* new thread needs its own signal stack */
	 *		uc->uc_stack.ss_sp = malloc(uc->uc_stack.ss_size);
	 *	} else {
	 *		/*
	 *		 * This is not used by anything; the stack is set below.
	 *		 * Perhaps if SS_DISABLE and set it to NULL/0?
	 *		 */
         *		u->uc_stack.ss_sp = stack_base;
         *		u->uc_stack.ss_size = stack_size;
	 *		u->uc_stack.ss_flags = 0; /* same as getucontext */
	 *	}
	 */
        u->uc_stack.ss_sp = stack_base;
        u->uc_stack.ss_size = stack_size;

        /* LINTED uintptr_t is safe */
        gr[_REG_RIP] = (uintptr_t)start;

        sp = (void **) (((uintptr_t)(stack_base + stack_size) & ~15));

        /* LINTED __greg_t is safe */
        gr[_REG_RDI] = (__greg_t)arg;
        *--sp = (void *) _lwp_exit;

	/*
	 * HERE: we set the stack: what is in uc_stack is currently irrelevant!
	 */
        /* LINTED uintptr_t is safe */
        gr[_REG_URSP] = (uintptr_t) sp;
....


christos