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

[Indu Bhagat]

On 6/16/25 12:23 AM, Jan Beulich wrote:
> 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.
> 

Yes. Its unpleasant and unfortunate.

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

(Aplogies I mixed it up in the other paragraph, but here is why we 
special case SFRAME_F_FDE_SORTED...)

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.

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

We have a check in sframe_header_sanity_check_p () in libsframe/sframe.c 
where we check (hp->sfh_preamble.sfp_flags | all_flags) != all_flags), 
where all_flags was defined as (SFRAME_F_FDE_SORTED | 
SFRAME_F_FRAME_POINTER | SFRAME_F_FDE_FUNC_START_ADDR_PCREL).

The sanity check is done for every sframe_decode () (used by ld and the 
dumping code).  So older binutils are effectively rejecting objects with 
new flags set.

The errors ATM by ld at least will be a bit of catch-all message though. 
  Something like:
   ld: error in object.o(.sframe); no .sframe will be created

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

Hmm. Thats a good idea.

I can define two sets in include/sframe-api.h.  That way we tie the 
implementation in libsframe with these expectations, without having to 
state this in the specification.

Something like this in include/sframe-api.h:

/* Set of all defined flags known to this implementation.  Any flag 
other than
    these, if set, is deemed invalid.  This is used for sanity checking to
    ensure that the implementation only admits sections with flags 
defined in
    the specific version.  */
#define SFRAME_F_ALL_FLAGS \
   (SFRAME_F_FDE_SORTED | SFRAME_F_FRAME_POINTER \
    | SFRAME_F_FDE_FUNC_START_ADDR_PCREL)

/* Set of flags that are required to be harmonious between all decoder and
    encoder objects participating in a link.  */
#define SFRAME_F_LD_MUSTHAVE_FLAGS \
   (SFRAME_F_FDE_FUNC_START_ADDR_PCREL)


I will then use SFRAME_F_ALL_FLAGS in libsframe/sframe.c 
sframe_header_sanity_check_p () instead of the current all_flags. So 
something like:

-  unsigned char all_flags = (SFRAME_F_FDE_SORTED | SFRAME_F_FRAME_POINTER
-                            | SFRAME_F_FDE_FUNC_START_ADDR_PCREL);
    /* Check preamble is valid.  */
    if (hp->sfh_preamble.sfp_magic != SFRAME_MAGIC
        || (hp->sfh_preamble.sfp_version != SFRAME_VERSION_1
           && hp->sfh_preamble.sfp_version != SFRAME_VERSION_2)
-      || (hp->sfh_preamble.sfp_flags | all_flags) != all_flags)
+      || hp->sfh_preamble.sfp_flags & ~SFRAME_F_ALL_FLAGS)


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

Ah, OK. I have added a check in sframe_encode () now:

   /* Implementation in the SFrame encoder APIs, e.g.,
      sframe_encoder_write_sframe assume flag 
SFRAME_F_FDE_FUNC_START_ADDR_PCREL
      set.  */
   if (!(flags & SFRAME_F_FDE_FUNC_START_ADDR_PCREL))
    return sframe_ret_set_errno (errp, SFRAME_ERR_ECTX_INVAL);