[PATCH] libsframe: handle SFrame FRE start/end IP offsets as unsigned

[Indu Bhagat]

On 5/23/25 1:37 AM, Jens Remus wrote:
> The SFrame FRE start address (fre_start_addr) is defined as unsigned
> 32-bit integer, as it is an offset from SFrame FDE function start
> address (sfde_func_start_address) and functions only grow upwards
> (towards higher addresses).
> 
> The SFrame FRE start IP offset is a synonym to the SFrame FRE start
> address.  The SFrame FRE end IP offset is either the value of the
> subsequent FDE start address minus one, if that exists, or the FDE
> function size minus one otherwise.  Both should therefore be handled
> as unsigned 32-bit integer.
> 
> In libsframe the "lookup PC" (pc) and SFrame FDE function start address
> (sfde_func_start_address) are both signed integers, as they are actually
> offsets from the SFrame section (.sframe).  The unsigned FDE start/end
> IP offsets may therefore only be safely compared against the offset of
> the lookup PC from FDE function start address if the FDE function start
> address is lower or equal to the lookup PC, as this guarantees the
> offset to be always positive:
> 
> Given:
> 
>    lookup_pc = pc - sframe_addr
> 
>    sfde_func_start_address = func_start_addr - sframe_addr
> 
> If the FDE function start address is lower or equal than the lookup PC,
> which both are signed offsets from SFrame section, then the function
> start address is also lower or equal to the PC, which are both unsigned:
> 
>    sfde_func_start_address <= lookup_pc
>    func_start_addr - sframe_addr <= pc - sframe_addr
>    func_start_addr <= pc
> 
> With that the offset of the lookup PC from FDE function start address
> (lookup_pc - sfde_func_start_address) must always be positive, if
> FDE function start address is lower or equal to the lookup PC:
> 
>    lookup_pc - sfde_func_start_address
>    = pc - sframe_addr - (func_start_addr - sframe_addr)
>    = pc - func_start_addr
> 
> libsframe/
> 	* sframe.c (sframe_find_fre): Define and handle start_ip_offset
> 	and end_ip_offset as unsigned (same as FRE fre_start_addr).
> 	(sframe_fre_check_range_p): Likewise.  Define PC offset (from
> 	function start address) as unsigned.  Assert that the FDE may
> 	apply to the PC.
> ---
>   libsframe/sframe.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/libsframe/sframe.c b/libsframe/sframe.c
> index e7b08ec6fa43..49931307f56d 100644
> --- a/libsframe/sframe.c
> +++ b/libsframe/sframe.c
> @@ -369,13 +369,13 @@ sframe_decoder_get_funcdesc_at_index (sframe_decoder_ctx *ctx,
>   
>   static bool
>   sframe_fre_check_range_p (sframe_func_desc_entry *fdep,
> -			  int32_t start_ip_offset, int32_t end_ip_offset,
> +			  uint32_t start_ip_offset, uint32_t end_ip_offset,
>   			  int32_t pc)
>   {
>     int32_t func_start_addr;
>     uint8_t rep_block_size;
>     uint32_t fde_type;
> -  int32_t pc_offset;
> +  uint32_t pc_offset;
>     bool mask_p;
>   
>     if (!fdep)
> @@ -386,6 +386,10 @@ sframe_fre_check_range_p (sframe_func_desc_entry *fdep,
>     mask_p = (fde_type == SFRAME_FDE_TYPE_PCMASK);
>     rep_block_size = fdep->sfde_func_rep_size;
>   
> +  /* Assert that FDE may apply to pc.  */
> +  sframe_assert (func_start_addr <= pc);
> +

Rather than assert, I think it is better to just handle the case and 
returning false instead.

if (func_start_addr > pc)
   return false;

> +  /* Given func_start_addr <= pc, pc - func_start_addr must be positive.  */
>     pc_offset = pc - func_start_addr;
>     /* For SFrame FDEs encoding information for repetitive pattern of insns,
>        masking with the rep_block_size is necessary to find the matching FRE.  */
> @@ -1096,9 +1100,8 @@ sframe_find_fre (sframe_decoder_ctx *ctx, int32_t pc,
>     sframe_frame_row_entry cur_fre;
>     sframe_func_desc_entry *fdep;
>     uint32_t fre_type, i;
> -  int32_t start_ip_offset;
>     int32_t func_start_addr;
> -  int32_t end_ip_offset;
> +  uint32_t start_ip_offset, end_ip_offset;
>     const char *fres;
>     size_t size = 0;
>     int err = 0;
> @@ -1115,6 +1118,7 @@ sframe_find_fre (sframe_decoder_ctx *ctx, int32_t pc,
>   
>     fres = ctx->sfd_fres + fdep->sfde_func_start_fre_off;
>     func_start_addr = fdep->sfde_func_start_address;
> +  sframe_assert (func_start_addr <= pc);
>   

I think this assert is not necessary.

We should rely the API sframe_get_funcdesc_with_addr_internal () doing 
the right thing.  If an SFrame FDE is found for the pc, the condition 
must be true.

If overall robustness still remains your concern, we can check for the 
case and do a sframe_ret_set_errno (errp, SFRAME_ERR_FDE_NOTFOUND);

>     for (i = 0; i < fdep->sfde_func_num_fres; i++)
>      {
> @@ -1125,8 +1129,9 @@ sframe_find_fre (sframe_decoder_ctx *ctx, int32_t pc,
>        start_ip_offset = cur_fre.fre_start_addr;
>        end_ip_offset = sframe_fre_get_end_ip_offset (fdep, i, fres + size);
>   
> -     /* Stop search if FRE's start_ip is greater than pc.  */
> -     if ((start_ip_offset + func_start_addr) > pc)
> +     /* Stop search if FRE's start_ip is greater than pc.  Given
> +	func_start_addr <= pc, pc - func_start_addr must be positive.  */
> +     if (start_ip_offset > (uint32_t)(pc - func_start_addr))
>          return sframe_set_errno (&err, SFRAME_ERR_FRE_INVAL);>
>        if (sframe_fre_check_range_p (fdep, start_ip_offset, end_ip_offset, pc))