[PATCH 0/4] x86_64-gnu signal fixes

[PATCH 0/4] x86_64-gnu signal fixes

[Sergey Bugaev]

Hello,

now that we got enough of the system running on x86_64-gnu, we can
actually test the signal machinery. In fact we sort of *have to*,
because /bin/sh receives SIGCHLD when a command completes, and wants
to handle it.

Suprisingly, most of the logic actually does just work! -- but several
things were broken; and this patch series contains the fixes. With
these patches (+ the thread setup fixes that I'll send as a separate
series) I'm able to receive a signal, do all the trampoline and
intr-rpc magic, run the handler, and sigreturn back into user code --
apparently successfully.

Sergey

[PATCH 1/4] hurd: Fix aligning signal stack pointer

[Sergey Bugaev]

Fixes 60f9bf974694d50daf58d46347b06a5975ac5ddd
"hurd: Port trampoline.c to x86_64"

Checked on x86_64-gnu.

Reported-by: Bruno Haible <bruno@clisp.org>
Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 sysdeps/mach/hurd/x86/trampoline.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sysdeps/mach/hurd/x86/trampoline.c b/sysdeps/mach/hurd/x86/trampoline.c
index e13c5d85..19bddad8 100644
--- a/sysdeps/mach/hurd/x86/trampoline.c
+++ b/sysdeps/mach/hurd/x86/trampoline.c
@@ -200,7 +200,7 @@ _hurd_setup_sighandler (struct hurd_sigstate *ss, const struct sigaction *action
   /* Align SP at 16 bytes.  Coupled with the fact that sigreturn_addr is
      16-byte aligned within the stackframe struct, this ensures that it ends
      up on a 16-byte aligned address, as required by the ABI.  */
-  sigsp = (void *) ((uintptr_t) sigsp & 16UL);
+  sigsp = (void *) ((uintptr_t) sigsp & ~15UL);
 #endif
 
   /* Push the arguments to call `trampoline' on the stack.  */
-- 
2.40.1

[PATCH 2/4] hurd: Align signal stack pointer after allocating stackframe

[Sergey Bugaev]

sizeof (*stackframe) appears to be divisible by 16, but we should not
rely on that. So make sure to leave enough space for the stackframe
first, and then align the final pointer at 16 bytes.

Checked on x86_64-gnu.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 sysdeps/mach/hurd/x86/trampoline.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/sysdeps/mach/hurd/x86/trampoline.c b/sysdeps/mach/hurd/x86/trampoline.c
index 19bddad8..1f92064e 100644
--- a/sysdeps/mach/hurd/x86/trampoline.c
+++ b/sysdeps/mach/hurd/x86/trampoline.c
@@ -196,15 +196,14 @@ _hurd_setup_sighandler (struct hurd_sigstate *ss, const struct sigaction *action
 #endif
     }
 
+  /* Push the arguments to call `trampoline' on the stack.  */
+  sigsp -= sizeof (*stackframe);
 #ifdef __x86_64__
   /* Align SP at 16 bytes.  Coupled with the fact that sigreturn_addr is
      16-byte aligned within the stackframe struct, this ensures that it ends
      up on a 16-byte aligned address, as required by the ABI.  */
   sigsp = (void *) ((uintptr_t) sigsp & ~15UL);
 #endif
-
-  /* Push the arguments to call `trampoline' on the stack.  */
-  sigsp -= sizeof (*stackframe);
   stackframe = sigsp;
 
   if (_hurdsig_catch_memory_fault (stackframe))
-- 
2.40.1

[PATCH 3/4] hurd: Fix sc_i386_thread_state layout

[Sergey Bugaev]

The real i386_thread_state Mach structure has an alignment of 8 on
x86_64. However, in struct sigcontext, the compiler was packing sc_gs
(which is the first member of sc_i386_thread_state) into the same 8-byte
slot as sc_error; this resulted in the rest of sc_i386_thread_state
members having wrong offsets relative to each other, and the overall
sc_i386_thread_state layout mismatching that of i386_thread_state.

Fix this by explicitly adding the required padding members, and
statically asserting that this results in the desired alignment.

The same goes for sc_i386_float_state.

Checked on x86_64-gnu.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 sysdeps/mach/hurd/x86/trampoline.c         | 6 ++++++
 sysdeps/mach/hurd/x86_64/bits/sigcontext.h | 8 ++++++++
 2 files changed, 14 insertions(+)

diff --git a/sysdeps/mach/hurd/x86/trampoline.c b/sysdeps/mach/hurd/x86/trampoline.c
index 1f92064e..6318c952 100644
--- a/sysdeps/mach/hurd/x86/trampoline.c
+++ b/sysdeps/mach/hurd/x86/trampoline.c
@@ -242,11 +242,17 @@ _hurd_setup_sighandler (struct hurd_sigstate *ss, const struct sigaction *action
 
       /* struct sigcontext is laid out so that starting at sc_gs mimics a
 	 struct i386_thread_state.  */
+      _Static_assert (offsetof (struct sigcontext, sc_i386_thread_state)
+		      % __alignof__ (struct i386_thread_state) == 0,
+		      "sc_i386_thread_state layout mismatch");
       memcpy (&scp->sc_i386_thread_state,
 	      &state->basic, sizeof (state->basic));
 
       /* struct sigcontext is laid out so that starting at sc_fpkind mimics
 	 a struct i386_float_state.  */
+      _Static_assert (offsetof (struct sigcontext, sc_i386_float_state)
+		      % __alignof__ (struct i386_float_state) == 0,
+		      "sc_i386_float_state layout mismatch");
       ok = machine_get_state (ss->thread, state, i386_FLOAT_STATE,
 			      &state->fpu, &scp->sc_i386_float_state,
 			      sizeof (state->fpu));
diff --git a/sysdeps/mach/hurd/x86_64/bits/sigcontext.h b/sysdeps/mach/hurd/x86_64/bits/sigcontext.h
index 3a3b34bc..63960544 100644
--- a/sysdeps/mach/hurd/x86_64/bits/sigcontext.h
+++ b/sysdeps/mach/hurd/x86_64/bits/sigcontext.h
@@ -46,6 +46,11 @@ struct sigcontext
     /* Error code associated with this signal (interpreted as `error_t').  */
     int sc_error;
 
+    /* Make sure the below members are properly aligned, and not packed
+       together with sc_error -- otherwise the layout won't match that of
+       i386_thread_state.  */
+    int sc_pad1;
+
     /* All following members are machine-dependent.  The rest of this
        structure is written to be laid out identically to:
        {
@@ -86,6 +91,9 @@ struct sigcontext
     long sc_ursp;		/* This stack pointer is used.  */
     int sc_ss;			/* Stack segment register.  */
 
+    /* Make sure the below has the same layout as i386_float_state.  */
+    int sc_pad2;
+
     /* Following mimics struct i386_float_state.  Structures and symbolic
        values can be found in <mach/i386/fp_reg.h>.  */
 #define sc_i386_float_state sc_fpkind
-- 
2.40.1

[PATCH 4/4] hurd: Fix computing user stack pointer

[Sergey Bugaev]

Fixes b574ae0a2876ee94e4fe617f878407bf818c2df0
"hurd: Implement sigreturn for x86_64"

Checked on x86_64-gnu.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 sysdeps/mach/hurd/x86_64/sigreturn.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sysdeps/mach/hurd/x86_64/sigreturn.c b/sysdeps/mach/hurd/x86_64/sigreturn.c
index 82247e3c..5d3a4d31 100644
--- a/sysdeps/mach/hurd/x86_64/sigreturn.c
+++ b/sysdeps/mach/hurd/x86_64/sigreturn.c
@@ -126,7 +126,7 @@ __sigreturn (struct sigcontext *scp)
        copy the registers onto the user's stack, switch there, pop and
        return.  */
 
-    uintptr_t *usp = (uintptr_t *) scp->sc_ursp - 128;
+    uintptr_t *usp = (uintptr_t *) (scp->sc_ursp - 128);
 
     *--usp = scp->sc_rip;
     *--usp = scp->sc_rfl;
-- 
2.40.1

Re: [PATCH 1/4] hurd: Fix aligning signal stack pointer

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev via Libc-alpha, le lun. 15 mai 2023 11:33:20 +0300, a ecrit:
> Fixes 60f9bf974694d50daf58d46347b06a5975ac5ddd
> "hurd: Port trampoline.c to x86_64"
> 
> Checked on x86_64-gnu.
> 
> Reported-by: Bruno Haible <bruno@clisp.org>
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  sysdeps/mach/hurd/x86/trampoline.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sysdeps/mach/hurd/x86/trampoline.c b/sysdeps/mach/hurd/x86/trampoline.c
> index e13c5d85..19bddad8 100644
> --- a/sysdeps/mach/hurd/x86/trampoline.c
> +++ b/sysdeps/mach/hurd/x86/trampoline.c
> @@ -200,7 +200,7 @@ _hurd_setup_sighandler (struct hurd_sigstate *ss, const struct sigaction *action
>    /* Align SP at 16 bytes.  Coupled with the fact that sigreturn_addr is
>       16-byte aligned within the stackframe struct, this ensures that it ends
>       up on a 16-byte aligned address, as required by the ABI.  */
> -  sigsp = (void *) ((uintptr_t) sigsp & 16UL);
> +  sigsp = (void *) ((uintptr_t) sigsp & ~15UL);
>  #endif
>  
>    /* Push the arguments to call `trampoline' on the stack.  */
> -- 
> 2.40.1
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.

Re: [PATCH 2/4] hurd: Align signal stack pointer after allocating stackframe

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev via Libc-alpha, le lun. 15 mai 2023 11:33:21 +0300, a ecrit:
> sizeof (*stackframe) appears to be divisible by 16, but we should not
> rely on that. So make sure to leave enough space for the stackframe
> first, and then align the final pointer at 16 bytes.
> 
> Checked on x86_64-gnu.
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  sysdeps/mach/hurd/x86/trampoline.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/sysdeps/mach/hurd/x86/trampoline.c b/sysdeps/mach/hurd/x86/trampoline.c
> index 19bddad8..1f92064e 100644
> --- a/sysdeps/mach/hurd/x86/trampoline.c
> +++ b/sysdeps/mach/hurd/x86/trampoline.c
> @@ -196,15 +196,14 @@ _hurd_setup_sighandler (struct hurd_sigstate *ss, const struct sigaction *action
>  #endif
>      }
>  
> +  /* Push the arguments to call `trampoline' on the stack.  */
> +  sigsp -= sizeof (*stackframe);
>  #ifdef __x86_64__
>    /* Align SP at 16 bytes.  Coupled with the fact that sigreturn_addr is
>       16-byte aligned within the stackframe struct, this ensures that it ends
>       up on a 16-byte aligned address, as required by the ABI.  */
>    sigsp = (void *) ((uintptr_t) sigsp & ~15UL);
>  #endif
> -
> -  /* Push the arguments to call `trampoline' on the stack.  */
> -  sigsp -= sizeof (*stackframe);
>    stackframe = sigsp;
>  
>    if (_hurdsig_catch_memory_fault (stackframe))
> -- 
> 2.40.1
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.

Re: [PATCH 3/4] hurd: Fix sc_i386_thread_state layout

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le lun. 15 mai 2023 11:33:22 +0300, a ecrit:
> The real i386_thread_state Mach structure has an alignment of 8 on
> x86_64. However, in struct sigcontext, the compiler was packing sc_gs
> (which is the first member of sc_i386_thread_state) into the same 8-byte
> slot as sc_error; this resulted in the rest of sc_i386_thread_state
> members having wrong offsets relative to each other, and the overall
> sc_i386_thread_state layout mismatching that of i386_thread_state.
> 
> Fix this by explicitly adding the required padding members, and
> statically asserting that this results in the desired alignment.
> 
> The same goes for sc_i386_float_state.
> 
> Checked on x86_64-gnu.
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  sysdeps/mach/hurd/x86/trampoline.c         | 6 ++++++
>  sysdeps/mach/hurd/x86_64/bits/sigcontext.h | 8 ++++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/sysdeps/mach/hurd/x86/trampoline.c b/sysdeps/mach/hurd/x86/trampoline.c
> index 1f92064e..6318c952 100644
> --- a/sysdeps/mach/hurd/x86/trampoline.c
> +++ b/sysdeps/mach/hurd/x86/trampoline.c
> @@ -242,11 +242,17 @@ _hurd_setup_sighandler (struct hurd_sigstate *ss, const struct sigaction *action
>  
>        /* struct sigcontext is laid out so that starting at sc_gs mimics a
>  	 struct i386_thread_state.  */
> +      _Static_assert (offsetof (struct sigcontext, sc_i386_thread_state)
> +		      % __alignof__ (struct i386_thread_state) == 0,
> +		      "sc_i386_thread_state layout mismatch");
>        memcpy (&scp->sc_i386_thread_state,
>  	      &state->basic, sizeof (state->basic));
>  
>        /* struct sigcontext is laid out so that starting at sc_fpkind mimics
>  	 a struct i386_float_state.  */
> +      _Static_assert (offsetof (struct sigcontext, sc_i386_float_state)
> +		      % __alignof__ (struct i386_float_state) == 0,
> +		      "sc_i386_float_state layout mismatch");
>        ok = machine_get_state (ss->thread, state, i386_FLOAT_STATE,
>  			      &state->fpu, &scp->sc_i386_float_state,
>  			      sizeof (state->fpu));
> diff --git a/sysdeps/mach/hurd/x86_64/bits/sigcontext.h b/sysdeps/mach/hurd/x86_64/bits/sigcontext.h
> index 3a3b34bc..63960544 100644
> --- a/sysdeps/mach/hurd/x86_64/bits/sigcontext.h
> +++ b/sysdeps/mach/hurd/x86_64/bits/sigcontext.h
> @@ -46,6 +46,11 @@ struct sigcontext
>      /* Error code associated with this signal (interpreted as `error_t').  */
>      int sc_error;
>  
> +    /* Make sure the below members are properly aligned, and not packed
> +       together with sc_error -- otherwise the layout won't match that of
> +       i386_thread_state.  */
> +    int sc_pad1;
> +
>      /* All following members are machine-dependent.  The rest of this
>         structure is written to be laid out identically to:
>         {
> @@ -86,6 +91,9 @@ struct sigcontext
>      long sc_ursp;		/* This stack pointer is used.  */
>      int sc_ss;			/* Stack segment register.  */
>  
> +    /* Make sure the below has the same layout as i386_float_state.  */
> +    int sc_pad2;
> +
>      /* Following mimics struct i386_float_state.  Structures and symbolic
>         values can be found in <mach/i386/fp_reg.h>.  */
>  #define sc_i386_float_state sc_fpkind
> -- 
> 2.40.1
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.

Re: [PATCH 4/4] hurd: Fix computing user stack pointer

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le lun. 15 mai 2023 11:33:23 +0300, a ecrit:
> Fixes b574ae0a2876ee94e4fe617f878407bf818c2df0
> "hurd: Implement sigreturn for x86_64"
> 
> Checked on x86_64-gnu.
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  sysdeps/mach/hurd/x86_64/sigreturn.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sysdeps/mach/hurd/x86_64/sigreturn.c b/sysdeps/mach/hurd/x86_64/sigreturn.c
> index 82247e3c..5d3a4d31 100644
> --- a/sysdeps/mach/hurd/x86_64/sigreturn.c
> +++ b/sysdeps/mach/hurd/x86_64/sigreturn.c
> @@ -126,7 +126,7 @@ __sigreturn (struct sigcontext *scp)
>         copy the registers onto the user's stack, switch there, pop and
>         return.  */
>  
> -    uintptr_t *usp = (uintptr_t *) scp->sc_ursp - 128;
> +    uintptr_t *usp = (uintptr_t *) (scp->sc_ursp - 128);
>  
>      *--usp = scp->sc_rip;
>      *--usp = scp->sc_rfl;
> -- 
> 2.40.1
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.

Re: [PATCH 1/4] hurd: Fix aligning signal stack pointer

[Adhemerval Zanella Netto]

On 15/05/23 13:27, Samuel Thibault via Libc-alpha wrote:
> Applied, thanks!
> 
> Sergey Bugaev via Libc-alpha, le lun. 15 mai 2023 11:33:20 +0300, a ecrit:
>> Fixes 60f9bf974694d50daf58d46347b06a5975ac5ddd
>> "hurd: Port trampoline.c to x86_64"
>>
>> Checked on x86_64-gnu.
>>
>> Reported-by: Bruno Haible <bruno@clisp.org>
>> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
>> ---
>>  sysdeps/mach/hurd/x86/trampoline.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/sysdeps/mach/hurd/x86/trampoline.c b/sysdeps/mach/hurd/x86/trampoline.c
>> index e13c5d85..19bddad8 100644
>> --- a/sysdeps/mach/hurd/x86/trampoline.c
>> +++ b/sysdeps/mach/hurd/x86/trampoline.c
>> @@ -200,7 +200,7 @@ _hurd_setup_sighandler (struct hurd_sigstate *ss, const struct sigaction *action
>>    /* Align SP at 16 bytes.  Coupled with the fact that sigreturn_addr is
>>       16-byte aligned within the stackframe struct, this ensures that it ends
>>       up on a 16-byte aligned address, as required by the ABI.  */
>> -  sigsp = (void *) ((uintptr_t) sigsp & 16UL);
>> +  sigsp = (void *) ((uintptr_t) sigsp & ~15UL);


Btw, we have the PTR_ALIGN_DOWN macro for this.