[PATCH v4 2/4] include: libsframe: Add API to get FRE offset count

[Indu Bhagat]

On 7/25/25 9:23 AM, Jens Remus wrote:
> This API will be later used by libsframe when dumping SFrame stack trace
> information (e.g. in objdump and readelf) to handle the special case of
> a SFrame FRE without any offsets.  Such FREs without any offsets will be
> used to represent RA undefined.  Other users of libsframe might need the
> same capability.
> 
> Rename the existing static function of same name to re-use its name for
> the global one.  The rename aligns the static function name to the
> likes.
> 
> While at it pass SFrame FRE as pointer to const to sanity check helper.
> 
> include/
> 	* sframe-api.h (sframe_fre_get_offset_count): New declaration.
> 
> libsframe/
> 	* libsframe.ver (sframe_fre_get_offset_count): List new API.
> 	* sframe.c (sframe_fre_get_offset_count): Rename to ...
> 	(sframe_get_fre_offset_count): ... this.
> 	(sframe_fre_sanity_check_p, sframe_fre_offset_bytes_size,
> 	flip_fre, sframe_get_fre_offset): Likewise.
> 	(sframe_fre_get_offset_count): New definition.
> 	(sframe_fre_sanity_check_p): Make frep pointer to const.
> 
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
> 
> Notes (jremus):
>      Changes in V4:
>      - New patch.
>      
>      Technically it would not be necessary to make sframe_fre_get_offset_count
>      global (i.e. list it in the version file), as libsframe could use it
>      in dumping SFrame. But other users of libsframe need the capability to
>      differentiate between a failure to get the CFA offset and a FRE without
>      any offsets.
>      

I think ideally we should now adapt the various sframe_fre_get_*_offset 
APIs too..

At the minimum, I think in sframe_fre_get_cfa_offset, we should check 
for errp:

   /* For s390x undo adjustment of CFA offset (to enable 8-bit offsets).  */
   if (!errp && sframe_decoder_get_abi_arch (dctx) == 
SFRAME_ABI_S390X_ENDIAN_BIG)
     offset = SFRAME_V2_S390X_CFA_OFFSET_DECODE (offset);

Next, IIUC currently sframe_get_fre_offset will return 
SFRAME_ERR_FREOFFSET_NOPRESENT when num FRE offsets is 0.

For sframe_fre_get_ra_offset and sframe_fre_get_fp_offset, returning the 
fixed RA offset or fixed FP offset respectively when 
sframe_fre_get_ra_undefined_p (fre) is true looks wierd.  So what do you 
think about:

  - adding a new enum in include/sframe-api.h (Not entirely sure if 
SFRAME_ERR_ZERO_FREOFFSETS is apt name / error in this context)
   _SFRAME_ITEM (SFRAME_ERR_ZERO_FREOFFSETS, "No FRE offsets present.")
   and return it in sframe_fre_get_offset...

  - In sframe_fre_get_ra_offset:
   /* If the RA offset was not being tracked, return the fixed RA offset
      from the SFrame header.  */
   if (ra_offset != SFRAME_CFA_FIXED_RA_INVALID
       && !sframe_fre_get_ra_undefined_p (fre))
     {
       if (errp)
         *errp = 0;
       return ra_offset;
     }
  - Similarly in sframe_fre_get_fp_offset...

What do you think ?

>      An alternative would be to add an API to test for RA undefined.  See
>      a following patch.
> 

I find the RA undefined API to be more direct.  But I am happy to go 
with your best judgement on this.  I do agree either one will suffice 
and that we do not need both.

