[PATCH v2 1/2] Refactor sigcontextinfo.h

classic Classic list List threaded Threaded
33 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 1/2] Refactor sigcontextinfo.h

Adhemerval Zanella-2


On 15/08/2019 08:59, Florian Weimer wrote:
> I did this:
>
> -#define GET_PC(ctx) ((intptr_t) (((ucontext_t *) \
> +#define GET_PC(ctx) (1 + (intptr_t) (((ucontext_t *) \
>       (ctx))->uc_mcontext.gregs[REG_RIP]))
>
> And none of the existing x86-64 tests failed. 8-(

I would expect so, since GET_PC is only used internally on libSegFault and profiler.
Both will just dump bogus values and we currently don't have tests to check it .

>
> So I strongly urge you to add a test like the one below.  I can commit
> it separately after your patch.  I still need to verify that it builds
> on all architectures.

I will integrate your tests if you are ok with that, however it requires some
changes. I will double check it on some different architectures also.

>
> Thanks,
> Florian
>
>
> p
>
> diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
> index 1ab6bcbfc8..df960aa7b4 100644
> --- a/sysdeps/unix/sysv/linux/Makefile
> +++ b/sysdeps/unix/sysv/linux/Makefile
> @@ -55,7 +55,7 @@ tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \
>   test-errno-linux tst-memfd_create tst-mlock2 tst-pkey \
>   tst-rlimit-infinity tst-ofdlocks tst-gettid tst-gettid-kill \
>   tst-tgkill
> -tests-internal += tst-ofdlocks-compat
> +tests-internal += tst-ofdlocks-compat tst-sigcontextinfo-get_pc
>  
>  # Generate the list of SYS_* macros for the system calls (__NR_*
>  # macros).  The file syscall-names.list contains all possible system
> diff --git a/sysdeps/unix/sysv/linux/tst-sigcontextinfo-get_pc.c b/sysdeps/unix/sysv/linux/tst-sigcontextinfo-get_pc.c
> new file mode 100644
> index 0000000000..f16b30250e
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/tst-sigcontextinfo-get_pc.c
> @@ -0,0 +1,84 @@
> +/* Test that the GET_PC macro is consistent with the unwinder.
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public License as
> +   published by the Free Software Foundation; either version 2.1 of the
> +   License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; see the file COPYING.LIB.  If
> +   not, see <http://www.gnu.org/licenses/>.  */
> +
> +/* This test searches for the value of the GET_PC macro in the
> +   addresses obtained from the backtrace function.  */
> +
> +#include <array_length.h>
> +#include <execinfo.h>
> +#include <inttypes.h>
> +#include <signal.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <support/check.h>
> +#include <support/xsignal.h>
> +
> +/* This file defines macros to access the content of the sigcontext element
> +   passed up by the signal handler.  */
> +#include <sigcontextinfo.h>
> +
> +#ifdef SA_SIGINFO
> +# define SIGCONTEXT siginfo_t *info, void *
> +#endif

We can assume SA_SIGINFO for Linux, currently the only system that does
not define is Hurd.

> +
> +static bool handler_called;
> +
> +static void
> +handler (int signal, SIGCONTEXT ctx)
> +{
> +  TEST_COMPARE (signal, SIGUSR1);
> +
> +  uintptr_t pc = GET_PC (ctx);
> +  printf ("info: address in signal handler: 0x%" PRIxPTR "\n", pc);
> +
> +  void *callstack[10];
> +  int callstack_count = backtrace (callstack, array_length (callstack));
> +  TEST_VERIFY_EXIT (callstack_count > 0);
> +  TEST_VERIFY_EXIT (callstack_count <= array_length (callstack));
> +  bool found = false;
> +  for (int i = 0; i < callstack_count; ++i)
> +    {
> +      const char *marker;
> +      if ((uintptr_t) callstack[i] == pc)
> +        {
> +          found = true;
> +          marker = " *";
> +        }
> +      else
> +        marker = "";
> +      printf ("info: call stack entry %d: 0x%" PRIxPTR "%s\n",
> +              i, (uintptr_t) callstack[i], marker);
> +    }
> +  TEST_VERIFY (found);
> +  handler_called = true;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  struct sigaction sa =
> +    {
> +     .sa_sigaction = &handler,

We need a '.sa_flags = SA_SIGINFO' here.

> +    };
> +  xsigaction (SIGUSR1, &sa, NULL);
> +  raise (SIGUSR1);
> +  TEST_VERIFY (handler_called);
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 1/2] Refactor sigcontextinfo.h

Florian Weimer-5
* Adhemerval Zanella:

>> So I strongly urge you to add a test like the one below.  I can commit
>> it separately after your patch.  I still need to verify that it builds
>> on all architectures.
>
> I will integrate your tests if you are ok with that, however it requires some
> changes.

Thanks.  What kind of changes?

SA_SIGINFO is missing from sa_flags.  I've already fixed that.

> I will double check it on some different architectures also.

It works on aarch64, i386, x86-64, s390, s390x, ppc64le.  I planned to
test ppc and ppc64 as well, but could not get a Beaker machine yet.

Florian
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 1/2] Refactor sigcontextinfo.h

