Re: struct sigcontext in Hurd/x86_64

[Bruno Haible]

Hello Sergey,

> > * glibc/sysdeps/mach/hurd/x86_64/bits/sigcontext.h lines 57..79
> > * glibc/sysdeps/mach/hurd/x86/trampoline.c lines 239..247.
> >   This code copies the values from the stack into a 'struct sigcontext'.
> >   But here the order of the registers is
> 
> No: trampoline.c copies the values of registers from the Mach thread
> state, as returned by thread_get_state (). There's no way that glibc
> could, from the userland, directly get at the values on the kernel
> stack.
> 
> thread_get_state () returns the i386_thread_state structure defined in
> gnumach i386/include/mach/i386/thread_status.h:
> ...
> The conversion between i386_saved_state and i386_thread_state is not a
> direct memcpy, but a bunch of explicit assignments (see
> i386_thread_state:thread_getstatus):
> ...
> 
> So there's no bug here.

Thanks for explaining. I now understand it better, and am fixing the
comments in my code (see below).

But another thing appears to be wrong: The role of sc_rsp versus sc_ursp
in glibc/sysdeps/mach/hurd/x86_64/bits/sigcontext.h.

What glibc/sysdeps/mach/hurd/x86/trampoline.c does for x86_64 is:

_hurd_setup_sighandler (...)
{
...
  state->basic.rsp = state->basic.ursp;
...
  sigsp = <appropriate new stack pointer for invoking the signal handler>;
...
  /* Push the arguments to call `trampoline' on the stack.  */
  sigsp -= sizeof (*stackframe);
...
      state->basic.ursp = (uintptr_t) sigsp;
...
}

But glibc/sysdeps/mach/hurd/x86_64/bits/sigcontext.h has these comments:

struct sigcontext
  {
...
    long sc_rsp;                /* Not used; sc_ursp is used instead.  */
...
    long sc_ursp;               /* This stack pointer is used.  */


A signal handler that a programmer has written and included in his application
needs to know about the %rsp value at the moment the signal occurred. The
value of the %rsp at the moment the signal handler is invoked is pointless
because
  - the signal handler can access it as &some_local_variable anyway,
  - it may be located on the signal stack, while the %rsp value at the moment
    the signal occurred may not have been on the signal stack. That is, the
    two values are far away.

So, given the code in trampoline.c shown above, the comments in sigcontext.h
are wrong and make the programmer use the wrong value.

To correct this, IMO:

  * Either all uses of basic.rsp and basic.ursp in trampoline.c, except in
    line 177, need to be swapped. In particular in line 73.

  * Or the comment in sigcontext.h need to be fixed:

    long sc_rsp;                /* The stack pointer when the signal occurred.  
*/
...
    long sc_ursp;               /* The stack pointer set up for the signal 
handler.  */

    and likewise in glibc/sysdeps/mach/hurd/i386/bits/sigcontext.h.


Bruno


diff --git a/src/fault-hurd-i386.h b/src/fault-hurd-i386.h
index 639ec7c..83693ec 100644
--- a/src/fault-hurd-i386.h
+++ b/src/fault-hurd-i386.h
@@ -21,12 +21,27 @@
 
 /* scp points to a 'struct sigcontext' (defined in
    glibc/sysdeps/mach/hurd/x86_64/bits/sigcontext.h).
-   The registers, at the moment the signal occurred, get pushed on the stack
-   through gnumach/x86_64/locore.S:alltraps and then copied into the struct
-   through glibc/sysdeps/mach/hurd/x86/trampoline.c.  */
-/* sc_rsp is unused (not set by gnumach/x86_64/locore.S:alltraps).  We need
-   to use sc_ursp.  */
-# define SIGSEGV_FAULT_STACKPOINTER  scp->sc_ursp
+   The registers, at the moment the signal occurred, get pushed on the kernel
+   stack through gnumach/x86_64/locore.S:alltraps. They are denoted by a
+   'struct i386_saved_state' (defined in gnumach/i386/i386/thread.h).
+   Upon invocation of the Mach interface function thread_get_state
+   <https://www.gnu.org/software/hurd/gnumach-doc/Thread-Execution.html>
+   (= __thread_get_state in glibc), defined in gnumach/kern/thread.c,
+   the function thread_getstatus, defined in gnumach/i386/i386/pcb.c, copies 
the
+   register values in a different arrangement into a 'struct 
i386_thread_state',
+   defined in gnumach/i386/include/mach/i386/thread_status.h. (Different
+   arrangement: trapno, err get dropped; different order of r8...r15; also rsp
+   gets set to 0.)
+   This 'struct i386_thread_state' is actually the 'basic' part of a
+   'struct machine_thread_all_state', defined in
+   glibc/sysdeps/mach/x86/thread_state.h.
+   From there, the function _hurd_setup_sighandler, defined in
+   glibc/sysdeps/mach/hurd/x86/trampoline.c, copies it into the appropriate
+   part of a 'struct sigcontext', defined in
+   glibc/sysdeps/mach/hurd/x86_64/bits/sigcontext.h.  */
+/* The way glibc/sysdeps/mach/hurd/x86/trampoline.c is written, we need to use
+   sc_rsp, not sc_ursp.  */
+# define SIGSEGV_FAULT_STACKPOINTER  scp->sc_rsp
 
 #else
 /* 32 bit registers */