>   include/sframe-api.h    |  6 ++++++
>   libsframe/libsframe.ver |  5 +++++
>   libsframe/sframe.c      | 24 ++++++++++++++++++------
>   3 files changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/include/sframe-api.h b/include/sframe-api.h
> index 8c26257fb643..02688e4edca4 100644
> --- a/include/sframe-api.h
> +++ b/include/sframe-api.h
> @@ -201,6 +201,12 @@ dump_sframe (sframe_decoder_ctx *decoder, uint64_t addr);
>   extern uint8_t
>   sframe_fre_get_base_reg_id (sframe_frame_row_entry *fre, int *errp);
>   
> +/* Get the number of offset from the FRE.  Set errp if failure.  */
> +
> +int
> +sframe_fre_get_offset_count (const sframe_decoder_ctx *dctx ATTRIBUTE_UNUSED,
> +			     const sframe_frame_row_entry *fre, int *errp);
> +
>   /* Get the CFA offset from the FRE.  If the offset is invalid, sets errp.  */
>   extern int32_t
>   sframe_fre_get_cfa_offset (sframe_decoder_ctx *dtcx,
> diff --git a/libsframe/libsframe.ver b/libsframe/libsframe.ver
> index 8cc80da2f26a..e02e29ae134f 100644
> --- a/libsframe/libsframe.ver
> +++ b/libsframe/libsframe.ver
> @@ -41,3 +41,8 @@ LIBSFRAME_2.0 {
>     local:
>       *;
>   } LIBSFRAME_0.0;
> +
> +LIBSFRAME_2.1 {
> +  global:
> +    sframe_fre_get_offset_count;
> +} LIBSFRAME_2.0;
> diff --git a/libsframe/sframe.c b/libsframe/sframe.c
> index 9814a3ae4c1f..108c2bf9d2a9 100644
> --- a/libsframe/sframe.c
> +++ b/libsframe/sframe.c
> @@ -114,7 +114,7 @@ sframe_get_hdr_size (sframe_header *sfh)
>   /* Access functions for frame row entry data.  */
>   
>   static uint8_t
> -sframe_fre_get_offset_count (uint8_t fre_info)
> +sframe_get_fre_offset_count (uint8_t fre_info)
>   {
>     return SFRAME_V1_FRE_OFFSET_COUNT (fre_info);
>   }
> @@ -284,7 +284,7 @@ sframe_fre_start_addr_size (uint32_t fre_type)
>   /* Check if the FREP has valid data.  */
>   
>   static bool
> -sframe_fre_sanity_check_p (sframe_frame_row_entry *frep)
> +sframe_fre_sanity_check_p (const sframe_frame_row_entry *frep)
>   {
>     uint8_t offset_size, offset_cnt;
>     uint8_t fre_info;
> @@ -300,7 +300,7 @@ sframe_fre_sanity_check_p (sframe_frame_row_entry *frep)
>         && offset_size != SFRAME_FRE_OFFSET_4B)
>       return false;
>   
> -  offset_cnt = sframe_fre_get_offset_count (fre_info);
> +  offset_cnt = sframe_get_fre_offset_count (fre_info);
>     if (offset_cnt > MAX_NUM_STACK_OFFSETS)
>       return false;
>   
> @@ -318,7 +318,7 @@ sframe_fre_offset_bytes_size (uint8_t fre_info)
>   
>     debug_printf ("offset_size =  %u\n", offset_size);
>   
> -  offset_cnt = sframe_fre_get_offset_count (fre_info);
> +  offset_cnt = sframe_get_fre_offset_count (fre_info);
>   
>     if (offset_size == SFRAME_FRE_OFFSET_2B
>         || offset_size == SFRAME_FRE_OFFSET_4B)	/* 2 or 4 bytes.  */
> @@ -442,7 +442,7 @@ flip_fre (char *fp, uint32_t fre_type, size_t *fre_size)
>     /* FRE info is uint8_t.  No need to flip.  */
>     fre_info = *(uint8_t*)fp;
>     offset_size = sframe_fre_get_offset_size (fre_info);
> -  offset_cnt = sframe_fre_get_offset_count (fre_info);
> +  offset_cnt = sframe_get_fre_offset_count (fre_info);
>   
>     /* Advance the buffer pointer to where the stack offsets are.  */
>     fre_info_size = sizeof (uint8_t);
> @@ -587,7 +587,7 @@ sframe_get_fre_offset (sframe_frame_row_entry *fre, int idx, int *errp)
>     if (fre == NULL || !sframe_fre_sanity_check_p (fre))
>       return sframe_set_errno (errp, SFRAME_ERR_FRE_INVAL);
>   
> -  offset_cnt = sframe_fre_get_offset_count (fre->fre_info);
> +  offset_cnt = sframe_get_fre_offset_count (fre->fre_info);
>     offset_size = sframe_fre_get_offset_size (fre->fre_info);
>   
>     if (offset_cnt < idx + 1)
> @@ -692,6 +692,18 @@ sframe_fre_get_base_reg_id (sframe_frame_row_entry *fre, int *errp)
>     return SFRAME_V1_FRE_CFA_BASE_REG_ID (fre_info);
>   }
>   
> +/* Get the number of offset from the FRE.  Set errp if failure.  */
> +
> +int
> +sframe_fre_get_offset_count (const sframe_decoder_ctx *dctx ATTRIBUTE_UNUSED,
> +			     const sframe_frame_row_entry *fre, int *errp)
> +{
> +  if (fre == NULL || !sframe_fre_sanity_check_p (fre))
> +    return sframe_set_errno (errp, SFRAME_ERR_FRE_INVAL);
> +
> +  return sframe_get_fre_offset_count (fre->fre_info);
> +}
> +
>   /* Get the CFA offset from the FRE.  If the offset is invalid, sets errp.  */
>   
>   int32_t