Adhemerval Zanella-2


On 15/08/2019 10:53, Florian Weimer wrote:

> * Adhemerval Zanella:
>
>>> So I strongly urge you to add a test like the one below.  I can commit
>>> it separately after your patch.  I still need to verify that it builds
>>> on all architectures.
>>
>> I will integrate your tests if you are ok with that, however it requires some
>> changes.
>
> Thanks.  What kind of changes?
>
> SA_SIGINFO is missing from sa_flags.  I've already fixed that.
>
>> I will double check it on some different architectures also.
>
> It works on aarch64, i386, x86-64, s390, s390x, ppc64le.  I planned to
> test ppc and ppc64 as well, but could not get a Beaker machine yet.

I can check on alpha, hppa, sh4, arm, m68k, sparc, and ia64. Both ia64 and
sparc does not pass a ucontext_t as third argument, so these are the ones
I would like to double-check.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 1/2] Refactor sigcontextinfo.h

Florian Weimer-5
In reply to this post by Adhemerval Zanella-2
* Adhemerval Zanella:

> diff --git a/sysdeps/unix/sysv/linux/sparc/sparc32/sigcontextinfo.h b/sysdeps/unix/sysv/linux/sparc/sparc32/sigcontextinfo.h
> index 9eec807a23..af1bcd21e2 100644
> --- a/sysdeps/unix/sysv/linux/sparc/sparc32/sigcontextinfo.h
> +++ b/sysdeps/unix/sysv/linux/sparc/sparc32/sigcontextinfo.h
> @@ -16,5 +16,38 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> -#define SIGCONTEXT struct sigcontext *
> -#define GET_PC(__ctx) ((void *) ((__ctx)->si_regs.pc))
> +#ifndef _SIGCONTEXTINFO_H
> +#define _SIGCONTEXTINFO_H
> +
> +/* The kernel sparc32 signal frame for SA_SIGINFO is layout as:

“is defined as”?

> +  struct rt_signal_frame32 {
> +   struct sparc_stackf32 ss;
> +   compat_siginfo_t info;
> +   struct pt_regs32 regs;
> +   compat_sigset_t mask;  
> +   u32 fpu_save;
> +   unsigned int insns[2];
> +   compat_stack_t stack;
> +   unsigned int extra_size;
> +   siginfo_extra_v8plus_t v8plus;
> +   u32 rwin_save;
> +  } __attribute__((aligned(8)));

Isn't it GNU style to put the { on a line by itself (unindenteed)?
This is also relevant to the code, see struct kernel_sigset_t a bit
below in the posted patch.

> +  And different that other architectures, SPARC32 pass the pt_regs32
> +  REGS pointer as third argument for sa_sigaction handler with
> +  SA_SIGINFO.  */

