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

[Jan Beulich]

On 13.06.2025 09:32, Indu Bhagat wrote:
> This patch convenes a set of changes in bfd, gas, ld, libsframe towards
> moving to the new encoding for the 'sfde_func_start_address' field in
> SFrame FDE.
> 
> First, gas must now mark all SFrame sections with the new flag
> SFRAME_F_FDE_FUNC_START_ADDR_PCREL.  gas was already emitting the field
> in the said encoding.

Looks like I'm lost again: If gas did things the "new way" already, why would
a change be needed? And how would one distinguish "new with flag", "new without
flag", and "old without flag"?

> 	* gas/gen-sframe.c (output_sframe_internal): Emit the flag
> 	SFRAME_F_FDE_FUNC_START_ADDR_PCREL.
> 
> Similarly for ld, adopt the new semantics of sfde_func_start_address
> consistently.  This means:
>   - When merging SFrame sections, check that all input SFrame sections
>     have the SFRAME_F_FDE_FUNC_START_ADDR_PCREL flag set.  If the check
>     fails, ld errors out.
>   - When merging SFrame sections, keep even the in-memory contents of
>     the FDE function start address (buffer passed to libsframe
>     sframe_encoder_write () for writing out) are encoded in the new
>     semantics.  While it is, in theory, possible that instead of doing this
>     change here, we adjust the value of sfde_func_start_address at the final
>     write (sframe_encoder_write) time.  But latter is not favorable for
>     maintenanance and may be generally confusing for developers.
>   - When creating SFrame for PLT entries, emit flag
>     SFRAME_F_FDE_FUNC_START_ADDR_PCREL.
> 
> bfd/
> 	* elf-sframe.c (_bfd_elf_merge_section_sframe): Check for flag
> 	SFRAME_F_FDE_FUNC_START_ADDR_PCREL set for all input bfds.  If
> 	not, error out.  Also, adopt the new semantics of function start
> 	address encoding.
> 	* bfd/elfxx-x86.c (_bfd_x86_elf_create_sframe_plt): Emit flag
> 	SFRAME_F_FDE_FUNC_START_ADDR_PCREL.
> 
> Next, for dumping SFrame sections, now that we are emitting the same
> encoding in GAS, non-relocatable and relocatable SFrame links, it is the
> time to set relocate to TRUE in debug_displays[].
> 
> binutils/
> 	* dwarf.c (struct dwarf_section_display): Allow sframe sections
> 	  to now be relocated.
> gas/testsuite/
> 	* gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.d: Update the
> 	test.  Relocatable SFrame sections now display non-zero value
> 	(appropriate function start address).
> 
> Now, as the SFrame sections on-disk and in-memory use the new semantics of
> sfde_func_start_address encoding (i.e., function start address is the
> offset from the sfde_func_start_address field to the start PC), the
> calculation to make it human readable (i.e., relatable to the addresses
> in .text sections) needs adjustment.
> 
> libsframe/
> 	* sframe-dump.c (dump_sframe_func_with_fres): Adjust the
> 	function start address for dumping.
> 
> Now that both the emission of the new encoding, and the relocation of
> sections before dumping them is in place, it is time to adjust the
> testcases.
> 
> gas/testsuite/
> 	* gas/cfi-sframe/cfi-sframe-aarch64-1.d: Update expected output
> 	to include SFRAME_F_FDE_FUNC_START_ADDR_PCREL instead of NONE.
> 	* gas/cfi-sframe/cfi-sframe-aarch64-2.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-aarch64-3.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-aarch64-4.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-common-1.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-common-10.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-common-11.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-common-2.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-common-3.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-common-4.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-common-5.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-common-6.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-common-7.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-common-8.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-common-9.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-x86_64-1.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-x86_64-2.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-x86_64-empty-1.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-x86_64-empty-2.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-x86_64-empty-3.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-x86_64-empty-4.d: Likewise.
> 	* gas/cfi-sframe/common-empty-1.d: Likewise.
> 	* gas/cfi-sframe/common-empty-2.d: Likewise.
> 	* gas/cfi-sframe/common-empty-3.d: Likewise.
> 	* gas/scfi/x86_64/scfi-cfi-sections-1.d: Likewise.
> 	* gas/scfi/x86_64/scfi-dyn-stack-1.d: Likewise.
> ld/testsuite/
> 	* ld-aarch64/sframe-simple-1.d: Update expected output to
> 	include SFRAME_F_FDE_FUNC_START_ADDR_PCREL.
> 	* ld-x86-64/sframe-ibt-plt-1.d: Likewise.
> 	* ld-x86-64/sframe-plt-1.d: Likewise.
> 	* ld-x86-64/sframe-pltgot-1.d: Likewise.
> 	* ld-x86-64/sframe-pltgot-2.d: Likewise.
> 	* ld-x86-64/sframe-simple-1.d: Likewise.
> 
> Naturally, the change of semantics for 'SFrame FDE function start address'
> has consequences on the implementation in libsframe.  As per the new
> semantics:
>   - Function start address in the SFrame FDE (sfde_func_start_address)
>     is an offset from the FDE function start address field to the start
>     PC of the associated function.
> 
> Note that, the libsframe library brings the SFrame section contents into
> its own memory to create a sframe_decoder_ctx object via sframe_decode
> ().  Many internal and user-interfacing APIs then use sframe_decoder_ctx
> object to interact and fulfill the work.
> 
> In context of changing semantics for sfde_func_start_address, following
> relevant examples may help understand the impact:
>   - sframe_find_fre () finds a the SFrame stack trace data (SFrame FRE)
>     given a lookup offset (offset of lookup_pc from the start of SFrame
>     section).  Now that the sfde_func_start_address includes the
>     distance from the sfde_func_start_address field to the start of
>     SFrame section itself, the comparison checks of
>     sfde_func_start_address with the incoming lookup offset need
>     adjustment.
>   - Some internal functions (sframe_get_funcdesc_with_addr_internal ()
>     finds SFrame FDE by using binary seach comparing
>     sfde_func_start_address fields, etc.) need adjustments.
>   - sframe_encoder_write () sorts the SFrame FDEs before writing out
>     the SFrame data.  Sorting of SFrame FDE via the internal function
>     sframe_sort_funcdesc() needs adjustments: the new encoding of
>     sfde_func_start_address means the distances are not from the same
>     anchor, so cannot be sorted directly.
> 
> This patch takes the approach of adding a new internal function:
>   - sframe_decoder_get_secrel_func_start_addr (): This function returns
>     the offset of the start PC of the function from the start of SFrame
>     section, i.e., it gives a section-relative offset.
> 
> As the sframe_decoder_get_secrel_func_start_addr () API needs the value
> of the function index in the FDE list, another internal API needs
> sframe_fre_check_range_p () adjustments too.
> 
> Sorting the FDEs (via sframe_sort_funcdesc ()) is done by first bringing
> all offsets in sfde_func_start_address relative to start of SFrame
> section, followed by sorting, and then readjusting the offsets accroding
> to the new position in the FDE list.
> 
> TBD:
>  - Version bump libsframe.  The change in encoding of
>    sfde_func_start_address means the APIs sframe_encoder_add_funcdesc ()
>    and sframe_find_fre () etc. are now backwards incompatible with previous
>    releases.

