Re: [Qemu-devel] [PATCH 4/4] linux-user: Handle rt_sigaction correctly for SPARC

[Philippe Mathieu-Daudé]

Hi Peter,

On 11/06/2017 03:33 PM, Peter Maydell wrote:
> SPARC is like Alpha in its handling of the rt_sigaction syscall:
> it takes an extra parameter 'restorer' which needs to be copied
> into the sa_restorer field of the sigaction struct. The order
> of the arguments differs slightly between SPARC and Alpha but
> the implementation is otherwise the same. (Compare the
> rt_sigaction() functions in arch/sparc/kernel/sys_sparc_64.c
> and arch/alpha/kernel/signal.c.)
> 
> Note that this change is somewhat moot until SPARC acquires
> support for actually delivering RT signals.
> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  linux-user/syscall.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index d4497de..8beab51 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -8556,8 +8556,16 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>      case TARGET_NR_rt_sigaction:
>          {

> -            struct target_sigaction act, oact, *pact = 0;
> +            /* For Alpha and SPARC this is a 5 argument syscall, with
> +             * a 'restorer' parameter which must be copied into the
> +             * sa_restorer field of the sigaction struct.
> +             * For Alpha that 'restorer' is arg5; for SPARC it is arg4,
> +             * and arg5 is the sigsetsize.
> +             * Alpha also has a separate rt_sigaction struct that it uses
> +             * here; SPARC uses the usual sigaction struct.
> +             */

I find it easier to read moving the #if after the comment.
Since using restorer for sparc improve readability we can use it for
alpha as well (code is a bit more consistent):

  #if defined(TARGET_ALPHA)
               target_ulong restorer = arg5;

>              struct target_rt_sigaction *rt_act;
> +            struct target_sigaction act, oact, *pact = 0;
>  
>              if (arg4 != sizeof(target_sigset_t)) {
>                  ret = -TARGET_EINVAL;

...
                   act.sa_restorer = restorer;

Whichever way:
Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

> @@ -8583,18 +8591,29 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>                  unlock_user_struct(rt_act, arg3, 1);
>              }
>  #else
> +#ifdef TARGET_SPARC
> +            target_ulong restorer = arg4;
> +            target_ulong sigsetsize = arg5;
> +#else
> +            target_ulong sigsetsize = arg4;
> +#endif
>              struct target_sigaction *act;
>              struct target_sigaction *oact;
>  
> -            if (arg4 != sizeof(target_sigset_t)) {
> +            if (sigsetsize != sizeof(target_sigset_t)) {
>                  ret = -TARGET_EINVAL;
>                  break;
>              }
>              if (arg2) {
> -                if (!lock_user_struct(VERIFY_READ, act, arg2, 1))
> +                if (!lock_user_struct(VERIFY_READ, act, arg2, 1)) {
>                      goto efault;
> -            } else
> +                }
> +#ifdef TARGET_SPARC
> +                act->sa_restorer = restorer;
> +#endif
> +            } else {
>                  act = NULL;
> +            }
>              if (arg3) {
>                  if (!lock_user_struct(VERIFY_WRITE, oact, arg3, 0)) {
>                      ret = -TARGET_EFAULT;
>