[RFC,SCHEME_B 4/7] ld: bfd: sframe: fix incorrect r_offset in RELA entries

[Jan Beulich]

On 14.04.2025 20:35, Indu Bhagat wrote:
> On 4/7/25 8:02 AM, Jan Beulich wrote:
>> On 07.04.2025 02:25, Indu Bhagat wrote:
>>> --- a/bfd/elflink.c
>>> +++ b/bfd/elflink.c
>>> @@ -11974,7 +11974,11 @@ elf_link_input_bfd (struct elf_final_link_info *flinfo, bfd *input_bfd)
>>>   		      continue;
>>>   		    }
>>>   
>>> -		  irela->r_offset += o->output_offset;
>>> +		  /* All SFrame FDEs from input sections are clubbed together
>>> +		     in the beginning of the output section.  RELA offsets are
>>> +		     already up to date.  PR ld/32666.  */
>>> +		  if (o->sec_info_type != SEC_INFO_TYPE_SFRAME)
>>> +		    irela->r_offset += o->output_offset;
>>
>> I'm irritated by there still being such special casing here. You go to some
>> lengths in the description to explain this, but I fear I'm still not groking
>> it even after reading multiple times. In particular the absence of any special
>> casing here for SHF_MERGE sections makes we wonder why your content re-ordering
>> is requiring such.
>>
> 
> A re-worded answer, in case it helps:
> 
> In SFrame, all FDEs (fixed length structure) follow the SFrame Header. The
> correct offset (of the first field of the FDE) has been manually 
> calculated via
> _bfd_elf_sframe_section_offset () (i.e., it calculates the offset of the 
> FDE in
> the final output SFrame section).
> 
> BTW, the "o" variable above holds the input section; that often is a 
> source of
> confusion for me as well.
> 
> Regarding SHF_MERGE, I am not sure where the r_offset is updated once
> elimination due to duplication is identified etc.  But as with other
> (non-SFrame) sections, no special casing for SHF_MERGE is needed because it
> works in a two step-process:
>    - update the r_offset to the updated value within the section
>    - adjust the r_offset with the output_offset of the section
>      (o->output_offset).
> Its only the SFrame section data layout that is not aligned to the two-step
> process.

And it can't be made match that other scheme? E.g. as outlined ...

>> To at least hide it from here, wouldn't it be possible to have
>> _bfd_elf_sframe_section_offset() do the "inverse" adjustment, for that to be
>> "undone" here? (For this it may help to remove the early return from the
>> function - the sole caller ensures it's the right type.)

... here?

> It is possible to hide it in _bfd_elf_sframe_section_offset().  I purposely
> chose to leave this in elflink.c (elf_link_input_bfd ()), because I 
> thought it
> is less bug-prone and easier to follow.  I can always leave a clear 
> comment in
> _bfd_elf_sframe_section_offset (); I think the current testcases already 
> should
> suffice to cover this code path.

Let me Cc Nick and Alan - maybe they have a specific view on this either
way, with their much longer experience.

Jan