Home Original page

[PATCH 3/5] libsframe: make flip_fde version aware

Indu Bhagat indu.bhagat@oracle.com
Fri Oct 17 19:57:21 GMT 2025 
Hi Jens,

Thanks for reviewing the series.

On 10/17/25 3:54 AM, Jens Remus wrote:
> Hello Indu!
> 
> On 10/17/2025 9:05 AM, Indu Bhagat via Binutils wrote:
>> Future versions of the format may have a different representation of an
>> SFrame FDE.  As the format evolves, endian flipping will need to be version
>> aware.
>>
>> Refactor flip_fde a bit by carving out an internal sframe_decode_fde API
>> which can read information from an on-disk SFrame FDE.
>>
>> libsframe/
>>          * sframe.c (flip_fde): Make version aware.
>>          (sframe_decode_fde): New internal definition.
>>          (flip_sframe): Use the new definitions.
> 
>> diff --git a/libsframe/sframe.c b/libsframe/sframe.c
> 
>> @@ -198,13 +198,30 @@ flip_header (sframe_header *sfheader)
>>     swap_thing (sfheader->sfh_freoff);
>>   }
>>   
>> -static void
>> -flip_fde (sframe_func_desc_entry *fdep)
>> +/* Endian flip the SFrame FDE at BUF, given the SFrame version VER.  Update
>> +   the FDE_SIZE to the size of the SFrame FDE flipped.
>> +
>> +   Return SFRAME_ERR if any error.  If error code is returned, the flipped FDEP
>> +   should not be used.  */
>> +
>> +static int
>> +flip_fde (char *buf, uint8_t ver, size_t *fde_size)
> 
> This is not strictly related to this patch, but I was astonished that
> that there is no checking whether buf still has *fde_size bytes left.
> Maybe pass the remaining buffer size into this function and have it
> check?
> 
> flip_fde (char *buf, size_t size, uint8_t ver, size_t *fde_size)
> 

I added this.

>>   {
>> -  swap_thing (fdep->sfde_func_start_address);
>> -  swap_thing (fdep->sfde_func_size);
>> -  swap_thing (fdep->sfde_func_start_fre_off);
>> -  swap_thing (fdep->sfde_func_num_fres);
>> +
>> +  if (ver == SFRAME_VERSION_2)
>> +    {
>> +      sframe_func_desc_entry_v2 *fdep = (sframe_func_desc_entry_v2 *) buf;
> 
>        if (size < sizeof (sframe_func_desc_entry_v2))
>          return SFRAME_ERR;
> 

Done.

> Or SFRAME_ERR_BUF_INVAL?  Or introduce a new error code
> SFRAME_ERR_BUF_OVERFLOW?
> 

So far, I have tried to follow the following for libsframe:
   - specific error codes are emitted via int err arg explicitly passed.
   - usage of sframe_set_errno (int *err, int errorcode)

(For most external facing APIs at least.)

Neither of flip_sframe, flip_fde etc have explicit arg ATM for error 
code.  These APIs simply return SFRAME_ERR in case of error, and the 
caller simply propagates SFRAME_ERR_BUF_INVAL upwards where applicable.

>> +      swap_thing (fdep->sfde_func_start_address);
>> +      swap_thing (fdep->sfde_func_size);
>> +      swap_thing (fdep->sfde_func_start_fre_off);
>> +      swap_thing (fdep->sfde_func_num_fres);
>> +
>> +      *fde_size = sizeof (sframe_func_desc_entry_v2);
>> +    }
>> +  else
>> +    return SFRAME_ERR; /* No other versions are possible ATM.  */
> 
> Why not SFRAME_ERR_VERSION_INVAL?
> 

See above for why not SFRAME_ERR_VERSION_INVAL.  Fine tuning the error 
code I think does not seem necessary here, we already do something 
sanity checks on the version apriori.

>> +
>> +  return 0;
>>   }
>>   
>>   /* Check if SFrame header has valid data.  */
>> @@ -428,6 +445,30 @@ sframe_fre_check_range_p (sframe_decoder_ctx *dctx, uint32_t func_idx,
>>     return (start_ip_offset <= pc_offset) && (end_ip_offset >= pc_offset);
>>   }
>>   
>> +/* Read the on-disk SFrame FDE of SFrame version VER from location BUF.
>> +
>> +   Return SFRAME_ERR if any error.  If error code is returned, the read values
>> +   should not be used.  */
>> +
>> +static int
>> +sframe_decode_fde (const char *buf, uint8_t ver, uint32_t *num_fres,
> 
> Same as above:
> 
> sframe_decode_fde (const char *buf, size_t size, uint8_t ver, uint32_t *num_fres,
> 
> 

Done.

>> +		   uint32_t *fre_type, uint32_t *fre_offset, size_t *fde_size)
>> +{
>> +  if (ver == SFRAME_VERSION_2)
>> +    {
>> +      sframe_func_desc_entry_v2 *fdep = (sframe_func_desc_entry_v2 *) buf;
> 
>        if (size < sizeof (sframe_func_desc_entry_v2))
>          return SFRAME_ERR;
> 
> Or SFRAME_ERR_BUF_INVAL or the like.
> 

Done with return SFRAME_ERR.

>> +      *num_fres = fdep->sfde_func_num_fres;
>> +      *fre_type = sframe_get_fre_type (fdep);
>> +      *fre_offset = fdep->sfde_func_start_fre_off;
>> +
>> +      *fde_size = sizeof (sframe_func_desc_entry_v2);
>> +    }
>> +  else
>> +    return SFRAME_ERR;
>> +
>> +  return 0;
>> +}
> Thanks and regards,
> Jens
More information about the Binutils mailing list