[v2,1/2] Refactor sigcontextinfo.h

  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-(

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.

Thanks,
Florian
  

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>
>
  

* 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
  

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.
  

* 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
  

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.
  

* 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
  

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?
  

* 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
  

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.
  

* 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
  

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).