“Unlike other architectures, sparc32 passes pt_regs32 REGS pointers as
the third argument to a sa_sigaction handler with SA_SIGINFO enabled.”
or something like that?

“Different that” appears in a few other places as well which also need
fixing.

> diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/register-dump.h b/sysdeps/unix/sysv/linux/sparc/sparc64/register-dump.h
> index 0f1078ad9e..722f06e09d 100644
> --- a/sysdeps/unix/sysv/linux/sparc/sparc64/register-dump.h
> +++ b/sysdeps/unix/sysv/linux/sparc/sparc64/register-dump.h
> @@ -60,17 +60,36 @@ hexvalue (unsigned long int value, char *buf, size_t len)
>      *--cp = '0';
>  }
>  
> +/* Different that other architectures, SPARC32 pass a pt_regs (or pt_regs32
> +   in 32 bits compat mode) struct pointer as third argument for sa_sigaction
> +   handler with SA_SIGINFO.

I think this is the sparc64 header, so the comment needs to be adjusted.

> diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/sigcontextinfo.h b/sysdeps/unix/sysv/linux/sparc/sparc64/sigcontextinfo.h
> index a2f2b1f7de..744b51c585 100644
> --- a/sysdeps/unix/sysv/linux/sparc/sparc64/sigcontextinfo.h
> +++ b/sysdeps/unix/sysv/linux/sparc/sparc64/sigcontextinfo.h
> @@ -16,8 +16,45 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> +#ifndef _SIGCONTEXTINFO_H
> +#define _SIGCONTEXTINFO_H
> +
> +#include <bits/types/siginfo_t.h>
> +
> +/* The kernel sparc64 signal frame for SA_SIGINFO is layout as:
> +
> +   struct rt_signal_frame {
> +     struct sparc_stackf ss;
> +     siginfo_t info;
> +     struct pt_regs regs;
> +     __siginfo_fpu_t *fpu_save;
> +     stack_t stack;
> +     sigset_t mask;
> +     __siginfo_rwin_t *rwin_save;
> +   };
> +
> +   And different that other architectures, SPARC64 pass the siginfo_t
> +   INFO pointer as third argument for sa_sigaction handler with
> +   SA_SIGINFO.  */
> +
>  #ifndef STACK_BIAS
>  #define STACK_BIAS 2047
>  #endif
> -#define SIGCONTEXT struct sigcontext *
> -#define GET_PC(__ctx) ((void *) ((__ctx)->sigc_regs.tpc))
> +
> +struct pt_regs
> +{
> + unsigned long u_regs[16];
> + unsigned long tstate;
> + unsigned long tpc;
> + unsigned long tnpc;
> + unsigned int y;
> + unsigned int magic;
> +};

Wrong indentation, and “unsigned long int” should be used.

The patch has trailing whitespace in a few places, FWIW.

Overall, I'm in favor of this change.  I do wonder if GET_PC should be
an inline function instead, so that we get type checking for its
argument (even if the implementation needs to cast the pointer to
something else).

Thnaks,
Florian
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 1/2] Refactor sigcontextinfo.h

Florian Weimer-5
In reply to this post by Adhemerval Zanella-2
* Adhemerval Zanella:

> On 15/08/2019 10:53, Florian Weimer wrote:
>> * Adhemerval Zanella:
>>
>>>> So I strongly urge you to add a test like the one below.  I can commit
>>>> it separately after your patch.  I still need to verify that it builds
>>>> on all architectures.
>>>
>>> I will integrate your tests if you are ok with that, however it requires some
>>> changes.
>>
>> Thanks.  What kind of changes?
>>
>> SA_SIGINFO is missing from sa_flags.  I've already fixed that.
>>
>>> I will double check it on some different architectures also.
>>
>> It works on aarch64, i386, x86-64, s390, s390x, ppc64le.  I planned to
>> test ppc and ppc64 as well, but could not get a Beaker machine yet.
>
> I can check on alpha, hppa, sh4, arm, m68k, sparc, and ia64. Both ia64 and
> sparc does not pass a ucontext_t as third argument, so these are the ones
> I would like to double-check.

