Re: [PATCH 07/11] s390: Implement backtrace on top of <unwind-link.h>

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

Hi Florian,

I've applied your patch series and also successfully run the testsuite 
on s390x/s390.

For s390, this is okay.



Just as info for future readers regarding
"The S/390 version preserves the backchain fallback for now.":
Here is the previous discussion:
https://www.sourceware.org/ml/libc-alpha/2020-02/msg00453.html
As soon as I have any news, I will reply there.

If we remove the backchain fallback. I assume the s390 specific 
backtrace.c files can be removed. Thus the debug/backtrace.c would be 
used. But I have to double check at this time.

If we still need the backchain fallback, I would either update the 
implementation in order to also get those "features":
/* Check whether we make any progress.  */
/* _Unwind_Backtrace seems to put NULL address above
   _start.  Fix it up here.  */

Or perhaps even better, add an architecture-fallback-hook in the default 
implementation.


Bye,
Stefan

On 2/13/20 6:08 PM, Florian Weimer wrote:
> ---
>   sysdeps/s390/s390-32/backtrace.c | 45 ++++++++++---------------------
>   sysdeps/s390/s390-64/backtrace.c | 46 ++++++++++----------------------
>   2 files changed, 28 insertions(+), 63 deletions(-)
>
> diff --git a/sysdeps/s390/s390-32/backtrace.c b/sysdeps/s390/s390-32/backtrace.c
> index 497e4f8875..21da6761f2 100644
> --- a/sysdeps/s390/s390-32/backtrace.c
> +++ b/sysdeps/s390/s390-32/backtrace.c
> @@ -17,12 +17,10 @@
>      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>
> +#include <unwind-link.h>
>   
>   /* This is a global variable set at program start time.  It marks the
>      highest used stack address.  */
> @@ -57,27 +55,11 @@ struct layout
>   struct trace_arg
>   {
>     void **array;
> +  struct unwind_link *unwind_link;
>     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)
>   {
> @@ -103,9 +85,6 @@ __backchain_backtrace (void **array, int size)
>   
>     return cnt;
>   }
> -#else
> -# define unwind_backtrace _Unwind_Backtrace
> -# define unwind_getip _Unwind_GetIP
>   #endif
>   
>   static _Unwind_Reason_Code
> @@ -116,7 +95,8 @@ backtrace_helper (struct _Unwind_Context *ctx, void *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);
> +    arg->array[arg->cnt]
> +      = (void *) UNWIND_LINK_PTR (arg->unwind_link, _Unwind_GetIP) (ctx);
>     if (++arg->cnt == arg->size)
>       return _URC_END_OF_STACK;
>     return _URC_NO_REASON;
> @@ -125,21 +105,24 @@ backtrace_helper (struct _Unwind_Context *ctx, void *a)
>   int
>   __backtrace (void **array, int size)
>   {
> -  struct trace_arg arg = { .array = array, .size = size, .cnt = -1 };
> +  struct trace_arg arg =
> +    {
> +     .array = array,
> +     .unwind_link = __libc_unwind_link_get (),
> +     .size = size,
> +     .cnt = -1
> +    };
>   
>     if (size <= 0)
>       return 0;
>   
>   #ifdef SHARED
> -  __libc_once_define (static, once);
> -
> -  __libc_once (once, init);
> -
> -  if (unwind_backtrace == NULL)
> +  if (arg.unwind_link == NULL)
>       return __backchain_backtrace (array, size);
>   #endif
>   
> -  unwind_backtrace (backtrace_helper, &arg);
> +  UNWIND_LINK_PTR (arg.unwind_link, _Unwind_Backtrace)
> +    (backtrace_helper, &arg);
>   
>     return arg.cnt != -1 ? arg.cnt : 0;
>   }
> diff --git a/sysdeps/s390/s390-64/backtrace.c b/sysdeps/s390/s390-64/backtrace.c
> index 5d14e01280..1b8818f219 100644
> --- a/sysdeps/s390/s390-64/backtrace.c
> +++ b/sysdeps/s390/s390-64/backtrace.c
> @@ -17,13 +17,10 @@
>      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>
> -
> +#include <unwind-link.h>
>   
>   /* This is a global variable set at program start time.  It marks the
>      highest used stack address.  */
> @@ -56,27 +53,11 @@ struct layout
>   struct trace_arg
>   {
>     void **array;
> +  struct unwind_link *unwind_link;
>     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)
>   {
> @@ -102,9 +83,6 @@ __backchain_backtrace (void **array, int size)
>   
>     return cnt;
>   }
> -#else
> -# define unwind_backtrace _Unwind_Backtrace
> -# define unwind_getip _Unwind_GetIP
>   #endif
>   
>   static _Unwind_Reason_Code
> @@ -115,7 +93,8 @@ backtrace_helper (struct _Unwind_Context *ctx, void *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);
> +    arg->array[arg->cnt]
> +      = (void *) UNWIND_LINK_PTR (arg->unwind_link, _Unwind_GetIP) (ctx);
>     if (++arg->cnt == arg->size)
>       return _URC_END_OF_STACK;
>     return _URC_NO_REASON;
> @@ -124,21 +103,24 @@ backtrace_helper (struct _Unwind_Context *ctx, void *a)
>   int
>   __backtrace (void **array, int size)
>   {
> -  struct trace_arg arg = { .array = array, .size = size, .cnt = -1 };
> +  struct trace_arg arg =
> +    {
> +     .array = array,
> +     .unwind_link = __libc_unwind_link_get (),
> +     .size = size,
> +     .cnt = -1
> +    };
>   
>     if (size <= 0)
>       return 0;
>   
>   #ifdef SHARED
> -  __libc_once_define (static, once);
> -
> -  __libc_once (once, init);
> -
> -  if (unwind_backtrace == NULL)
> +  if (arg.unwind_link == NULL)
>       return __backchain_backtrace (array, size);
>   #endif
>   
> -  unwind_backtrace (backtrace_helper, &arg);
> +  UNWIND_LINK_PTR (arg.unwind_link, _Unwind_Backtrace)
> +    (backtrace_helper, &arg);
>   
>     return arg.cnt != -1 ? arg.cnt : 0;
>   }