s390: Remove backchain-based fallback from backtrace

  Stefan,

do you agree this change is correct?  It's not really clear to me how
the fallback unwinder can work given its dependency on _Unwind_GetIP.

I tested this on s390-linux-gnu and s390x-linux-gnu (but couldn't run
the C++ tests on s390 due to test environment limitations).

Thanks,
Florian
8<------------------------------------------------------------------8<
This backtrace variant is used when _Unwind_Backtrace is not found in
libgcc_s.  It still depends on _Unwind_GetIP from libgcc_s, so it does
not work if loading libgcc_s fails.  Therefore, as long as libgcc_s
provides a definition of _Unwind_Backtrace, the fallback is not useful.

After this change, the implementation is the same on s390 and s390x
and can be shared.

-----
 sysdeps/s390/{s390-32 => }/backtrace.c |  58 +------------
 sysdeps/s390/s390-64/backtrace.c       | 147 ---------------------------------
 2 files changed, 1 insertion(+), 204 deletions(-)
  

Hi Florian,

is there a background story behind this patch or do you just want to 
consolidate the s390-32/s390-64 implementation?

On 2/10/20 7:04 PM, Florian Weimer wrote:
> Stefan,
> 
> do you agree this change is correct?  It's not really clear to me how
> the fallback unwinder can work given its dependency on _Unwind_GetIP.
The current situation:

In init, the libgcc_s.so.1 functions _Unwind_Backtrace and _Unwind_GetIP 
are dlsym'ed. If _Unwind_GetIP is not available, then _Unwind_Backtrace 
is also marked as not available.

In __backtrace, there is a check if _Unwind_Backtrace and implicitly 
_Unwind_GetIP is available.
If not available, then the __backchain_backtrace is used.
Otherwise the _Unwind_Backtrace from libgcc is used.


__backchain_backtrace does not rely on libgcc_s.so.1.
Instead it tries to walk along the backchain (according to the ABI). But 
I assume the usual binaries do not have those backchain pointers as you 
have to use the gcc -mbackchain flag: "Maintain backchain pointer.". If 
not activated, the corresponding backchain-slots on the stack are not 
set and contain random values.

As far as I know, there are customers which build with this backchain 
feature. Thus we don't want to remove it.


_Unwind_Backtrace from libgcc can't handle the backchain. It relies on 
gcc -fasynchronous-unwind-tables.

That means, if somebody has no libgcc and has not build with 
-mbackchain, then __backchain_backtrace will try to walk anywhere and we 
could get segfaults as it seems the sanity checks (backchain-slot == 
NULL or backchain-slot > __libc_stack_end) are not sufficient!?

This also means, if somebody has build with backchain and has libgcc 
installed, then always _Unwind_Backtrace is called.
If the binary was build with gcc -fno-asynchronous-unwind-tables, then 
there is no retry with __backchain_backtrace and there won't be a backtrace.
If the binary contains the unwind-tables, then those are used and not 
the backchain.


Do you know how other architectures behave if there are no unwind-tables?

Would it be possible to check if the current binaries have unwind-tables 
and then use _Unwind_Backtrace if libgcc is available? If not, 
__backchain_backtrace could be used as fallback.

Should __backchain_backtrace have more checks regarding addresses in 
backchain-slot and/or the assumed function-address?