Sounds good.  The tests (with the SA_SIGINFO fix applied) builds
everywhere using build-many-glibcs.py.

Thanks,
Florian
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 1/2] Refactor sigcontextinfo.h

Adhemerval Zanella-2


On 15/08/2019 11:36, Florian Weimer wrote:

> * Adhemerval Zanella:
>
>> On 15/08/2019 10:53, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>>> So I strongly urge you to add a test like the one below.  I can commit
>>>>> it separately after your patch.  I still need to verify that it builds
>>>>> on all architectures.
>>>>
>>>> I will integrate your tests if you are ok with that, however it requires some
>>>> changes.
>>>
>>> Thanks.  What kind of changes?
>>>
>>> SA_SIGINFO is missing from sa_flags.  I've already fixed that.
>>>
>>>> I will double check it on some different architectures also.
>>>
>>> It works on aarch64, i386, x86-64, s390, s390x, ppc64le.  I planned to
>>> test ppc and ppc64 as well, but could not get a Beaker machine yet.
>>
>> I can check on alpha, hppa, sh4, arm, m68k, sparc, and ia64. Both ia64 and
>> sparc does not pass a ucontext_t as third argument, so these are the ones
>> I would like to double-check.
>
> Sounds good.  The tests (with the SA_SIGINFO fix applied) builds
> everywhere using build-many-glibcs.py.

The check call backtrace with in a signal handle, so it needs to be built with
-fasynchronous-unwind-tables (testing on sparc showed that without this option
it can't get a full stacktrace, I think some ABIs have the same issue).

Another issue is ia64 trick to try to find the instruction in bundle by
multiplying the ri number by 4 and adding on sc_ip makes the resulting
value not comparable to the one got from backtrace.  I see this is best
effort, since not all instructions have the same length (although all
bundle are 128-bits).  I think ia64 libdebug and profiling can live
without this hack, and it simplifies the bz#12683 fix as well.

I will resend the patch with this changes and the test included.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 1/2] Refactor sigcontextinfo.h

Florian Weimer-5
* Adhemerval Zanella:

> On 15/08/2019 11:36, Florian Weimer wrote:
>> * Adhemerval Zanella:
>>
>>> On 15/08/2019 10:53, Florian Weimer wrote:
>>>> * Adhemerval Zanella:
>>>>
>>>>>> So I strongly urge you to add a test like the one below.  I can commit
>>>>>> it separately after your patch.  I still need to verify that it builds
>>>>>> on all architectures.
>>>>>
>>>>> I will integrate your tests if you are ok with that, however it requires some
>>>>> changes.
>>>>
>>>> Thanks.  What kind of changes?
>>>>
>>>> SA_SIGINFO is missing from sa_flags.  I've already fixed that.
>>>>
>>>>> I will double check it on some different architectures also.
>>>>
>>>> It works on aarch64, i386, x86-64, s390, s390x, ppc64le.  I planned to
>>>> test ppc and ppc64 as well, but could not get a Beaker machine yet.
>>>
>>> I can check on alpha, hppa, sh4, arm, m68k, sparc, and ia64. Both ia64 and
>>> sparc does not pass a ucontext_t as third argument, so these are the ones
>>> I would like to double-check.
>>
>> Sounds good.  The tests (with the SA_SIGINFO fix applied) builds
>> everywhere using build-many-glibcs.py.
>
> The check call backtrace with in a signal handle, so it needs to be
> built with -fasynchronous-unwind-tables (testing on sparc showed that
> without this option it can't get a full stacktrace, I think some ABIs
> have the same issue).

Oh, right.  Wouldn't raise.c have to be compiled in this way?

(We should really build all of glibc with asynchronous unwind tables.)

Thanks,
Florian
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 1/2] Refactor sigcontextinfo.h

Adhemerval Zanella-2
In reply to this post by Florian Weimer-5


On 15/08/2019 11:07, Florian Weimer wrote:

