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

[Claudiu Zissulescu-Ianculescu]

Hi,

First, thank you, Florian, for all your effort in reviewing my patches!

As Florian is currently very busy, I’d like to ask if any other
maintainer would be willing to pick up and review my SFrame glibc
patches. I would really love to see them included in the upcoming glibc
release.

Thank you,
Claudiu

On 6/24/25 11:32 AM, Florian Weimer wrote:
> * 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
>