[Qemu-devel] [PATCH] linux-user: Enable sigaltstack syscall for sh4

[Qemu-devel] [PATCH] linux-user: Enable sigaltstack syscall for sh4

[John Paul Adrian Glaubitz]

Hello!

I'm the current maintainer for the Debian sh4 port. I maintain several
buildds as well as port packages for the sh4 architecture.

Recently I discovered qemu for building packages in an emulated chroot
environment with qemu-sh4-static. While setting up python, qemu issued
an error message about an unimplemented syscall, 186 which is sigaltstack.

Looking up the sources, I found that linux-user/syscall.c enables
sigaltstack for a limited number of architectures only. In order to fix
the problem with the missing sigaltstack syscall, I added TARGET_SH4
to the list of architectures which seems to work.

The following sample C program tests the functionality of sigaltstack.
Without my patch, the program segfaults on qemu-sh4. With the patch,
it works as expected.

=====================================================================

#include <setjmp.h>
#include <signal.h>
#include <stdlib.h>
#include <stdio.h>

jmp_buf exit_jmp;

void handler(int x)
{
  longjmp(exit_jmp, 1);
}

int f(void)
{
  return f();
}

int main(void)
{
  stack_t sigstack;
  sigstack.ss_sp = malloc(1024*1024);
  sigstack.ss_size = 1024*1024;
  sigstack.ss_flags = 0;
  sigaltstack(&sigstack, NULL);
  struct sigaction sa;
  sa.sa_handler = handler;
  sigemptyset(&sa.sa_mask);
  sa.sa_flags = SA_ONSTACK;
  sigaction(SIGSEGV, &sa, NULL);
  if (setjmp(exit_jmp) == 0)
  {
    return f();
  }
  puts("recovered");
  return 0;
}

=====================================================================

Any idea what else I should test to verify my changes? Otherwise, it
would be great if my patch could be applied to add sigaltstack for
sh4.

Thanks,

Adrian

> [1] https://wiki.debian.org/SH4/sbuildQEMU

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

[Qemu-devel] [PATCH] linux-user: Enable sigaltstack syscall for sh4

[John Paul Adrian Glaubitz]

---
 linux-user/syscall.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 6c64ba6..4d864b2 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8294,7 +8294,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
     case TARGET_NR_sigaltstack:
 #if defined(TARGET_I386) || defined(TARGET_ARM) || defined(TARGET_MIPS) || \
     defined(TARGET_SPARC) || defined(TARGET_PPC) || defined(TARGET_ALPHA) || \
-    defined(TARGET_M68K) || defined(TARGET_S390X) || defined(TARGET_OPENRISC)
+    defined(TARGET_M68K) || defined(TARGET_S390X) || defined(TARGET_OPENRISC) || \
+    defined(TARGET_SH4)
         ret = do_sigaltstack(arg1, arg2, get_sp_from_cpustate((CPUArchState *)cpu_env));
         break;
 #else
-- 
2.6.2

Re: [Qemu-devel] [PATCH] linux-user: Enable sigaltstack syscall for sh4

[Peter Maydell]

On 18 November 2015 at 22:15, John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> ---
>  linux-user/syscall.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 6c64ba6..4d864b2 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -8294,7 +8294,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>      case TARGET_NR_sigaltstack:
>  #if defined(TARGET_I386) || defined(TARGET_ARM) || defined(TARGET_MIPS) || \
>      defined(TARGET_SPARC) || defined(TARGET_PPC) || defined(TARGET_ALPHA) || \
> -    defined(TARGET_M68K) || defined(TARGET_S390X) || defined(TARGET_OPENRISC)
> +    defined(TARGET_M68K) || defined(TARGET_S390X) || defined(TARGET_OPENRISC) || \
> +    defined(TARGET_SH4)
>          ret = do_sigaltstack(arg1, arg2, get_sp_from_cpustate((CPUArchState *)cpu_env));
>          break;
>  #else