Other thoughts?
> 
> I tested this on s390-linux-gnu and s390x-linux-gnu (but couldn't run
> the C++ tests on s390 due to test environment limitations).
> 
> Thanks,
> Florian
> 8<------------------------------------------------------------------8<
> This backtrace variant is used when _Unwind_Backtrace is not found in
> libgcc_s.  It still depends on _Unwind_GetIP from libgcc_s, so it does
> not work if loading libgcc_s fails.  Therefore, as long as libgcc_s
> provides a definition of _Unwind_Backtrace, the fallback is not useful.
> 
> After this change, the implementation is the same on s390 and s390x
> and can be shared.
> 
> -----
>   sysdeps/s390/{s390-32 => }/backtrace.c |  58 +------------
>   sysdeps/s390/s390-64/backtrace.c       | 147 ---------------------------------
>   2 files changed, 1 insertion(+), 204 deletions(-)
> 
> diff --git a/sysdeps/s390/s390-32/backtrace.c b/sysdeps/s390/backtrace.c
> similarity index 60%
> rename from sysdeps/s390/s390-32/backtrace.c
> rename to sysdeps/s390/backtrace.c
> index 497e4f8875..5801e5e211 100644
> --- a/sysdeps/s390/s390-32/backtrace.c
> +++ b/sysdeps/s390/backtrace.c
> @@ -24,36 +24,6 @@
>   #include <stdlib.h>
>   #include <unwind.h>
>   
> -/* This is a global variable set at program start time.  It marks the
> -   highest used stack address.  */
> -extern void *__libc_stack_end;
> -
> -
> -/* This is the stack layout we see for every non-leaf function.
> -           size                    offset
> -    %r15 ->    +------------------+
> -             4 | back chain       |  0
> -             4 | end of stack     |  4
> -             8 | glue             |  8
> -             8 | scratch          | 16
> -            40 | save area r6-r15 | 24
> -            16 | save area f4,f6  | 64
> -            16 | empty            | 80
> -               +------------------+
> -   r14 in the save area holds the return address.
> -*/
> -
> -struct layout
> -{
> -  int back_chain;
> -  int end_of_stack;
> -  int glue[2];
> -  int scratch[2];
> -  int save_grps[10];
> -  int save_fp[4];
> -  int empty[2];
> -};
> -
>   struct trace_arg
>   {
>     void **array;
> @@ -77,32 +47,6 @@ init (void)
>     if (unwind_getip == NULL)
>       unwind_backtrace = NULL;
>   }
> -
> -static int
> -__backchain_backtrace (void **array, int size)
> -{
> -  /* We assume that all the code is generated with frame pointers set.  */
> -  struct layout *stack;
> -  int cnt = 0;
> -
> -  __asm__ ("LR  %0,%%r15" : "=d" (stack) );
> -  /* We skip the call to this function, it makes no sense to record it.  */
> -  stack = (struct layout *) stack->back_chain;
> -  while (cnt < size)
> -    {
> -      if (stack == NULL || (void *) stack > __libc_stack_end)
> -	/* This means the address is out of range.  Note that for the
> -	   toplevel we see a frame pointer with value NULL which clearly is
> -	   out of range.  */
> -	break;
> -
> -      array[cnt++] = (void *) (stack->save_grps[8] & 0x7fffffff);
> -
> -      stack = (struct layout *) stack->back_chain;
> -    }
> -
> -  return cnt;
> -}
>   #else
>   # define unwind_backtrace _Unwind_Backtrace
>   # define unwind_getip _Unwind_GetIP
> @@ -136,7 +80,7 @@ __backtrace (void **array, int size)
>     __libc_once (once, init);
>   
>     if (unwind_backtrace == NULL)
> -    return __backchain_backtrace (array, size);
> +    return 0;
>   #endif
>   
>     unwind_backtrace (backtrace_helper, &arg);
> diff --git a/sysdeps/s390/s390-64/backtrace.c b/sysdeps/s390/s390-64/backtrace.c
> deleted file mode 100644
> index 5d14e01280..0000000000
> --- a/sysdeps/s390/s390-64/backtrace.c
> +++ /dev/null
> @@ -1,147 +0,0 @@
> -/* Return backtrace of current program state.  64 bit S/390 version.
> -   Copyright (C) 2001-2020 Free Software Foundation, Inc.
> -   Contributed by Martin Schwidefsky <schwidefsky@de.ibm.com>.
> -   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; if not, see
> -   <https://www.gnu.org/licenses/>.  */
> -
> -#include <libc-lock.h>
> -#include <dlfcn.h>
> -#include <execinfo.h>
> -#include <stddef.h>
> -#include <stdlib.h>
> -#include <unwind.h>
> -
> -
> -/* This is a global variable set at program start time.  It marks the
> -   highest used stack address.  */
> -extern void *__libc_stack_end;
> -
> -
> -/* This is the stack layout we see for every non-leaf function.
> -           size                    offset
> -    %r15 ->    +------------------+
> -             8 | back chain       |   0
> -             8 | end of stack     |   8
> -            32 | scratch          |  16
> -            80 | save area r6-r15 |  48
> -            16 | save area f4,f6  | 128
> -            16 | empty            | 144
> -               +------------------+
> -   r14 in the save area holds the return address.
> -*/
> -
> -struct layout
> -{
> -  long back_chain;
> -  long end_of_stack;
> -  long scratch[4];
> -  long save_grps[10];
> -  long save_fp[2];
> -  long empty[2];
> -};
> -
> -struct trace_arg
> -{
> -  void **array;
> -  int cnt, size;
> -};
> -
> -#ifdef SHARED
> -static _Unwind_Reason_Code (*unwind_backtrace) (_Unwind_Trace_Fn, void *);
> -static _Unwind_Ptr (*unwind_getip) (struct _Unwind_Context *);
> -
> -static void
> -init (void)
> -{
> -  void *handle = __libc_dlopen ("libgcc_s.so.1");
> -
> -  if (handle == NULL)
> -    return;
> -
> -  unwind_backtrace = __libc_dlsym (handle, "_Unwind_Backtrace");
> -  unwind_getip = __libc_dlsym (handle, "_Unwind_GetIP");
> -  if (unwind_getip == NULL)
> -    unwind_backtrace = NULL;
> -}
> -
> -static int
> -__backchain_backtrace (void **array, int size)
> -{
> -  /* We assume that all the code is generated with frame pointers set.  */
> -  struct layout *stack;
> -  int cnt = 0;
> -
> -  __asm__ ("LGR  %0,%%r15" : "=d" (stack) );
> -  /* We skip the call to this function, it makes no sense to record it.  */
> -  stack = (struct layout *) stack->back_chain;
> -  while (cnt < size)
> -    {
> -      if (stack == NULL || (void *) stack > __libc_stack_end)
> -	/* This means the address is out of range.  Note that for the
> -	   toplevel we see a frame pointer with value NULL which clearly is
> -	   out of range.  */
> -	break;
> -
> -      array[cnt++] = (void *) stack->save_grps[8];
> -
> -      stack = (struct layout *) stack->back_chain;
> -    }
> -
> -  return cnt;
> -}
> -#else
> -# define unwind_backtrace _Unwind_Backtrace
> -# define unwind_getip _Unwind_GetIP
> -#endif
> -
> -static _Unwind_Reason_Code
> -backtrace_helper (struct _Unwind_Context *ctx, void *a)
> -{
> -  struct trace_arg *arg = a;
> -
> -  /* We are first called with address in the __backtrace function.
> -     Skip it.  */
> -  if (arg->cnt != -1)
> -    arg->array[arg->cnt] = (void *) unwind_getip (ctx);
> -  if (++arg->cnt == arg->size)
> -    return _URC_END_OF_STACK;
> -  return _URC_NO_REASON;
> -}
> -
> -int
> -__backtrace (void **array, int size)
> -{
> -  struct trace_arg arg = { .array = array, .size = size, .cnt = -1 };
> -
> -  if (size <= 0)
> -    return 0;
> -
> -#ifdef SHARED
> -  __libc_once_define (static, once);
> -
> -  __libc_once (once, init);
> -
> -  if (unwind_backtrace == NULL)
> -    return __backchain_backtrace (array, size);
> -#endif
> -
> -  unwind_backtrace (backtrace_helper, &arg);
> -
> -  return arg.cnt != -1 ? arg.cnt : 0;
> -}
> -
> -weak_alias (__backtrace, backtrace)
> -libc_hidden_def (__backtrace)
>
  

