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

[Jan Beulich]

On 16.06.2025 08:43, Indu Bhagat wrote:
> On 6/13/25 5:39 AM, Jan Beulich wrote:
>> 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? 
> 
> Short answer:
> Because SFrame relocatable links were broken.  To fix them using 
> scheme#B approach, we needed to fix the sfde_func_start_address in the 
> ET_REL binary emitted by the linker.  To convey a bugfix, we now emit 
> the flag SFRAME_F_FDE_FUNC_START_ADDR_PCREL.
> 
> As gas also emits ET_REL binary (with sfde_func_start_address as offset 
> from the field itself), so we mark with 
> SFRAME_F_FDE_FUNC_START_ADDR_PCREL.  Its basically "the right thing to 
> do" for consistency (all ET_REL binaries have the same encoding and 
> hence, the flag set) moving forward.
> 
> Longer version:
> On trunk, there are two sets of encodings (and buggy SFrame relocatable 
> links):
>    - (#1) ET_REL generated by gas had sfde_func_start_address as offset 
> from the field itself.
>    - (#2) ET_DYN, ET_EXEC, and ET_REL (relocatable links) generated by 
> ld had sfde_func_start_address as offset from start of section.
> 
> In the first attempt 
> (https://sourceware.org/pipermail/binutils/2025-March/139853.html), I 
> tried to bring  (#1) to also emit sfde_func_start_address as offset from 
> start of section.  I informally refer to this one as Scheme#A patchset, 
> but the review feedback was to avoid abusing the available relocations 
> (https://sourceware.org/pipermail/binutils/2025-March/139980.html).
> 
> So now with this current patchset (informally called Scheme#B), we:
>    - (#1) ET_REL generated by gas continues to have 
> sfde_func_start_address as offset from the field itself.
>    - (#2) ET_DYN, ET_EXEC, and ET_REL (relocatable links) generated by 
> ld have sfde_func_start_address as offset from field itself.
> 
>> And how would one distinguish "new with flag", "new without
>> flag", and "old without flag"?
> 
> For those generated by gas, there is no way to distinguish between the 
> "new with flag" or "old without flag".  The emitted data has always been 
> the same, thats all that can be said and relied on.

IOW objects generated with binutils not including this series simply can't
be used going forward.

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

You didn't address this question.

>> What about flags which are set, but which aren't defined right now?
> 
> Flags which arent defined are not set.  The rest of the bits (bits at 
> position [4:8] of the flags field should be zero.  Perhaps I should add 
> a line in the spec document that undefined bits in the flags field are 
> expected to be zero.

And then, if another flag gets defined later, how are older binutils
supposed to be dealing with such an object? Imo they simply ought to
refuse handling it.

> This code will need to evolve as more flags are added.  Currently the 
> three flags defined are such that only SFRAME_F_FDE_SORTED needs reset 
> at the time of linking, because this property in broken while the 
> sections are being linked in memory (as the SFrame FDEs are being placed 
> in the output section) until the eventual sort at write time.
> 
> The code does not deal with SFRAME_F_FRAME_POINTER currently as this 
> flag is not set by any producer ATM.  This flag indicates that the 
> functions in the section also preserve frame pointer.
> 
> That said, to be more precise, the creation of the encoder object for 
> the output SFrame section above should explicitly set 
> SFRAME_F_FDE_FUNC_START_ADDR_PCREL now (not being done above).  This is 
> because the data created by the linker is in that encoding now.  I will 
> add this flag here.
> 
>>> @@ -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?
> 
> Of the other flags, SFRAME_F_FRAME_POINTER should have similar handling, 
> but no producer sets it currently.  So it is skipped from this check ATM...

Odd.

> If more flags are added, this may need adjustments later.

Imo it would be very good if checking like this wouldn't need adjustment for
every new flag added. Should you then perhaps at least have a #define
expanding to the OR of all flags which need to be consistent between encoder
and decoder? That #define and the flag #define-s themselves could live next
to each other, thus requiring modification in a central place rather than in
two disconnected locations?

>>> --- 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?
> 
> Perhaps its a symptom of (my) biased perspective from the starting point 
> of this implementation, but now that you question, I still do find the 
> flag specified like so in the caller to be clearer and less error prone.

Well, yes, it may indeed be fine for doc purposes. But then perhaps check
in the function that all callers pass the flag set?

Jan