[RFC 4/4] ld: bfd: sframe: fix incorrect r_addend in RELA entries

[Indu Bhagat]

On 3/10/25 6:16 AM, Jens Remus wrote:
> On 08.03.2025 08:38, Indu Bhagat wrote:
>> With the fix in GAS, we now use a different PC-relative RELA for
>> updating the SFrame FDE function start address: The value is the offset
>> of the start PC of the function from the start of the SFrame section.
>>
>> When RELAs are output (e.g. for relocatable links), there is need to
>> adjust the r_addend.  This is because the r_addend values still have the
>> correct values for the _input_ SFrame section being linked / relocated.
>> The values must now be (before outputing the RELAs) with respect to the
>> _output_ SFrame section.
>>
>> PS: This patch should be merged with the previous commits before final
>> commit (Otherwise the tests will fail).  It is currently a separate
>> patch as I would like to check if this is OK to do.  If such a "addend
>> fixup" is risky or wrong, it seems we will need a new type of RELOC for
>> SFrame sections.
> 
>> diff --git a/bfd/elf-sframe.c b/bfd/elf-sframe.c
> 
>> @@ -574,6 +574,37 @@ _bfd_elf_sframe_section_offset (bfd *output_bfd 
>> ATTRIBUTE_UNUSED,
>>         + out_fde_idx * sizeof (sframe_func_desc_entry));
>>   }
>> +/* Get the "canonicalized" addend for the symbol reference 
>> corresponding to the
>> +   relocation at RELOC_INDEX.  E.g.,  for the following reloc for the 
>> SFrame
> 
> Nit: s/reloc/relocs/
> 

Done.

>> +   FDE function start address:
>> +    Offset          Type          Sym. Name + Addend
>> +    00000000001c  R_X86_64_PC32   .text + 1c
>> +    000000000030  R_X86_64_PC32   .text + 3b
>> +   The canonicalized addend are 0 and b respectively as the relocs 
>> are for
>> +   symbols (.text + 0) and (.text + b) respectively.
> 
> Maybe it would help to use the function symbols foo and bar in the example:
> 
>     relocation at RELOC_INDEX.  E.g.,  for the following annotated 
> relocs for the SFrame
>     FDE function start address of FDE[0] and FDE[1] for functions foo 
> and bar:
>      Offset          Type          Sym. Name + Addend
>      00000000001c  R_X86_64_PC32   .text + 1c   // FDE[0] for foo 
> at .text + 0
>      000000000030  R_X86_64_PC32   .text + 3b   // FDE[1] for bar 
> at .text + b
>     With:
>       1c = 0 + sizeof(sframe_header) + 0 * sizeof(sframe_fde)
>       3b = b + sizeof(sframe_header) + 1 * sizeof(sframe_fde)
>     The canonicalized addend are 0 and b respectively as the relocs are for
>     symbols (.text + 0) and (.text + b) respectively.
> 

Amended.

>> +
>> +   This is used to manually adjust the RELA addends to ensure correct 
>> values
>> +   for relocatable links.  */
>> +
>> +bfd_vma
>> +_bfd_elf_sframe_section_addend (bfd *output_bfd ATTRIBUTE_UNUSED,
>> +                  struct bfd_link_info *info ATTRIBUTE_UNUSED,
>> +                  asection *sec,
>> +                  unsigned int reloc_index,
>> +                  bfd_vma addend)
>> +{
>> +  struct sframe_dec_info *sfd_info;
>> +
>> +  if (sec->sec_info_type != SEC_INFO_TYPE_SFRAME)
>> +    return addend;
>> +
>> +  sfd_info = (struct sframe_dec_info *) elf_section_data (sec)- 
>> >sec_info;
>> +  BFD_ASSERT (sfd_info && sfd_info->sfd_ctx);
>> +
>> +  return (addend - (sframe_decoder_get_hdr_size (sfd_info->sfd_ctx)
>> +            + reloc_index * sizeof (sframe_func_desc_entry)));
> 
> IIUC this is only valid as long as reloc_index == fde_index.
> 

Yes, and reloc_index should be the fde_index for input sections.  Do you 
see any issue ?  I can add a comment around the same.

>> +}
>> +
>>   /* Write out the .sframe section.  This must be called after
>>      _bfd_elf_merge_section_sframe has been called on all input
>>      .sframe sections.  */
>> diff --git a/bfd/elflink.c b/bfd/elflink.c
>> index 895fbb0206e..6bce2966359 100644
>> --- a/bfd/elflink.c
>> +++ b/bfd/elflink.c
>> @@ -11939,7 +11939,9 @@ elf_link_input_bfd (struct elf_final_link_info 
>> *flinfo, bfd *input_bfd)
>>             last_offset = o->output_offset;
>>             if (!bfd_link_relocatable (flinfo->info))
>>           last_offset += o->output_section->vma;
>> -          for (next_erel = 0; irela < irelaend; irela++, next_erel++)
>> +          unsigned int num_reloc = 0;
>> +          for (next_erel = 0; irela < irelaend;
>> +           irela++, next_erel++, num_reloc++)
> 
> The introduction of num_reloc could be omitted.  See below.
> 

Done.

And thanks for reviewing.

>>           {
>>             unsigned long r_symndx;
>>             asection *sec;
>> @@ -12070,10 +12072,24 @@ elf_link_input_bfd (struct 
>> elf_final_link_info *flinfo, bfd *input_bfd)
>>                   }
>>               }
>> -              /* Adjust the addend according to where the
>> -             section winds up in the output section.  */
>>                 if (rela_normal)
>> -            irela->r_addend += sec->output_offset;
>> +            {
>> +              if (o->sec_info_type == SEC_INFO_TYPE_SFRAME)
>> +                {
> 
>                    unsigned int num_reloc = irela - internal_relocs;
> 
>> +                  bfd_vma addend
>> +                   = _bfd_elf_sframe_section_addend (output_bfd,
>> +                                   flinfo->info, o,
>> +                                   num_reloc,
>> +                                   irela->r_addend);
>> +                  /* Adjust the addend in the output RELA.  The
>> +                 input SFrame section has already been
>> +                 relocated.  */
>> +                  irela->r_addend = addend + irela->r_offset;
>> +                }
>> +              /* Adjust the addend according to where the
>> +                 section winds up in the output section.  */
>> +              irela->r_addend += sec->output_offset;
>> +            }
>>               }
>>             else
>>               {
> 
> Regards,
> Jens