* Stefan Liebler:

> Hi Florian,
>
> is there a background story behind this patch or do you just want to
> consolidate the s390-32/s390-64 implementation?

I'm working to consolidate the dynamic linking of the unwinder for the
backtrace function and for pthread_cancel/pthread_exit.  That's how I
encountered the issue.

> On 2/10/20 7:04 PM, Florian Weimer wrote:
>> Stefan,
>>
>> do you agree this change is correct?  It's not really clear to me how
>> the fallback unwinder can work given its dependency on _Unwind_GetIP.
> The current situation:
>
> In init, the libgcc_s.so.1 functions _Unwind_Backtrace and
> _Unwind_GetIP are dlsym'ed. If _Unwind_GetIP is not available, then
> _Unwind_Backtrace is also marked as not available.
>
> In __backtrace, there is a check if _Unwind_Backtrace and implicitly
> _Unwind_GetIP is available.
> If not available, then the __backchain_backtrace is used.
> Otherwise the _Unwind_Backtrace from libgcc is used.

Ah, I somehow misread the code.  __backchain_backtrace does not actually
call backtrace_helper, so it does not depend on _Unwind_GetIP.

> __backchain_backtrace does not rely on libgcc_s.so.1.
> Instead it tries to walk along the backchain (according to the
> ABI). But I assume the usual binaries do not have those backchain
> pointers as you have to use the gcc -mbackchain flag: "Maintain
> backchain pointer.". If not activated, the corresponding
> backchain-slots on the stack are not set and contain random values.

> As far as I know, there are customers which build with this backchain
> feature. Thus we don't want to remove it.