Unfortunately this isn't sufficient. You also need to add
the code to the sh4-specific functions in linux-user/signal.c
which honours the requested sigaltstack when taking and returning
from signal handlers.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] linux-user: Enable sigaltstack syscall for sh4

[John Paul Adrian Glaubitz]

On 11/19/2015 10:17 AM, Peter Maydell wrote:
> Unfortunately this isn't sufficient. You also need to add
> the code to the sh4-specific functions in linux-user/signal.c
> which honours the requested sigaltstack when taking and returning
> from signal handlers.

My supplied test case shows that sigaltstack works unless I am
overseeing anything? Laurent Vivier (CC'ed) who has done some
extensive qemu development thinks that my change should be enough.

Here's the output of my test case (CC'ing Michael Karcher who
suggested the test case):

(sid-sh4-sbuild)root@jessie32:/tmp# cat stackoverflow.c

#include <setjmp.h>
#include <signal.h>
#include <stdlib.h>
#include <stdio.h>

jmp_buf exit_jmp;

void handler(int x)
{
  longjmp(exit_jmp, 1);
}

int f(void)
{
  return f();
}

int main(void)
{
  stack_t sigstack;
  sigstack.ss_sp = malloc(1024*1024);
  sigstack.ss_size = 1024*1024;
  sigstack.ss_flags = 0;
  sigaltstack(&sigstack, NULL);
  struct sigaction sa;
  sa.sa_handler = handler;
  sigemptyset(&sa.sa_mask);
  sa.sa_flags = SA_ONSTACK;
  sigaction(SIGSEGV, &sa, NULL);
  if (setjmp(exit_jmp) == 0)
    {
      return f();
    }
  puts("recovered");
  return 0;
}
(sid-sh4-sbuild)root@jessie32:/tmp# gcc stackoverflow.c -o stackoverflow
(sid-sh4-sbuild)root@jessie32:/tmp# ./stackoverflow
recovered
(sid-sh4-sbuild)root@jessie32:/tmp#

Now commenting "sigaltstack" out:

(sid-sh4-sbuild)root@jessie32:/tmp# cat stackoverflow.c

#include <setjmp.h>
#include <signal.h>
#include <stdlib.h>
#include <stdio.h>

jmp_buf exit_jmp;

void handler(int x)
{
  longjmp(exit_jmp, 1);
}

int f(void)
{
  return f();
}

int main(void)
{
  stack_t sigstack;
  sigstack.ss_sp = malloc(1024*1024);
  sigstack.ss_size = 1024*1024;
  sigstack.ss_flags = 0;
  // sigaltstack(&sigstack, NULL);
  struct sigaction sa;
  sa.sa_handler = handler;
  sigemptyset(&sa.sa_mask);
  sa.sa_flags = SA_ONSTACK;
  sigaction(SIGSEGV, &sa, NULL);
  if (setjmp(exit_jmp) == 0)
    {
      return f();
    }
  puts("recovered");
  return 0;
}
(sid-sh4-sbuild)root@jessie32:/tmp# gcc stackoverflow.c -o stackoverflow
(sid-sh4-sbuild)root@jessie32:/tmp# ./stackoverflow
qemu: uncaught target signal 11 (Segmentation fault) - core dumped
Segmentation fault
(sid-sh4-sbuild)root@jessie32:/tmp#

Thus, for me it seems sigaltstack behaves as expected with the patch
applied.

Am I missing something obvious?

Cheers,
Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [Qemu-devel] [PATCH] linux-user: Enable sigaltstack syscall for sh4

[Laurent Vivier]

Hi,

On 19/11/2015 10:28, John Paul Adrian Glaubitz wrote:
> On 11/19/2015 10:17 AM, Peter Maydell wrote:
>> Unfortunately this isn't sufficient. You also need to add
>> the code to the sh4-specific functions in linux-user/signal.c
>> which honours the requested sigaltstack when taking and returning
>> from signal handlers.

it seems all needed functions for sh4 signal handling are already
written in linux-user/signal.c, I thing about setup_frame(),
setup_rt_frame(), do_sigreturn() and do_rt_sigreturn().

Do we need more ?

