[PATCH v3 2/5] elf: Add SFrame stack tracing

[Jens Remus]

Hello Claudiu,

thank you for posting v3 based on latest master!  This eased applying it
locally for review a lot.  Rebasing my experimental s390x support on top
surfaced the following minor issues.  Additionally I wonder whether
a local variable for the FP value would make the code cleaner.

On 16.04.2025 11:42, claudiu.zissulescu-ianculescu@oracle.com wrote:

> 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.
> 
> Signed-off-by: Claudiu Zissulescu <claudiu.zissulescu-ianculescu@oracle.com>

> diff --git a/sysdeps/generic/sframe.c b/sysdeps/generic/sframe.c
> new file mode 100644

> +      /* RA offset is available, get the value stored in the stack
> +       location.  */

Nit: Bad indentation.

> +      ra_stack_loc = cfa + ra_offset;
> +      return_addr = read_stack_value (ra_stack_loc);
> +
> +      ra_lst[i] = (void *)return_addr;
> +
> +      /* Set up for the next frame.  */

Maybe move comment (see below)?

> +      /* Get the FP offset from the FRE.  If the offset is
> +	 unavailable, sets err.  */
> +      fp_offset = sframe_fre_get_fp_offset (dctx, frep, &err);
> +      if (err == _URC_NO_REASON)
> +	{
> +	  /* Frame is aavailable, get the value stored in the stack
> +	     location.  */

Nit: s/aavailable/available/. Maybe also s/Frame/FP offset/?

> +	  fp_stack_loc = cfa + fp_offset;
> +	  frame->fp = read_stack_value (fp_stack_loc);

This is the only frame field being set immediately.  While it cannot
occur that frame->fp gets set and below is not executed, I wonder
whether introducing a local variable frame_ptr (or fp and renaming
return_addr to ra) and changing below as follows would be cleaner?

> +	}
> +

       /* Set up for the next frame.  */

> +      frame->sp = cfa + SFRAME_SP_VAL_OFFSET;

       frame->fp = frame_ptr;

> +      frame->pc = return_addr;
> +    }
> +  return i;
> +}

Thanks and regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303)
+49-7031-16-1128 Office
jremus@de.ibm.com

IBM

IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/