[PATCH,V4 4/9] include: sframe: doc: define new flag SFRAME_F_FDE_FUNC_START_ADDR_PCREL

[Indu Bhagat]

On 6/27/25 12:10 AM, Jan Beulich wrote:
> On 19.06.2025 08:24, Indu Bhagat wrote:
>> --- a/include/sframe-api.h
>> +++ b/include/sframe-api.h
>> @@ -36,6 +36,14 @@ typedef struct sframe_encoder_ctx sframe_encoder_ctx;
>>   #define MAX_OFFSET_BYTES  \
>>     ((SFRAME_FRE_OFFSET_4B * 2 * MAX_NUM_STACK_OFFSETS))
>>   
>> +/* Set of all defined flags known to this implementation.  Any flag other than
>> +   these, if set, is deemed invalid.  This is used for sanity checking to
>> +   ensure that the implementation only admits sections with flags defined in
>> +   the specific version.  */
>> +#define SFRAME_F_ALL_FLAGS \
>> +  (SFRAME_F_FDE_SORTED | SFRAME_F_FRAME_POINTER \
>> +   | SFRAME_F_FDE_FUNC_START_ADDR_PCREL)
> 
> While I think I can guess why you decided to put this here, ...
> 
>> --- a/include/sframe.h
>> +++ b/include/sframe.h
>> @@ -82,9 +82,15 @@ extern "C"
>>   /* Various flags for SFrame.  */
>>   
>>   /* Function Descriptor Entries are sorted on PC.  */
>> -#define SFRAME_F_FDE_SORTED	0x1
>> +#define SFRAME_F_FDE_SORTED		    0x1
>>   /* Functions preserve frame pointer.  */
>> -#define SFRAME_F_FRAME_POINTER 0x2
>> +#define SFRAME_F_FRAME_POINTER		    0x2
>> +/* Function start address in SFrame FDE is encoded as the distance from the
>> +   location of the sfde_func_start_address to the start PC of the function.
>> +   If absent, the function start address in SFrame FDE is encoded as the
>> +   distance from the start of the SFrame FDE section to the start PC of the
>> +   function.  */
>> +#define SFRAME_F_FDE_FUNC_START_ADDR_PCREL  0x4
>>   
>>   #define SFRAME_CFA_FIXED_FP_INVALID 0
>>   #define SFRAME_CFA_FIXED_RA_INVALID 0
> 
> ... I think it would be better overall if it was put next to where all the
> flags are defined. This way it's going to be less likely that an addition here
> would not be accompanied (right away) by an update to SFRAME_F_ALL_FLAGS.
> 

Okay.  I have now moved it to include/sframe.h like so:

/* Set of all defined flags in SFrame V2.  */
#define SFRAME_V2_F_ALL_FLAGS \
   (SFRAME_F_FDE_SORTED | SFRAME_F_FRAME_POINTER \
    | SFRAME_F_FDE_FUNC_START_PCREL)


>> --- a/libsframe/doc/sframe-spec.texi
>> +++ b/libsframe/doc/sframe-spec.texi
>> @@ -128,6 +128,17 @@ the data structure.
>>   @item
>>   The above two imply that each SFrame function descriptor entry has a fixed size
>>   of 20 bytes instead of its size of 17 bytes in SFrame format version 1.
>> +@item
>> +A new flag SFRAME_F_FDE_FUNC_START_ADDR_PCREL, released as an errata to SFrame
> 
> Nit: Aiui singular is "erratum". Also, when you use the word "released", don't
> you rather mean "as an erratum correction"? Finally, as to indentifier length:
> Do you think omitting the "ADDR" infix would be undue for whatever reason?
> 

Omitting "ADDR" infix: Sure, its a good idea.

Now I have:

  - Add a new flag SFRAME_F_FDE_FUNC_START_PCREL, as an erratum to SFrame
    Version 2, to indicate the encoding of the SFrame FDE function start
    address field:

>> --- a/libsframe/sframe.c
>> +++ b/libsframe/sframe.c
>> @@ -205,12 +205,11 @@ flip_fde (sframe_func_desc_entry *fdep)
>>   static bool
>>   sframe_header_sanity_check_p (sframe_header *hp)
>>   {
>> -  unsigned char all_flags = SFRAME_F_FDE_SORTED | SFRAME_F_FRAME_POINTER;
>>     /* Check preamble is valid.  */
>>     if (hp->sfh_preamble.sfp_magic != SFRAME_MAGIC
>>         || (hp->sfh_preamble.sfp_version != SFRAME_VERSION_1
>>   	  && hp->sfh_preamble.sfp_version != SFRAME_VERSION_2)
>> -      || (hp->sfh_preamble.sfp_flags | all_flags) != all_flags)
>> +      || hp->sfh_preamble.sfp_flags & ~SFRAME_F_ALL_FLAGS)
> 
> Nit: Please parenthesize the & expression, to clarify precedence.
> 

OK.