> My supplied test case shows that sigaltstack works unless I am
> overseeing anything? Laurent Vivier (CC'ed) who has done some
> extensive qemu development thinks that my change should be enough.
> 
> Here's the output of my test case (CC'ing Michael Karcher who
> suggested the test case):
> 
> (sid-sh4-sbuild)root@jessie32:/tmp# cat stackoverflow.c
> 
> #include <setjmp.h>
> #include <signal.h>
> #include <stdlib.h>
> #include <stdio.h>
> 
> jmp_buf exit_jmp;
> 
> void handler(int x)
> {
>   longjmp(exit_jmp, 1);
> }
> 
> int f(void)
> {
>   return f();
> }
> 
> int main(void)
> {
>   stack_t sigstack;
>   sigstack.ss_sp = malloc(1024*1024);
>   sigstack.ss_size = 1024*1024;
>   sigstack.ss_flags = 0;
>   sigaltstack(&sigstack, NULL);
>   struct sigaction sa;
>   sa.sa_handler = handler;
>   sigemptyset(&sa.sa_mask);
>   sa.sa_flags = SA_ONSTACK;
>   sigaction(SIGSEGV, &sa, NULL);
>   if (setjmp(exit_jmp) == 0)
>     {
>       return f();
>     }
>   puts("recovered");
>   return 0;
> }
> (sid-sh4-sbuild)root@jessie32:/tmp# gcc stackoverflow.c -o stackoverflow
> (sid-sh4-sbuild)root@jessie32:/tmp# ./stackoverflow
> recovered
> (sid-sh4-sbuild)root@jessie32:/tmp#
> 
> Now commenting "sigaltstack" out:
> 
> (sid-sh4-sbuild)root@jessie32:/tmp# cat stackoverflow.c
> 
> #include <setjmp.h>
> #include <signal.h>
> #include <stdlib.h>
> #include <stdio.h>
> 
> jmp_buf exit_jmp;
> 
> void handler(int x)
> {
>   longjmp(exit_jmp, 1);
> }
> 
> int f(void)
> {
>   return f();
> }
> 
> int main(void)
> {
>   stack_t sigstack;
>   sigstack.ss_sp = malloc(1024*1024);
>   sigstack.ss_size = 1024*1024;
>   sigstack.ss_flags = 0;
>   // sigaltstack(&sigstack, NULL);
>   struct sigaction sa;
>   sa.sa_handler = handler;
>   sigemptyset(&sa.sa_mask);
>   sa.sa_flags = SA_ONSTACK;
>   sigaction(SIGSEGV, &sa, NULL);
>   if (setjmp(exit_jmp) == 0)
>     {
>       return f();
>     }
>   puts("recovered");
>   return 0;
> }
> (sid-sh4-sbuild)root@jessie32:/tmp# gcc stackoverflow.c -o stackoverflow
> (sid-sh4-sbuild)root@jessie32:/tmp# ./stackoverflow
> qemu: uncaught target signal 11 (Segmentation fault) - core dumped
> Segmentation fault
> (sid-sh4-sbuild)root@jessie32:/tmp#
> 
> Thus, for me it seems sigaltstack behaves as expected with the patch
> applied.
> 
> Am I missing something obvious?
> 
> Cheers,
> Adrian
> 

Re: [Qemu-devel] [PATCH] linux-user: Enable sigaltstack syscall for sh4

[Peter Maydell]

On 19 November 2015 at 10:22, Laurent Vivier <lvivier@redhat.com> wrote:
> Hi,
>
> On 19/11/2015 10:28, John Paul Adrian Glaubitz wrote:
>> On 11/19/2015 10:17 AM, Peter Maydell wrote:
>>> Unfortunately this isn't sufficient. You also need to add
>>> the code to the sh4-specific functions in linux-user/signal.c
>>> which honours the requested sigaltstack when taking and returning
>>> from signal handlers.
>
> it seems all needed functions for sh4 signal handling are already
> written in linux-user/signal.c, I thing about setup_frame(),
> setup_rt_frame(), do_sigreturn() and do_rt_sigreturn().

