[PATCH v6 4/5] elf: Add SFrame stack tracing

[Florian Weimer]

* claudiu zissulescu-ianculescu:

> From: Claudiu Zissulescu <claudiu.zissulescu-ianculescu@oracle.com>
>
> This patch adds the necessary bits to enable stack tracing using
> SFrame.  In the case the new SFrame stack tracing procedure doesn't
> find SFrame related info, the stack tracing falls back on default
> Dwarf implementation.
>
> The new SFrame stack tracing procedure is added to bebug/backtrace.c
> file, the support functions are added in sysdeps folder, namely
> sframe.h, read-sframe.c and read-sfame.h.

Typo: [d]ebug/backtrace.c

Why do you place the support routines into sysdeps/generic/?  I don't see
any sysdeps overrides.

Global, non-installed include files can go into include/.

> diff --git a/debug/backtrace.c b/debug/backtrace.c
> index 05cdc84e8d..aca0b5ade9 100644
> --- a/debug/backtrace.c
> +++ b/debug/backtrace.c
> @@ -20,6 +20,7 @@
>  #include <stdlib.h>
>  #include <unwind.h>
>  #include <unwind-link.h>
> +#include <sframe.h>
>  
>  struct trace_arg
>  {
> @@ -30,6 +31,29 @@ struct trace_arg
>    int size;
>  };
>  
> +/* Initialize the SFrame backtrace routine, and try to backtrace the
> +   current stack using SFrame info.  The return value of SFrame stack
> +   tracer must be larger than one to consider the SFrame backtrace
> +   valid. Otherwise, there may be the case that glibc is compiled
> +   using SFrame but the application not.  The SFrame backtracer can
> +   fallback on Dwarf unwinder also when it is a posibility to return a
> +   truncated trace.  This happens when a stack frame is detected
> +   missing SFrame information.  */
> +
> +#define DO_SFRAME_BACKTRACE(ARRAY, SIZE)			\
> +  do								\
> +    {								\
> +      int cnt;							\
> +      frame frame;						\
> +      frame.pc = getPC();					\
> +      frame.sp = getSP();					\
> +      frame.fp = (_Unwind_Ptr) __builtin_frame_address (0);	\
> +      cnt = stacktrace_sframe (ARRAY, SIZE, &frame);		\
> +      if (cnt > 1)						\
> +	return cnt;						\
> +    }								\
> +  while(0)
> +
>  static _Unwind_Reason_Code
>  backtrace_helper (struct _Unwind_Context *ctx, void *a)
>  {
> @@ -72,7 +96,14 @@ __backtrace (void **array, int size)
>       .cnt = -1
>      };
>  
> -  if (size <= 0 || arg.unwind_link == NULL)
> +  if (size <= 0)
> +    return 0;
> +
> +  /* Try first the SFrame backtracer.  */
> +  DO_SFRAME_BACKTRACE (array, size);
> +
> +  /* Try the dwarf unwinder.  */
> +  if (arg.unwind_link == NULL)
>      return 0;

I dislike the macro with the control flow.  As far as I can see, there
is just a single result value (cnt), so you could easily use an inline
function.  But it addresses my concern regarding truncated backtraces in
a mix DWARF/SFrame environment.  Still this would benefit from a test
that shows this works.

The internal functions should use __ prefixes (at least if they are
declared with external linkage), so that they do not conflict with
application definitions when linking statically.

> +int
> +stacktrace_sframe (void **ra_lst, int count, frame *frame)
> +{
> +  _Unwind_Ptr sframe_vma, cfa, return_addr, ra_stack_loc, fp_stack_loc, pc,
> +    frame_ptr;
> +  int cfa_offset, fp_offset, ra_offset, i;
> +  sframe_frame_row_entry fred, *frep = &fred;
> +
> +  if (!ra_lst || !count)
> +    return 0;
> +
> +  for (i = 0; i < count; i++)
> +    {
> +      _Unwind_Reason_Code err;
> +      struct dl_find_object data;
> +      sframe_decoder_ctx decoder_context, *dctx = &decoder_context;
> +
> +      /* Clean decoder context.  */
> +      memset (dctx, 0, sizeof (sframe_decoder_ctx));
> +
> +      /* Load and set up the SFrame stack trace info for pc.  */
> +      if (_dl_find_object_helper ((void *) frame->pc, &data) < 0)
> +	/* Force fallback to DWARF stacktracer.  */
> +	return 0;

Inconsistent tabs vs whitespace.

> +
> +      sframe_vma = (_Unwind_Ptr) data.dlfo_sframe;
> +      if (!sframe_vma)
> +	{
> +#ifdef MD_DECODE_SIGNAL_FRAME
> +	  /* I cannot find a valid SFrame section. Check if it is a
> +	     signal frame.  */
> +	  if (MD_DECODE_SIGNAL_FRAME (frame) == _URC_NO_REASON)
> +	    {
> +	      ra_lst[i] = (void *) frame->pc;
> +	      continue;
> +	    }
> +#endif
> +	  /* Force fallback to DWARF stacktracer.  */
> +	  return 0;
> +	}

I don't quite understand how this approach is valid.  Wouldn't we have
to check MD_DECODE_SIGNAL_FRAME (frame) first?  Otherwise we might get
incorrect results if _dl_find_object encounters a valid-looking code
address.

(Not a complete review, sorry.)

Thanks,
Florian