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

[Jan Beulich]

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.

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

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

Jan