Other architectures have calls to do_sigaltstack or other
sigaltstack handling in their frame setup and sigreturn
functions. SH4 doesn't, which implied to me that we were
missing something beyond just enabling the call to do_sigaltstack.
Maybe that's for something else?

(Incidentally the only targets other than SH4 not listed in
this ifdef are CRIS, Microblaze, TileGX and Unicore32.
Maybe we should just go ahead and remove the ifdef entirely,
if do_sigaltstack() doesn't really have any target-specific
requirements?)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] linux-user: Enable sigaltstack syscall for sh4

[Laurent Vivier]

On 19/11/2015 12:15, Peter Maydell wrote:
> On 19 November 2015 at 10:22, Laurent Vivier <lvivier@redhat.com> wrote:
>> Hi,
>>
>> On 19/11/2015 10:28, John Paul Adrian Glaubitz wrote:
>>> On 11/19/2015 10:17 AM, Peter Maydell wrote:
>>>> Unfortunately this isn't sufficient. You also need to add
>>>> the code to the sh4-specific functions in linux-user/signal.c
>>>> which honours the requested sigaltstack when taking and returning
>>>> from signal handlers.
>>
>> it seems all needed functions for sh4 signal handling are already
>> written in linux-user/signal.c, I thing about setup_frame(),
>> setup_rt_frame(), do_sigreturn() and do_rt_sigreturn().
> 
> Other architectures have calls to do_sigaltstack or other
> sigaltstack handling in their frame setup and sigreturn
> functions. SH4 doesn't, which implied to me that we were
> missing something beyond just enabling the call to do_sigaltstack.
> Maybe that's for something else?

Well, I didn't really check the code in these functions, but SH4 has the
call in do_rt_sigreturn().

The only check I did is to compare setup_frame() with the one from
kernel and it seems ok.

But I think the patch from Adrian is just fixing a forgetting in the
original SH4 patch:

c3b5bc8 SH4: Signal handling for the user space emulator, by Magnus Damm.

This patch uses do_sigaltstack() without enabling sigaltstack() syscall.

> (Incidentally the only targets other than SH4 not listed in
> this ifdef are CRIS, Microblaze, TileGX and Unicore32.
> Maybe we should just go ahead and remove the ifdef entirely,
> if do_sigaltstack() doesn't really have any target-specific
> requirements?)

I don't know if these targets have a lot of users. At least for SH4, we
can enable it and see if someone is screaming (I like the "crash-test
method" :) ). I think Adrian will take care of this (and I can help).

Thanks,
Laurent

Re: [Qemu-devel] [PATCH] linux-user: Enable sigaltstack syscall for sh4

[John Paul Adrian Glaubitz]

On 11/19/2015 01:20 PM, Laurent Vivier wrote:
> I don't know if these targets have a lot of users. At least for SH4, we
> can enable it and see if someone is screaming (I like the "crash-test
> method" :) ). I think Adrian will take care of this (and I can help).

Exactly :).

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [Qemu-devel] [PATCH] linux-user: Enable sigaltstack syscall for sh4

[Peter Maydell]

On 19 November 2015 at 12:20, Laurent Vivier <lvivier@redhat.com> wrote:
> On 19/11/2015 12:15, Peter Maydell wrote:
>> (Incidentally the only targets other than SH4 not listed in
>> this ifdef are CRIS, Microblaze, TileGX and Unicore32.
>> Maybe we should just go ahead and remove the ifdef entirely,
>> if do_sigaltstack() doesn't really have any target-specific
>> requirements?)
>
> I don't know if these targets have a lot of users. At least for SH4, we
> can enable it and see if someone is screaming (I like the "crash-test
> method" :) ). I think Adrian will take care of this (and I can help).

TileGX is new and not fully implemented yet anyway; Unicore32
is pretty moribund. I don't think CRIS and MicroBlaze have a lot
of use of the linux-user target; cc'ing Edgar as maintainer.

So I would say dropping the ifdef is safe, but probably best
left til 2.6.

thanks
-- PMM