> * Adhemerval Zanella:
>
>> diff --git a/sysdeps/unix/sysv/linux/sparc/sparc32/sigcontextinfo.h b/sysdeps/unix/sysv/linux/sparc/sparc32/sigcontextinfo.h
>> index 9eec807a23..af1bcd21e2 100644
>> --- a/sysdeps/unix/sysv/linux/sparc/sparc32/sigcontextinfo.h
>> +++ b/sysdeps/unix/sysv/linux/sparc/sparc32/sigcontextinfo.h
>> @@ -16,5 +16,38 @@
>>     License along with the GNU C Library; if not, see
>>     <http://www.gnu.org/licenses/>.  */
>>  
>> -#define SIGCONTEXT struct sigcontext *
>> -#define GET_PC(__ctx) ((void *) ((__ctx)->si_regs.pc))
>> +#ifndef _SIGCONTEXTINFO_H
>> +#define _SIGCONTEXTINFO_H
>> +
>> +/* The kernel sparc32 signal frame for SA_SIGINFO is layout as:
>
> “is defined as”?

Ack, I will use your suggestion.

>
>> +  struct rt_signal_frame32 {
>> +   struct sparc_stackf32 ss;
>> +   compat_siginfo_t info;
>> +   struct pt_regs32 regs;
>> +   compat_sigset_t mask;  
>> +   u32 fpu_save;
>> +   unsigned int insns[2];
>> +   compat_stack_t stack;
>> +   unsigned int extra_size;
>> +   siginfo_extra_v8plus_t v8plus;
>> +   u32 rwin_save;
>> +  } __attribute__((aligned(8)));
>
> Isn't it GNU style to put the { on a line by itself (unindenteed)?
> This is also relevant to the code, see struct kernel_sigset_t a bit
> below in the posted patch.

Well this is copies from kernel source (arch/sparc/kernel/signal32.c:59),
I will change to GNU style.

>
>> +  And different that other architectures, SPARC32 pass the pt_regs32
>> +  REGS pointer as third argument for sa_sigaction handler with
>> +  SA_SIGINFO.  */
>
> “Unlike other architectures, sparc32 passes pt_regs32 REGS pointers as
> the third argument to a sa_sigaction handler with SA_SIGINFO enabled.”
> or something like that?

Ack, I changed to your suggestion.

>
> “Different that” appears in a few other places as well which also need
> fixing.

Ack (it seems it appears on ia64 and sparc64 as well, I changed them too).

>
>> diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/register-dump.h b/sysdeps/unix/sysv/linux/sparc/sparc64/register-dump.h
>> index 0f1078ad9e..722f06e09d 100644
>> --- a/sysdeps/unix/sysv/linux/sparc/sparc64/register-dump.h
>> +++ b/sysdeps/unix/sysv/linux/sparc/sparc64/register-dump.h
>> @@ -60,17 +60,36 @@ hexvalue (unsigned long int value, char *buf, size_t len)
>>      *--cp = '0';
>>  }
>>  
>> +/* Different that other architectures, SPARC32 pass a pt_regs (or pt_regs32
>> +   in 32 bits compat mode) struct pointer as third argument for sa_sigaction
>> +   handler with SA_SIGINFO.
>
> I think this is the sparc64 header, so the comment needs to be adjusted.

Ack, I used a similar text you suggested for sparc32.

>
>> diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/sigcontextinfo.h b/sysdeps/unix/sysv/linux/sparc/sparc64/sigcontextinfo.h
>> index a2f2b1f7de..744b51c585 100644
>> --- a/sysdeps/unix/sysv/linux/sparc/sparc64/sigcontextinfo.h
>> +++ b/sysdeps/unix/sysv/linux/sparc/sparc64/sigcontextinfo.h
>> @@ -16,8 +16,45 @@
>>     License along with the GNU C Library; if not, see
>>     <http://www.gnu.org/licenses/>.  */
>>  
>> +#ifndef _SIGCONTEXTINFO_H
>> +#define _SIGCONTEXTINFO_H
>> +
>> +#include <bits/types/siginfo_t.h>
>> +
>> +/* The kernel sparc64 signal frame for SA_SIGINFO is layout as:
>> +
>> +   struct rt_signal_frame {
>> +     struct sparc_stackf ss;
>> +     siginfo_t info;
>> +     struct pt_regs regs;
>> +     __siginfo_fpu_t *fpu_save;
>> +     stack_t stack;
>> +     sigset_t mask;
>> +     __siginfo_rwin_t *rwin_save;
>> +   };
>> +
>> +   And different that other architectures, SPARC64 pass the siginfo_t
>> +   INFO pointer as third argument for sa_sigaction handler with
>> +   SA_SIGINFO.  */


