[PATCH,V4 6/9] bfd: gas: ld: libsframe: adopt new encoding for FDE func start addr field

[Jan Beulich]

On 19.06.2025 08:24, Indu Bhagat wrote:
> --- a/bfd/elf-sframe.c
> +++ b/bfd/elf-sframe.c
> @@ -328,6 +328,8 @@ _bfd_elf_merge_section_sframe (bfd *abfd,
>    int8_t sfd_ctx_fixed_ra_offset;
>    uint8_t dctx_version;
>    uint8_t ectx_version;
> +  uint8_t dctx_flags;
> +  uint8_t ectx_flags;
>    int encerr = 0;
>  
>    struct elf_link_hash_table *htab;
> @@ -351,6 +353,8 @@ _bfd_elf_merge_section_sframe (bfd *abfd,
>    if (sfd_ctx == NULL || sfe_info == NULL)
>      return false;
>  
> +  dctx_flags = sframe_decoder_get_flags (sfd_ctx);
> +
>    if (htab->sfe_info.sfe_ctx == NULL)
>      {
>        sfd_ctx_abi_arch = sframe_decoder_get_abi_arch (sfd_ctx);
> @@ -361,8 +365,18 @@ _bfd_elf_merge_section_sframe (bfd *abfd,
>        if (!sfd_ctx_abi_arch)
>  	return false;
>  
> +      /* In-memory FDEs in the encoder object are unsorted during linking and
> +	 will be sorted before emission.  Reset SFRAME_F_FDE_SORTED to aptly
> +	 reflect that (doing so has no other functional value at this time
> +	 though).  */
> +      uint8_t tflags = dctx_flags & ~SFRAME_F_FDE_SORTED;
> +      /* ld always generates an output section with
> +	 SFRAME_F_FDE_FUNC_START_ADDR_PCREL flag set.  Later using
> +	 SFRAME_F_LD_MUSTHAVE_FLAGS, it is enforced that the provided input
> +	 sections also have this flag set.  */
> +      tflags |= SFRAME_F_FDE_FUNC_START_ADDR_PCREL;
>        htab->sfe_info.sfe_ctx = sframe_encode (SFRAME_VERSION_2,
> -					      0, /* SFrame flags.  */
> +					      tflags, /* SFrame flags.  */
>  					      sfd_ctx_abi_arch,
>  					      sfd_ctx_fixed_fp_offset,
>  					      sfd_ctx_fixed_ra_offset,
> @@ -410,7 +424,15 @@ _bfd_elf_merge_section_sframe (bfd *abfd,
>  	  " .sframe generation"));
>        return false;
>      }
> -
> +  /* Check that all SFrame sections being linked have harmonious flags.  */

Nit: Please can you retain blank lines like the one you remove here? (It
was odd to have two successive blank lines here prior to this change, yes.)

> +  ectx_flags = sframe_encoder_get_flags (sfe_ctx);
> +  if ((dctx_flags ^ ectx_flags) & SFRAME_F_LD_MUSTHAVE_FLAGS)

For this kind of check, then name of the new constant is misleading. The
"MUSTHAVE" in there suggests every input (or every caller) has to set all
of those flags. Question is whether you don't really mean here what the
name says:

  if (!(dctx_flags & ectx_flags & SFRAME_F_LD_MUSTHAVE_FLAGS))

? Of course ...

> +    {
> +      _bfd_error_handler
> +	(_("input SFrame sections with different FDE encoding prevent"
> +	  " .sframe generation"));

... the wording here suggests the ^ you have is meant. IOW its pretty
confusing right now. It's further odd (to me at least) that both decoder
and encoder flags would be related to the input section: Isn't the
encoder dealing with the output?

> @@ -431,6 +453,7 @@ _bfd_elf_merge_section_sframe (bfd *abfd,
>        bool pltn_reloc_by_hand = false;
>        unsigned int pltn_r_offset = 0;
>        uint8_t rep_block_size = 0;
> +      uint32_t offsetof_fde_in_sec = 0;

Btw, the scope of especially such single-use helper variables would
better be ...

> @@ -481,6 +504,15 @@ _bfd_elf_merge_section_sframe (bfd *abfd,
>  		address += sframe_read_value (abfd, contents,
>  					      pltn_r_offset, 4);
>  	      address += (sec->output_offset + r_offset);
> +	      /* SFrame FDE function start address is an offset from the
> +		 sfde_func_start_address field to the start PC.  The
> +		 calculation below is the distance of sfde_func_start_address
> +		 field from the start of the output SFrame section.  */
> +	      offsetof_fde_in_sec
> +		= sframe_encoder_get_offsetof_fde_start_addr (sfe_ctx,
> +							      cur_fidx + num_enc_fidx,
> +							      NULL);
> +	      address -= offsetof_fde_in_sec;

... the narrowest possible one.

> --- a/bfd/elfxx-x86.c
> +++ b/bfd/elfxx-x86.c
> @@ -1891,7 +1891,7 @@ _bfd_x86_elf_create_sframe_plt (bfd *output_bfd,
>      }
>  
>    *ectx = sframe_encode (SFRAME_VERSION_2,
> -			 0,
> +			 SFRAME_F_FDE_FUNC_START_ADDR_PCREL,
>  			 SFRAME_ABI_AMD64_ENDIAN_LITTLE,
>  			 SFRAME_CFA_FIXED_FP_INVALID,
>  			 -8, /*  Fixed RA offset.  */

I'm curious: How come only x86 has such a call (that now needs modifying)?
Didn't Arm64 gain SFrame support already as well?

> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -665,6 +665,10 @@ output_sframe_internal (void)
>    /* The function descriptor entries as dumped by the assembler are not
>       sorted on PCs.  */
>    unsigned char sframe_flags = 0;
> +  /* Fix PR ld/32666 - Incorrect .rela.sframe when using ld -r.
> +     With the fix now in place, we indicate the new encoding with an additional
> +     flag in SFrame Version 2.  */
> +  sframe_flags |= SFRAME_F_FDE_FUNC_START_ADDR_PCREL;
>  
>    unsigned int num_fdes = get_num_sframe_fdes ();
>    unsigned int num_fres = get_num_sframe_fres ();

While generally mixing declarations and statements is now deemed acceptable,
I'd still like to ask that we don't over-extend the use thereof. In the case
here I don't see why the two comments can't be joined and the initializer
simply be updated. Looks like the new part of the comment could also be less
verbose without losing information: "PR ld/32666: Indicated the new encoding
with ..."

> --- a/libsframe/sframe-dump.c
> +++ b/libsframe/sframe-dump.c
> @@ -133,8 +133,15 @@ dump_sframe_func_with_fres (sframe_decoder_ctx *sfd_ctx,
>    /* Get the SFrame function descriptor.  */
>    sframe_decoder_get_funcdesc (sfd_ctx, funcidx, &num_fres,
>  			       &func_size, &func_start_address, &func_info);
> -  /* Calculate the virtual memory address for function start pc.  */
> +/* Calculate the virtual memory address for function start pc.  Some older

Nit: You're screwing up indentation here.

> @@ -1691,15 +1723,23 @@ sframe_encoder_add_funcdesc_v2 (sframe_encoder_ctx *encoder,
>  static int
>  sframe_sort_funcdesc (sframe_encoder_ctx *encoder)
>  {
> -  sframe_header *ehp;
> +  sframe_header *ehp = sframe_encoder_get_header (encoder);
>  
> -  ehp = sframe_encoder_get_header (encoder);
>    /* Sort and write out the FDE table.  */
>    sf_fde_tbl *fd_info = encoder->sfe_funcdesc;
>    if (fd_info)
>      {
> +      for (unsigned int i = 0; i < fd_info->count; i++)
> +	fd_info->entry[i].sfde_func_start_address
> +	  += sframe_encoder_get_offsetof_fde_start_addr (encoder, i, NULL);
> +
>        qsort (fd_info->entry, fd_info->count,
>  	     sizeof (sframe_func_desc_entry), fde_func);
> +
> +      for (unsigned int i = 0; i < fd_info->count; i++)
> +	fd_info->entry[i].sfde_func_start_address
> +	  -= sframe_encoder_get_offsetof_fde_start_addr (encoder, i, NULL);

Especially when there are many entries but they are nearly sorted already,
isn't this adding undue overhead? The problem being that, as it looks, in
fde_func() you have no way to get hold of "encoder". Question nevertheless
is whether there isn't another way of doing the address adjustment as
needed in fde_func() rather than globally (and twice) here.

> --- a/libsframe/testsuite/libsframe.encode/encode-1.c
> +++ b/libsframe/testsuite/libsframe.encode/encode-1.c
> @@ -41,7 +41,7 @@ add_fde1 (sframe_encoder_ctx *encode, int idx)
>  
>    unsigned char finfo = sframe_fde_create_func_info (SFRAME_FRE_TYPE_ADDR1,
>  						     SFRAME_FDE_TYPE_PCINC);
> -  err = sframe_encoder_add_funcdesc (encode, 0xfffff03e, 0x1b, finfo, 4);
> +  err = sframe_encoder_add_funcdesc (encode, 0xfffff022, 0x1b, finfo, 4);
>    if (err == -1)
>      return err;
>  
> @@ -66,7 +66,7 @@ add_fde2 (sframe_encoder_ctx *encode, int idx)
>  
>    unsigned char finfo = sframe_fde_create_func_info (SFRAME_FRE_TYPE_ADDR1,
>  						     SFRAME_FDE_TYPE_PCINC);
> -  err = sframe_encoder_add_funcdesc (encode, 0xfffff059, 0x10, finfo, 4);
> +  err = sframe_encoder_add_funcdesc (encode, 0xfffff029, 0x10, finfo, 4);
>    if (err == -1)
>      return err;

Hmm, more magic numbers that are now being adjusted in an entirely opaque
way.

Jan