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

[Jens Remus]

On 7/29/2025 9:09 AM, Indu Bhagat wrote:
> 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);

Since errp is an optional pointer this won't work.  Either we leave it
as is or I change it as follows, so that SFRAME_ERR is returned in case
of an error:

 sframe_fre_get_cfa_offset (sframe_decoder_ctx *dctx,
                           sframe_frame_row_entry *fre, int *errp)
 {
-  int32_t offset = sframe_get_fre_offset (fre, SFRAME_FRE_CFA_OFFSET_IDX, errp);
+  int err;
+  int32_t offset = sframe_get_fre_offset (fre, SFRAME_FRE_CFA_OFFSET_IDX, &err);

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

+  if (errp)
+    *errp = err;
   return offset;
 }

Maybe I should add a preparatory patch that contains all the changes
that add support for FREs without any offsets, which would include the
above?

> 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...

Why not keep the existing SFRAME_ERR_FREOFFSET_NOPRESENT and ...

>  - In sframe_fre_get_ra_offset:
>   /* If the RA offset was not being tracked, return the fixed RA offset
>      from the SFrame header.  */

  _SFRAME_ITEM (SFRAME_ERR_RA_UNDEFINED, "Undefined return address.")

  if (sframe_fre_get_ra_undefined_p (fre))
    return sframe_set_errno (errp, SFRAME_ERR_RA_UNDEFINED);

If you want to distinguish the no offset / RA undefined case from
SFRAME_ERR_FREOFFSET_NOPRESENT on architectures that don't track
RA.

>   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.

I agree, I do prefer the RA undefined API too.  What about the following
order of patches:

Support for SFrame FDEs without any FREs
Support for SFrame FREs without any offsets
Add API to get RA undefined
Represent .cfi_undefined RA as FRE without offsets

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/