Btw, I used a similar text you suggested for sparc32.

>> +
>>  #ifndef STACK_BIAS
>>  #define STACK_BIAS 2047
>>  #endif
>> -#define SIGCONTEXT struct sigcontext *
>> -#define GET_PC(__ctx) ((void *) ((__ctx)->sigc_regs.tpc))
>> +
>> +struct pt_regs
>> +{
>> + unsigned long u_regs[16];
>> + unsigned long tstate;
>> + unsigned long tpc;
>> + unsigned long tnpc;
>> + unsigned int y;
>> + unsigned int magic;
>> +};
>
> Wrong indentation, and “unsigned long int” should be used.

Ack.

>
> The patch has trailing whitespace in a few places, FWIW.

Ack.

>
> Overall, I'm in favor of this change.  I do wonder if GET_PC should be
> an inline function instead, so that we get type checking for its
> argument (even if the implementation needs to cast the pointer to
> something else).

Good suggestion, I will change to an inline function.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 1/2] Refactor sigcontextinfo.h

Adhemerval Zanella-2
In reply to this post by Florian Weimer-5


On 15/08/2019 14:56, Florian Weimer wrote:

> * Adhemerval Zanella:
>
>> On 15/08/2019 11:36, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> On 15/08/2019 10:53, Florian Weimer wrote:
>>>>> * Adhemerval Zanella:
>>>>>
>>>>>>> So I strongly urge you to add a test like the one below.  I can commit
>>>>>>> it separately after your patch.  I still need to verify that it builds
>>>>>>> on all architectures.
>>>>>>
>>>>>> I will integrate your tests if you are ok with that, however it requires some
>>>>>> changes.
>>>>>
>>>>> Thanks.  What kind of changes?
>>>>>
>>>>> SA_SIGINFO is missing from sa_flags.  I've already fixed that.
>>>>>
>>>>>> I will double check it on some different architectures also.
>>>>>
>>>>> It works on aarch64, i386, x86-64, s390, s390x, ppc64le.  I planned to
>>>>> test ppc and ppc64 as well, but could not get a Beaker machine yet.
>>>>
>>>> I can check on alpha, hppa, sh4, arm, m68k, sparc, and ia64. Both ia64 and
>>>> sparc does not pass a ucontext_t as third argument, so these are the ones
>>>> I would like to double-check.
>>>
>>> Sounds good.  The tests (with the SA_SIGINFO fix applied) builds
>>> everywhere using build-many-glibcs.py.
>>
>> The check call backtrace with in a signal handle, so it needs to be
>> built with -fasynchronous-unwind-tables (testing on sparc showed that
>> without this option it can't get a full stacktrace, I think some ABIs
>> have the same issue).
>
> Oh, right.  Wouldn't raise.c have to be compiled in this way?

It is not strictly necessary for the test, since it will hit the expected PC
just before the raise call. However on some ABIs, sparc for instance,
backtrace stops right before the raise call.

>
> (We should really build all of glibc with asynchronous unwind tables.)

Does libgcc_s require asynchronous unwind tables to correctly unwind? Or does
it need just for PTHREAD_CANCEL_ASYNCHRONOUS, where the unwind happens in the
signal handler instead of the syscall entry?
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 1/2] Refactor sigcontextinfo.h

Florian Weimer-5
* Adhemerval Zanella:

