[PATCH,V4 6/9] bfd: gas: ld: libsframe: adopt new encoding for FDE func start addr field
Indu Bhagat
indu.bhagat@oracle.com
Mon Jun 30 07:27:53 GMT 2025
On 6/27/25 12:45 AM, Jan Beulich wrote:
> 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.)
>
Thanks. Fixed.
>> + 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?
>
(Yes, encoder is dealing with the output.)
Hmm. I want to check that all input sections have the set of "data
encoding" related flags. At this time, SFRAME_F_FDE_FUNC_START_PCREL is
the only flag in this category. This is to say that the linker at this
time will not update the encoding of data on the fly, and hence the set
of "data encoding" related flags must match with the input decoder objects.
Given that we initialize ectx_flags with the first decoder context flags
uint8_t tflags = dctx_flags & ~SFRAME_F_FDE_SORTED;
tflags |= SFRAME_F_FDE_FUNC_START_PCREL;
sframe_encode (..., tflags, ...)
Perhaps its better to not create a new SFRAME_F_LD_MUSTHAVE_FLAGS and
continue to use SFRAME_F_FDE_FUNC_START_PCREL for now, until the point
another flag shows similar property, and it helps coming up with an
appropriate name ? I can think of SFRAME_F_LD_REQUIRED_FLAGS, but not
sure if this helps with the confusion.
>> @@ -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.
>
OK. done.
>> --- 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?
>
For AArch64, SFrame for plt* is not being generated still. This is
tracked in the SFrame wiki.
>> --- 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 ..."
>
OK. Now I have:
/* The function descriptor entries as dumped by the assembler are not
sorted on PCs. Fix for PR ld/32666 requires setting of an additional
flag in SFrame Version 2. */
unsigned char sframe_flags = SFRAME_F_FDE_FUNC_START_PCREL;
>> --- 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.
>
Fixed.
>> @@ -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.
>
I think qsort_r may be helpful as it allows an additional argument (Then
we can use keep section->vma in the encoder object and we can do a
&fde->func_start_addr - section->vma to calculate the offset for an
FDE). BTW, libctf uses qsort_r, and over time has addressed portability
concerns with it (few commits I saw were
6b22174ff163b828a2069f22353018ca97fc0fb4 and
a0486bac41d6ce47f27795a5abbca5cc53ddba00, there may be more).
My first instinct was to not bring the additional implementation in
libsframe too, and have it addressed like above...
>> --- 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.
>
Yeah. I need to work on this testcase to remove the magic numbers. I
will work on it as a separate commit (note sure when I will get to it,
had already noted it down to take a look at libsframe.encode and
libsframe.decode tests. With recent patches we removed magic numbers
from lisbframe.find tests only so far).
Thanks for reviewing
More information about the Binutils
mailing list