[PATCH 2/3] gas: Use SFrame header and FDE field sizes when generating .sframe

[Indu Bhagat]

On 2/21/25 8:56 AM, Jens Remus wrote:
> The use of SFRAME_RELOC_SIZE in generation of SFrame stack trace
> information from CFI directives erroneously suggested that this could
> be used to configure a different relocation size.  But in practice it
> is tied to the SFrame field sizes it is used for and therefore cannot
> be changed.
> 
Hi Jens,

Thanks for improving SFrame implementation.

Right. Your patch is correct in that it identifies following issues:
   - #1. SFRAME_RELOC_SIZE may be mistakenly deemed configurable.
   - #2. Tieing fields like sfh_fre_len, or other sub-section offsets 
(sfh_fdeoff, sfh_freoff) to SFRAME_RELOC_SIZE is incorrect.

However, IMO having each individual emit_expr () use the sizeof_member 
() gives the impression that each affected member, especially those in 
category #2, can evolve independently (as the format evolves). This is 
not the case.

So perhaps it is clearer if we identify the two distinct members here 
and add them to the struct sframe_version_ops:
   - unsigned char addr_size
   - unsigned char subsection_offset_size (to avoid confusion with the 
SFrame stack offsets, if any)

Then initialize them to 4 in sframe_set_version () for SFRAME_VERSION_2. 
It still needs to be worked out how supporting FDE func start address of 
size 8 bytes will look like for SFRAME_VERSION_3, but I think adding 
these to the sframe_version_ops will be clearer.

WDYT ?

> Replace the uses of SFRAME_RELOC_SIZE by the size of the respective
> SFrame header and FDE fields when emitting SFrame information.  While
> at it enhance some comments.
> 
> gas/
> 	* gen-sframe.c (SFRAME_RELOC_SIZE): Delete.
> 	(sizeof_member): Define.
> 	(output_sframe_funcdesc): Use size of SFrame FDE fields instead
> 	of SFRAME_RELOC_SIZE.
> 	(output_sframe_internal): Use size of SFrame header fields
> 	instead of SFRAME_RELOC_SIZE.
> 
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
>   gas/gen-sframe.c | 29 +++++++++++++----------------
>   1 file changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index 13478efab6b9..575d4ef8afc9 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -26,9 +26,8 @@
>   
>   #ifdef support_sframe_p
>   
> -/* By default, use 32-bit relocations from .sframe into .text.  */
> -#ifndef SFRAME_RELOC_SIZE
> -# define SFRAME_RELOC_SIZE 4
> +#ifndef sizeof_member
> +# define sizeof_member(type, member)	(sizeof (((type *)0)->member))
>   #endif
>   
>   /* Whether frame row entries track RA.
> @@ -613,11 +612,9 @@ output_sframe_funcdesc (symbolS *start_of_fre_section,
>   			struct sframe_func_entry *sframe_fde)
>   {
>     expressionS exp;
> -  unsigned int addr_size;
>     symbolS *dw_fde_start_addrS, *dw_fde_end_addrS;
>     unsigned int pauth_key;
>   
> -  addr_size = SFRAME_RELOC_SIZE;
>     dw_fde_start_addrS = get_dw_fde_start_addrS (sframe_fde->dw_fde);
>     dw_fde_end_addrS = get_dw_fde_end_addrS (sframe_fde->dw_fde);
>   
> @@ -626,21 +623,24 @@ output_sframe_funcdesc (symbolS *start_of_fre_section,
>     exp.X_add_symbol = dw_fde_start_addrS; /* to location.  */
>     exp.X_op_symbol = symbol_temp_new_now (); /* from location.  */
>     exp.X_add_number = 0;
> -  emit_expr (&exp, addr_size);
> +  emit_expr (&exp, sizeof_member (sframe_func_desc_entry,
> +				  sfde_func_start_address));
>   
>     /* Size of the function in bytes.  */
>     exp.X_op = O_subtract;
>     exp.X_add_symbol = dw_fde_end_addrS;
>     exp.X_op_symbol = dw_fde_start_addrS;
>     exp.X_add_number = 0;
> -  emit_expr (&exp, addr_size);
> +  emit_expr (&exp, sizeof_member (sframe_func_desc_entry,
> +				  sfde_func_size));
>   
>     /* Offset to the first frame row entry.  */
>     exp.X_op = O_subtract;
>     exp.X_add_symbol = fre_symbol; /* Minuend.  */
>     exp.X_op_symbol = start_of_fre_section; /* Subtrahend.  */
>     exp.X_add_number = 0;
> -  emit_expr (&exp, addr_size);
> +  emit_expr (&exp, sizeof_member (sframe_func_desc_entry,
> +				  sfde_func_start_fre_off));
>   
>     /* Number of FREs.  */
>     out_four (sframe_fde->num_fres);
> @@ -681,9 +681,6 @@ output_sframe_internal (void)
>     unsigned char abi_arch = 0;
>     int fixed_fp_offset = SFRAME_CFA_FIXED_FP_INVALID;
>     int fixed_ra_offset = SFRAME_CFA_FIXED_RA_INVALID;
> -  unsigned int addr_size;
> -
> -  addr_size = SFRAME_RELOC_SIZE;
>   
>     /* The function descriptor entries as dumped by the assembler are not
>        sorted on PCs.  */
> @@ -738,26 +735,26 @@ output_sframe_internal (void)
>     out_four (num_fdes); /* Number of FDEs.  */
>     out_four (num_fres); /* Number of FREs.  */
>   
> -  /* FRE sub-section len.  */
> +  /* Size of FRE sub-section.  */
>     exp.X_op = O_subtract;
>     exp.X_add_symbol = end_of_frame_section;
>     exp.X_op_symbol = start_of_fre_section;
>     exp.X_add_number = 0;
> -  emit_expr (&exp, addr_size);
> +  emit_expr (&exp, sizeof_member (sframe_header, sfh_fre_len));
>   
> -  /* Offset of Function Index sub-section.  */
> +  /* Offset of FDE sub-section.  */
>     exp.X_op = O_subtract;
>     exp.X_add_symbol = end_of_frame_hdr;
>     exp.X_op_symbol = start_of_func_desc_section;
>     exp.X_add_number = 0;
> -  emit_expr (&exp, addr_size);
> +  emit_expr (&exp, sizeof_member (sframe_header, sfh_fdeoff));
>   
>     /* Offset of FRE sub-section.  */
>     exp.X_op = O_subtract;
>     exp.X_add_symbol = start_of_fre_section;
>     exp.X_op_symbol = end_of_frame_hdr;
>     exp.X_add_number = 0;
> -  emit_expr (&exp, addr_size);
> +  emit_expr (&exp, sizeof_member (sframe_header, sfh_freoff));
>   
>     symbol_set_value_now (end_of_frame_hdr);
>     symbol_set_value_now (start_of_func_desc_section);