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

[DJ Delorie]

Aside from the __ issue, LGTM although I include some comments for
improvements.

claudiu.zissulescu-ianculescu@oracle.com writes:
> +/* 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

Worse, the application may be using shared objects compiled differently.
Also, glibc has some callback cases that need to be considered.
Example: Application calls shared object A which calls shared object B
which calls qsort() in glibc which calls a helper in shared object C
which raises SIGFPE which is handled by object D which asks for a
backtrace; each of which may or may not have SFrame encoding.

I think at this point a "all is SFrame or we fallback to all is dwarf"
is probably acceptable to get SFrame up and running so that it can be
further tested and validated, but at some point in the future, we need
to handle hybrid stacks.

For an example of how this could work, see gcc's EH code, which checks
the type of each frame as it unwinds.

> +   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)			\

I don't think this should be a macro, as it's only used once and hides
the fact that it returns.  That makes the code where it is used harder
to understand.  At the very least...

> +  /* Try first the SFrame backtracer.  */
> +  DO_SFRAME_BACKTRACE (array, size);

this comment should *say* that there's a hidden return here.

> +int32_t
> +sframe_fre_get_cfa_offset (sframe_decoder_ctx *dctx __attribute__ ((__unused__)),
> +			   sframe_frame_row_entry *fre,
> +			   _Unwind_Reason_Code *errp)

Non-static functions that aren't API must begin with __ (as was noted in
a previous review).

> +int32_t
> +sframe_fre_get_fp_offset (sframe_decoder_ctx *dctx,
> +			  sframe_frame_row_entry *fre,
> +			  _Unwind_Reason_Code *errp)

Same here.

> +int32_t
> +sframe_fre_get_ra_offset (sframe_decoder_ctx *dctx,
> +			  sframe_frame_row_entry *fre,
> +			  _Unwind_Reason_Code *errp)

And here.

> +_Unwind_Reason_Code
> +sframe_decode (sframe_decoder_ctx *dctx, const char *sf_buf)

And here.

> +_Unwind_Reason_Code
> +sframe_find_fre (sframe_decoder_ctx *ctx, int32_t pc,
> +		 sframe_frame_row_entry *frep)

And here.

> +int
> +stacktrace_sframe (void **ra_lst, int count, frame *frame)
> +{
> +	  /* I cannot find any FREs, try to see if it is a signal
> +	     frame, and if so decode it.  */

FYI Typically we avoid the "I" form unless you're writing a comment for
another developer to read, like "I couldn't figure out how to do this
better."  For documenting logic flow, use a view-neutral form like "If
there are no more FREs, ..."