> _Unwind_Backtrace from libgcc can't handle the backchain. It relies on
> gcc -fasynchronous-unwind-tables.
>
> That means, if somebody has no libgcc and has not build with
> -mbackchain, then __backchain_backtrace will try to walk anywhere and
> we could get segfaults as it seems the sanity checks (backchain-slot
> == NULL or backchain-slot > __libc_stack_end) are not sufficient!?
>
> This also means, if somebody has build with backchain and has libgcc
> installed, then always _Unwind_Backtrace is called.
> If the binary was build with gcc -fno-asynchronous-unwind-tables, then
> there is no retry with __backchain_backtrace and there won't be a
> backtrace.
> If the binary contains the unwind-tables, then those are used and not
> the backchain.

That sounds like a problem, but it's not the one I originally expected.

I still think the code change in my patch is valid because pretty much
all systems will have libgcc_s installed (it's required for C++, after
all), and thus the backchain processing is essentially dead.  It's only
used of dlopen fails (e.g., due to lack of file descriptors).  But
that's not useful because the fallback implements something completely
different.

> Do you know how other architectures behave if there are no
> unwind-tables?

There is considerable disagreement whether unwind tables (asynchronous
or not) are part of the ABI.  I think they are, but people trying to
build the smallest binaries disagree quite vehemently.

Some targets say that if a frame does not have unwinding information,
then the presence of a frame pointer must be assumed.  But that's far
from uniform.  On other targets, lack of unwinding information means
that the current function is a leaf function which does not change the
stack pointer.

> Would it be possible to check if the current binaries have
> unwind-tables and then use _Unwind_Backtrace if libgcc is available?
> If not, __backchain_backtrace could be used as fallback.
>
> Should __backchain_backtrace have more checks regarding addresses in
> backchain-slot and/or the assumed function-address?

I think this has to be part of the libgcc_s unwinder because in many
environments, the system libraries have DWARF unwinding data, but other
objects may not have them and could use the backchain.  Then the
question becomes how to mark frames as using the backchain-based
unwinder.

Thanks,
Florian
  

On 2/11/20 4:44 PM, Florian Weimer wrote:
> * Stefan Liebler:
> 
>> Hi Florian,
>>
>> is there a background story behind this patch or do you just want to
>> consolidate the s390-32/s390-64 implementation?
> 
> I'm working to consolidate the dynamic linking of the unwinder for the
> backtrace function and for pthread_cancel/pthread_exit.  That's how I
> encountered the issue.
> 
>> On 2/10/20 7:04 PM, Florian Weimer wrote:
...
>> That means, if somebody has no libgcc and has not build with
>> -mbackchain, then __backchain_backtrace will try to walk anywhere and
>> we could get segfaults as it seems the sanity checks (backchain-slot
>> == NULL or backchain-slot > __libc_stack_end) are not sufficient!?
>>
>> This also means, if somebody has build with backchain and has libgcc
>> installed, then always _Unwind_Backtrace is called.
>> If the binary was build with gcc -fno-asynchronous-unwind-tables, then
>> there is no retry with __backchain_backtrace and there won't be a
>> backtrace.
>> If the binary contains the unwind-tables, then those are used and not
>> the backchain.
> 
> That sounds like a problem, but it's not the one I originally expected.
> 
> I still think the code change in my patch is valid because pretty much
> all systems will have libgcc_s installed (it's required for C++, after
> all), and thus the backchain processing is essentially dead.  It's only
> used of dlopen fails (e.g., due to lack of file descriptors).  But
> that's not useful because the fallback implements something completely
> different.
> 
Please wait before removing it at all. We'll discuss this internally if 
removing / moving to libgcc_s is okay for us and try to figure out if 
the customer in mind is having a libgcc_s or not.

Bye,
Stefan
 >
...
>> Should __backchain_backtrace have more checks regarding addresses in
>> backchain-slot and/or the assumed function-address?
> 
> I think this has to be part of the libgcc_s unwinder because in many
> environments, the system libraries have DWARF unwinding data, but other
> objects may not have them and could use the backchain.  Then the
> question becomes how to mark frames as using the backchain-based
> unwinder.
> 
> Thanks,
> Florian
>
  

* Stefan Liebler:

>> I still think the code change in my patch is valid because pretty much
>> all systems will have libgcc_s installed (it's required for C++, after
>> all), and thus the backchain processing is essentially dead.  It's only
>> used of dlopen fails (e.g., due to lack of file descriptors).  But
>> that's not useful because the fallback implements something completely
>> different.

> Please wait before removing it at all. We'll discuss this internally if 
> removing / moving to libgcc_s is okay for us and try to figure out if 
> the customer in mind is having a libgcc_s or not.

Absolutely.  There is no rush.  I can port over both versions to the
new libgcc_s loading code in the interim.  Thanks for considering this
change.