Re: [PATCH v2 1/4] hurd: Simplify init-first.c further

[Sergey Bugaev <bugaevc@gmail.com>]

On Thu, Feb 23, 2023 at 2:26 AM Samuel Thibault <samuel.thibault@gnu.org> wrote:
> Did you try to run make check?

No.

I'm cross-compiling, so I don't think make check would be able to run
any tests. And from what I remember from building glibc on the Hurd
itself back in 2021, make check takes a very long time and either
never really completes or brings the system into some weird state.

If you're able to run make check on your end, please do so (but wait
until I send v3 with the changes you've requested below).

Are there specific tests for the various combinations of startup
variants? (shared vs static, args already on the stack vs not, exec
server present vs not) If so, maybe I could run just them and not the
full thing; that would be easier on my system.

> Sergey Bugaev via Libc-alpha, le mer. 22 févr. 2023 00:19:29 +0300, a ecrit:
> > This drops all of the return address rewriting kludges. The only
> > remaining hack is the jump out of a call stack while adjusting the
> > stack pointer.
>
> Is this hack really still needed? Since we don't switch stack any more,
> we could as well just return normally?

No, we can't just return normally, we have to adjust the stack pointer
and jump out. We don't _switch_ the stack as in use an entirely
different area of memory for the stack, but we do adjust the stack
pointer within the existing stack area.

Are the comments I have added not clear enough about this? If so,
maybe they should be expanded further. The reason we have to return in
this weird way is that:
1. The normal startup code (_start1) expects to find args/env on the
top of the stack. Like, literally, argc at sp[0], argv[0] at sp[1] and
so on.
2. _hurd_startup, which is the code that receives args/env from the
exec server, allocates them in its own stack frame, with alloca. So it
cannot return, and neither can _hurd_stack_setup.
Instead of returning, _hurd_startup invokes a callback (doinit) that
(eventually) just sets the stack pointer to point to this data (so it
now is on the top of the stack, just as _start1 expects) and jumps to
_hurd_stack_setup's caller (i.e. to _start). This would very much
break things if _start was itself using the stack for anything, since
the stack pointer is now different, but the only thing _start does is
it calls _hurd_startup and then jumps to _start1, so that works.

And the same is done in the SHARED startup path by _dl_sysdep_start,
except that it uses the RETURN_TO macro instead of direct inline
assembly. That macro only really differs in that it does *not* zero
out ebp/rbp, so I wonder how come that doesn't break backtraces.

> > --- a/sysdeps/mach/hurd/dl-sysdep.c
> > +++ b/sysdeps/mach/hurd/dl-sysdep.c
> > @@ -207,6 +207,9 @@ _dl_sysdep_start (void **start_argptr,
> >           }
> >       }
> >
> > +      extern void _dl_init_first (void *data);
>
> Please put extern function declaration into a header, dl-sysdep.h for
> instance.

Makes sense, thanks.

>
> > diff --git a/sysdeps/mach/hurd/i386/init-first.c b/sysdeps/mach/hurd/i386/init-first.c
> > index a558da16..34e8dcc0 100644
> > --- a/sysdeps/mach/hurd/i386/init-first.c
> > +++ b/sysdeps/mach/hurd/i386/init-first.c
> > +  {
> > +    /* Check if the stack we are now on is different from
> > +       the one described by _hurd_stack_{base,size}.  */
> >
> > +    char dummy;
> > +    const vm_address_t newsp = (vm_address_t) &dummy;
> > +
> > +    if (d->stack_size != 0 && (newsp < d->stack_base
> > +                            || newsp - d->stack_base > d->stack_size))
> > +      /* The new stack pointer does not intersect with the
> > +      stack the exec server set up for us, so free that stack.  */
> > +      __vm_deallocate (__mach_task_self (), d->stack_base, d->stack_size);
> > +  }
>
> Again, I don't think this is needed any more since we don't switch stack
> any more?

Good point, most likely not. I'll drop it and see if anything breaks.

Sergey