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

Laurent Vivier posted 5 patches 7 years, 7 months ago
There is a newer version of this series
[Qemu-devel] [PATCH for 2.13 2/5] linux-user: remove unneeded #ifdef in signal.c
Posted by Laurent Vivier 7 years, 7 months ago
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 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)
 {
-- 
2.14.3


Re: [Qemu-devel] [PATCH for 2.13 2/5] linux-user: remove unneeded #ifdef in signal.c
Posted by Peter Maydell 7 years, 7 months ago
On 22 March 2018 at 21:58, Laurent Vivier <laurent@vivier.eu> wrote:
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  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