Re: [PATCH] s390: Remove backchain-based fallback from backtrace

[Stefan Liebler <stli at linux dot ibm dot com>]

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)