[3/4] ARC/signal: shield sa_restorer from compiler toggle side-effects

when building uClibc with -O0 (DODEBUG build) the default sigrestorer
had some extra glue code generated for stack manipulation which was
messing up resume from signal path.

So annotate the function with -Os so that gcc would only generate the
bare min 2 instruction TRAP sequence

Reported-and-Debugged-by: Alexey Brodkin <abrodkin@synopsys.com>
Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 libc/sysdeps/linux/arc/sigaction.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

On 26 March 2015 at 09:55, Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:
> when building uClibc with -O0 (DODEBUG build) the default sigrestorer
> had some extra glue code generated for stack manipulation which was
> messing up resume from signal path.
>
> So annotate the function with -Os so that gcc would only generate the
> bare min 2 instruction TRAP sequence

.. which is the reason the rt_sigrestorer and default_sigrestorer are
usually implemented in a separate file (usually asm, IIRC).
I think the toplevel impl that is used in x86_64 works fine and has
the nice benefit to be in one source-file.
See e.g.: libc/sysdeps/linux/x86_64/sigaction.c

This would have avoided all the sigaction dance in
65bc357213f1c08e339757923740f5d68212cf76 and
f74294bd6768af59e33dc14c6fed41f01d0adc5b it seems, but anyway.

Do you think the x86_64 scheme makes sense for you and you can peruse it?

thanks,
>
> Reported-and-Debugged-by: Alexey Brodkin <abrodkin@synopsys.com>
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> ---
>  libc/sysdeps/linux/arc/sigaction.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/libc/sysdeps/linux/arc/sigaction.c b/libc/sysdeps/linux/arc/sigaction.c
> index 4a4c9e2d0821..3e2f91cd73e8 100644
> --- a/libc/sysdeps/linux/arc/sigaction.c
> +++ b/libc/sysdeps/linux/arc/sigaction.c
> @@ -13,7 +13,8 @@
>  /*
>   * Default sigretrun stub if user doesn't specify SA_RESTORER
>   */
> -static void __default_rt_sa_restorer(void)
> +static void __attribute__((noinline, optimize ("-Os")))
> +__default_rt_sa_restorer(void)
>  {
>         INTERNAL_SYSCALL_NCS(__NR_rt_sigreturn, , 0);
>  }
> --
> 1.9.1
>
> _______________________________________________
> uClibc mailing list
> uClibc@uclibc.org
> http://lists.busybox.net/mailman/listinfo/uclibc

On Thursday 26 March 2015 04:49 PM, Bernhard Reutner-Fischer wrote:
> On 26 March 2015 at 09:55, Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:
>> when building uClibc with -O0 (DODEBUG build) the default sigrestorer
>> had some extra glue code generated for stack manipulation which was
>> messing up resume from signal path.
>>
>> So annotate the function with -Os so that gcc would only generate the
>> bare min 2 instruction TRAP sequence
> .. which is the reason the rt_sigrestorer and default_sigrestorer are
> usually implemented in a separate file (usually asm, IIRC).

Yeah, we used to have it asm which was nice and fine, except that it was a
different source file so PIC references to restorer were not super optimal.

> I think the toplevel impl that is used in x86_64 works fine and has
> the nice benefit to be in one source-file.
> See e.g.: libc/sysdeps/linux/x86_64/sigaction.c
>
> This would have avoided all the sigaction dance in
> 65bc357213f1c08e339757923740f5d68212cf76 and
> f74294bd6768af59e33dc14c6fed41f01d0adc5b it seems, but anyway.

Only the first one, latter is not related to restorer it just avoids a hop in
sigaction path.

>
> Do you think the x86_64 scheme makes sense for you and you can peruse it?

I could, but....

x86 approach solves the multi-src file issue, but we still lack dwarf unwind info,
w/o adding explicit asm CFA ops which AFAIKR our tools still don't support. With C
we get the dwarf info for free.

Thx,
-Vineet


>
> thanks,
>> Reported-and-Debugged-by: Alexey Brodkin <abrodkin@synopsys.com>
>> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>> ---
>>  libc/sysdeps/linux/arc/sigaction.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/libc/sysdeps/linux/arc/sigaction.c b/libc/sysdeps/linux/arc/sigaction.c
>> index 4a4c9e2d0821..3e2f91cd73e8 100644
>> --- a/libc/sysdeps/linux/arc/sigaction.c
>> +++ b/libc/sysdeps/linux/arc/sigaction.c
>> @@ -13,7 +13,8 @@
>>  /*
>>   * Default sigretrun stub if user doesn't specify SA_RESTORER
>>   */
>> -static void __default_rt_sa_restorer(void)
>> +static void __attribute__((noinline, optimize ("-Os")))
>> +__default_rt_sa_restorer(void)
>>  {
>>         INTERNAL_SYSCALL_NCS(__NR_rt_sigreturn, , 0);
>>  }
>> --
>> 1.9.1
>>
>> _______________________________________________
>> uClibc mailing list
>> uClibc@uclibc.org
>> http://lists.busybox.net/mailman/listinfo/uclibc

On 26 March 2015 at 12:34, Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:

> x86 approach solves the multi-src file issue, but we still lack dwarf unwind info,
> w/o adding explicit asm CFA ops which AFAIKR our tools still don't support. With C
> we get the dwarf info for free.

Ah right. Fair enough.
Please use attribute_optimize("Os") __attribute_noinline__ instead.
thanks,