[RFC 1/4] gas: ld: sframe: fix RELA r_offset handling

[Indu Bhagat]

On 3/21/25 7:13 AM, Jan Beulich wrote:
> On 20.03.2025 16:54, Indu Bhagat wrote:
>> On 3/13/25 7:04 AM, Jan Beulich wrote:
>>> On 08.03.2025 08:38, Indu Bhagat wrote:
>>>> In SFrame V2, SFrame function descriptor entry is a 32-bit signed
>>>> integer (sfde_func_start_address) which is meant to indicate the start
>>>> PC of the function.  According to the specification, it intends to hold
>>>> the offset of the start PC of the function from the
>>>> _start_of_the_SFrame_section_.
>>>
>>> Which isn't something that can normally be expressed by relocations. Hence
>>> is it maybe the spec which is flawed? As mentioned elsewhere, relocations
>>> should retain their normal meaning. Custom adjustments based on it being
>>> .sframe sections that are being processed feels wrong. Unaware tools ought
>>> to be able to deal with this just fine.
>>>
>>
>> Re: change the spec, this issue needs to be fixed for SFrame V2, and the
>> fix backported.
> 
> I fear I'll need to leave it to Nick then to approve any of this series,
> in line with him having approved the original sframe work.
> 

I understand your reservations.  Hopefully we converge on an acceptable 
solution to fix the relocatable links of SFrame for SFrame V2.

>> Given the way SFrame sections are by design, unaware tools linking
>> SFrame sections will not dump out valid output SFrame section.
> 
> Right - I consider this a deficiency of the spec.
> 

Deficiency of the spec: I initially thought of categorizing it as such 
and then addressing it by moving to Scheme#B in the spec (i.e., 
sfde_func_start_address is then the distance from relocation itself to 
the start PC of function).  Switching to Scheme#B will have its own 
consequences on stack tracers which needs to be evaluated...

But... on more thinking, It seems to me, that if there was an available 
PC-relative relocation, which is relative to the start of the section 
containing the relocation, and not relative to the address of the 
relocation itself,  this likely would not be considered a spec bug...

>>> There also looks to be a number of unrelated changes in this patch.
>>> Without there being anything said about those they look as if they're
>>> there by mistake.
>>
>> There are two set of changes in this patch, which need to go hand in
>> hand (i.e., in the same patch I am afraid): gas emits a different RELA
>> and ld does not do any fixup as it was doing earlier.  I cant seem to
>> find which of the stub seems unrelated.  Can you point them to me ?
> 
> Starting from the top, there's renaming of a local var pltn_reloc_by_hand,
> there's insertion of a (seemingly random) blank line, and there's removal
> of a FIXME comment. (That latter change may indeed be related, and I may
> merely not be appreciating the connection.) And then in output_sframe()
> there's removal of a blank line, for no apparent reason.
> 

Thanks.

In spirit of making it clearer:
_bfd_elf_merge_section_sframe () is invoked for merging input SFrame 
sections in memory for an eventual write later via 
_bfd_elf_write_section_sframe ().  _bfd_elf_merge_section_sframe () gets 
relocated contents.

For linker created .sframe sections (for .plt, .plt.* sections), there 
are no rela/cookies.  So the fixup needs to be done by hand.  Earlier 
the fixup was including the r_offset (thinko/bug), but now:
   - In _bfd_x86_elf_finish_dynamic_sections (): when SFrame FDE for 
.plt.* are generated, the sfde_func_start_address holds the distance 
from (sframe_start - plt entry start); abiding by the specification.
   - In _bfd_elf_merge_section_sframe (), the only fixup necessary is 
for pltN entries.

That said, I will address your review comments for next posting.

- I guess the original name of pltn_reloc_by_hand is still appropriate. 
I will just continue to use that.
- The FIXME comment has been something valuable for testing only for 
E_DYN/E_EXEC binaries, perhaps I should let that sit there too.  Doesnt 
hurt.
- The removal of blank line in output_sframe () was my bad. I saw it 
after sending the series, have reverted it.