Re: [Qemu-devel] [PATCH for 2.13 2/5] linux-user: remove unneeded #ifdef in signal.c

[Peter Maydell]

On 22 March 2018 at 21:58, Laurent Vivier <address@hidden> wrote:
> Signed-off-by: Laurent Vivier <address@hidden>
> ---
>  linux-user/signal.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 2c08ca14cf..514145b299 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -253,17 +253,15 @@ int do_sigprocmask(int how, const sigset_t *set, 
> sigset_t *oldset)
>      return 0;
>  }
>
> -#if !defined(TARGET_OPENRISC) && !defined(TARGET_NIOS2)
>  /* Just set the guest's signal mask to the specified value; the
>   * caller is assumed to have called block_signals() already.
>   */
> -static void set_sigmask(const sigset_t *set)
> +static __attribute__((unused)) void set_sigmask(const sigset_t *set)
>  {
>      TaskState *ts = (TaskState *)thread_cpu->opaque;
>
>      ts->signal_mask = *set;
>  }
> -#endif
>
>  /* siginfo conversion */
>
> @@ -533,8 +531,7 @@ static void force_sig(int sig)
>   * up the signal frame. oldsig is the signal we were trying to handle
>   * at the point of failure.
>   */
> -#if !defined(TARGET_RISCV)
> -static void force_sigsegv(int oldsig)
> +static __attribute__((unused)) void force_sigsegv(int oldsig)
>  {
>      if (oldsig == SIGSEGV) {
>          /* Make sure we don't try to deliver the signal again; this will
> @@ -545,8 +542,6 @@ static void force_sigsegv(int oldsig)
>      force_sig(TARGET_SIGSEGV);
>  }
>
> -#endif
> -
>  /* abort execution with signal */
>  static void QEMU_NORETURN dump_core_and_abort(int target_sig)
>  {

I don't think this is the right approach. In both of these
cases, the fact that the function is unused is a red flag that
there is a bug in the signal emulation code for those guest CPUs.
If the guest CPU code isn't calling set_sigmask() it is failing
to correctly handle the sigmask field in the ucontext on
signal return, and if it's not calling force_sigsegv() then
it's doing something wrong with the handling of "we couldn't
write to the signal frame" on signal entry.

I think it's worth keeping the #ifdef as a flag and
a list of what targets we need to fix.

(The "riscv isn't using force_sigsegv()" fix looks like it should be
trivial -- their setup_rt_frame() has the code path but it's
incorrectly trying to do this by hand rather than calling
force_sigsegv().)

thanks
-- PMM