It's not just a version bump of the library. You also need to move the
affected public functions to a new version (2.0?) in the .ver file, I
think.

>  If this change is backported, we need to reserve a version
>    bump for a backport too I think.

That can simply be 1.1? I'd be rather hesitant with backporting, though.

> --- 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,12 @@ _bfd_elf_merge_section_sframe (bfd *abfd,
>        if (!sfd_ctx_abi_arch)
>  	return false;
>  
> +      /* Reset SFRAME_F_FDE_SORTED for the encoder context.  This will be set
> +	 later when FDEs are finally sorted before emission in the output
> +	 section.  */
> +      uint8_t tflags = dctx_flags & ~SFRAME_F_FDE_SORTED;
>        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,

I'm likely confused here, too: Why would the one flag need special casing?
What about flags which are set, but which aren't defined right now?

> @@ -410,7 +418,16 @@ _bfd_elf_merge_section_sframe (bfd *abfd,
>  	  " .sframe generation"));
>        return false;
>      }
> -
> +  /* Check that all SFrame sections being linked have harmonious flags.  */
> +  ectx_flags = sframe_encoder_get_flags (sfe_ctx);
> +  if ((dctx_flags & SFRAME_F_FDE_FUNC_START_ADDR_PCREL)
> +      != (ectx_flags & SFRAME_F_FDE_FUNC_START_ADDR_PCREL))

And why is the one flag special?

Such a check can, btw, be written in a less verbose / redundant manner:

  if ((dctx_flags ^ ectx_flags) & ~SFRAME_F_FDE_FUNC_START_ADDR_PCREL)

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

If the PC-relative encoding is now the only valid way of encoding things,
why would every caller need to specify the flag?

Jan