>> Oh, right.  Wouldn't raise.c have to be compiled in this way?
>
> It is not strictly necessary for the test, since it will hit the expected PC
> just before the raise call. However on some ABIs, sparc for instance,
> backtrace stops right before the raise call.

Hmm.  That's strange.  Maybe that's because there's a trampoline that
requires unwinding tables?

>> (We should really build all of glibc with asynchronous unwind tables.)
>
> Does libgcc_s require asynchronous unwind tables to correctly unwind? Or does
> it need just for PTHREAD_CANCEL_ASYNCHRONOUS, where the unwind happens in the
> signal handler instead of the syscall entry?

I think this is highly architecture-specific.  There are architecture
defaults if there are no unwind tables (some assume a frame pointer,
some assume a leaf function without any stack adjustments).  In general,
I suspect that on many architectures, unwind tables are required for
unwinding.  Distributions really should build all system libraries with
unwind tables for that reason, but not all of them do, unfortunately.
(Building with -fexceptions also enables an equivalent of SafeSEH for
thread cancellation handling.  This is avoids potential exploits that
target the cancellation cleanup routine, which is relevant even if
pthread_cancel is never actually called.)

Thanks,
Florian
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 1/2] Refactor sigcontextinfo.h

Adhemerval Zanella-2


On 19/08/2019 08:34, Florian Weimer wrote:

> * Adhemerval Zanella:
>
>>> Oh, right.  Wouldn't raise.c have to be compiled in this way?
>>
>> It is not strictly necessary for the test, since it will hit the expected PC
>> just before the raise call. However on some ABIs, sparc for instance,
>> backtrace stops right before the raise call.
>
> Hmm.  That's strange.  Maybe that's because there's a trampoline that
> requires unwinding tables?

I am not sure, sparc has the sigaction trampoline implemented in sigaction
and it is not build with -fasynchronous-unwind-tables.

>
>>> (We should really build all of glibc with asynchronous unwind tables.)
>>
>> Does libgcc_s require asynchronous unwind tables to correctly unwind? Or does
>> it need just for PTHREAD_CANCEL_ASYNCHRONOUS, where the unwind happens in the
>> signal handler instead of the syscall entry?
>
> I think this is highly architecture-specific.  There are architecture
> defaults if there are no unwind tables (some assume a frame pointer,
> some assume a leaf function without any stack adjustments).  In general,
> I suspect that on many architectures, unwind tables are required for
> unwinding.  Distributions really should build all system libraries with
> unwind tables for that reason, but not all of them do, unfortunately.
> (Building with -fexceptions also enables an equivalent of SafeSEH for
> thread cancellation handling.  This is avoids potential exploits that
> target the cancellation cleanup routine, which is relevant even if
> pthread_cancel is never actually called.)

It is indeed architecture-specific from my work with BZ#12683, which
make some code 'unwindable' in some platforms and not in other with
same compiler flags.

To enable unwind table as default one thing that we might check is the
result extra runtime size required to map the EH segments.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 1/2] Refactor sigcontextinfo.h

Florian Weimer-5
* Adhemerval Zanella:

> To enable unwind table as default one thing that we might check is the
> result extra runtime size required to map the EH segments.

Hmm.  It's 25 KiB on x86-64.  Is it much larger on other architectures?

I'm working on slowly reducing private RSS (on all architectures).
Maybe I can get this change in return? 8-)

Thanks,
Florian
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 1/2] Refactor sigcontextinfo.h

Adhemerval Zanella-2


On 20/08/2019 07:16, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> To enable unwind table as default one thing that we might check is the
>> result extra runtime size required to map the EH segments.
>
> Hmm.  It's 25 KiB on x86-64.  Is it much larger on other architectures?

I will try to spare some time to check this on other architectures.

>
> I'm working on slowly reducing private RSS (on all architectures).
> Maybe I can get this change in return? 8-)

I don't recall if is mapped in private process space or shared. If is
private, is kind large in fact (although compare to the libc.so one
could argue is feasible).

12