[PATCH 1/2] libsframe: add new dump_sframe_reloc
Indu Bhagat
indu.bhagat@oracle.com
Sat Mar 8 08:11:20 GMT 2025
On 2/26/25 3:54 PM, Indu Bhagat wrote:
> On 2/25/25 11:14 PM, Jan Beulich wrote:
>> On 26.02.2025 00:10, Indu Bhagat wrote:
>>> On 2/24/25 2:37 AM, Jan Beulich wrote:
>>>> On 21.02.2025 15:48, Indu Bhagat wrote:
>>>>> On 2/21/25 12:54 AM, Jan Beulich wrote:
>>>>>> On 21.02.2025 01:43, Indu Bhagat wrote:
>>>>>>> On 2/19/25 1:38 AM, Jan Beulich wrote:
>>>>>>>> On 17.02.2025 17:58, Indu Bhagat wrote:
>>>>>>>>> --- a/libsframe/sframe.c
>>>>>>>>> +++ b/libsframe/sframe.c
>>>>>>>>> @@ -102,6 +102,37 @@ sframe_ret_set_errno (int *errp, int error)
>>>>>>>>> return NULL;
>>>>>>>>> }
>>>>>>>>> +/* If the input buffer containing the SFrame section has been
>>>>>>>>> relocated, there
>>>>>>>>> + will be a need to do fixups too. The fixup merely accounts
>>>>>>>>> for the offset
>>>>>>>>> + of the byte from the start of the section.
>>>>>>>>> +
>>>>>>>>> + Currently used by dump_sframe_reloc. The caller must have
>>>>>>>>> decoded (and
>>>>>>>>> + hence, endian flipped) the input buffer before calling this
>>>>>>>>> function. */
>>>>>>>>> +
>>>>>>>>> +int
>>>>>>>>> +sframe_fde_tbl_reloc_fixup (sframe_decoder_ctx *dctx)
>>>>>>>>> +{
>>>>>>>>> + uint8_t sframe_ver = sframe_decoder_get_version (dctx);
>>>>>>>>> + uint32_t num_fdes = sframe_decoder_get_num_fidx (dctx);
>>>>>>>>> + unsigned int buf_offset = 0;
>>>>>>>>> + sframe_func_desc_entry *fde;
>>>>>>>>> + uint32_t i = 0;
>>>>>>>>> +
>>>>>>>>> + if (sframe_ver != SFRAME_VERSION_2 || !dctx->sfd_funcdesc)
>>>>>>>>> + return SFRAME_ERR;
>>>>>>>>> +
>>>>>>>>> + buf_offset += sframe_decoder_get_hdr_size (dctx);
>>>>>>>>> + while (i < num_fdes)
>>>>>>>>> + {
>>>>>>>>> + fde = &dctx->sfd_funcdesc[i];
>>>>>>>>> + fde->sfde_func_start_address += buf_offset;
>>>>>>>>> + buf_offset += sizeof (sframe_func_desc_entry);
>>>>>>>>> + i++;
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>>> + return 0;
>>>>>>>>> +}
>>>>>>>>
>>>>>>>> In the bug report, comment 1, you specifically provided
>>>>>>>>
>>>>>>>> $ readelf -r file.o
>>>>>>>> Relocation section '.rela.sframe' at offset 0x8728 contains 3
>>>>>>>> entries:
>>>>>>>> Offset Info Type Sym.
>>>>>>>> Value Sym. Name + Addend
>>>>>>>> 00000000001c 000200000002 R_X86_64_PC32
>>>>>>>> 0000000000000000 .text + 0
>>>>>>>> 000000000030 000200000002 R_X86_64_PC32
>>>>>>>> 0000000000000000 .text + 30
>>>>>>>> 000000000044 000200000002 R_X86_64_PC32
>>>>>>>> 0000000000000000 .text + 40
>>>>>>>>
>>>>>>>> Having peeked also at patch 2, I fail to see where these
>>>>>>>> relocations
>>>>>>>> are actually applied. The function above appears to be making
>>>>>>>> assumptions
>>>>>>>> about where relocations need applying, and why type they are. Am I
>>>>>>>> overlooking anything?
>>>>>>>>
>>>>>>>
>>>>>>> The reloc machinery is invoked in the load_specific_debug_section ()
>>>>>>> functions.
>>>>>>
>>>>>> Yet then why is further fixing up necessary? And how to you know
>>>>>> how to
>>>>>> do the fixups, when you don't further look at the relocations
>>>>>> coming from
>>>>>> the object.
>>>>>>
>>>>>
>>>>> IIUC, load_specific_debug_section () loads the debug section at
>>>>> address
>>>>> 0. For PC-relative offsets, where the .text start address is also 0,
>>>>> load_specific_debug_section () calculates the S + A - P correctly.
>>>>>
>>>>> In SFrame FDE start address, we would like to indicate the function
>>>>> start address (hence, the PC-relative relocation). In that
>>>>> respect, the
>>>>> function does assume that there is PC-relative relocation at the start
>>>>> of every FDE (for function start address); something I think is OK
>>>>> to do
>>>>> as this is baked into the format.
>>>>
>>>> Quoting from
>>>> https://sourceware.org/binutils/docs/sframe-spec.html#SFrame-
>>>> Function-Descriptor-Entries:
>>>>
>>>> "0x00 int32_t sfde_func_start_address Signed 32-bit
>>>> integral field
>>>> denoting the virtual memory address of the described function."
>>>>
>>>> The field being signed and only 32 bits may be a hidden hint at it
>>>> being
>>>> a PC-relative value, but nothing like this is being said.
>>>>
>>>> But yes, if that's a part of the spec, then ...
>>>>
>>>>>>> Not sure if it is not clear from code comments: Such reloc fixup for
>>>>>>> pc-relative relocations is necessary only for the usecase of textual
>>>>>>> dump of the debug sections containing relocations.
>>>>>>
>>>>>> It states this, yes, but without saying why this would be needed
>>>>>> in the
>>>>>> first place. Are you suggesting that the amount of relocation
>>>>>> processing
>>>>>> invoked from load_specific_debug_section() is insufficient? If so,
>>>>>> why
>>>>>> would this affect sframe only?
>>>>>>
>>>>>
>>>>> From looking at the involved code in bfd/, binutils/dwarf.c, my
>>>>> understanding was that the fixup is necessary if the debug section is
>>>>> also loaded at address 0. Other DWARF sections also need similar
>>>>> fixups. E.g., in get_encoded_value () in binutils/dwarf.c :
>>>>>
>>>>> if ((encoding & 0x70) == DW_EH_PE_pcrel)
>>>>> val += section->address + (data - section->start);
>>>>
>>>> ... something similar to this is certainly wanted here. The further
>>>> trouble I'd then be in is that what you have quoted above is relatively
>>>> obvious to perform exactly that. Whereas
>>>>
>>>> buf_offset += sframe_decoder_get_hdr_size (dctx);
>>>> while (i < num_fdes)
>>>> {
>>>> fde = &dctx->sfd_funcdesc[i];
>>>> fde->sfde_func_start_address += buf_offset;
>>>> buf_offset += sizeof (sframe_func_desc_entry);
>>>> i++;
>>>> }
>>>>
>>>> doesn't make this quite as clear, to me at least. Which, at least in
>>>> part,
>>>> may be due to there being an apparent assumption that
>>>> sfde_func_start_address is the first field of the struct. That is true,
>>>> and it's not going to change, yet I consider it bad practice to use
>>>> such
>>>> implicitly, rather than making it explicit (and rely on the compiler to
>>>> eliminate the constant 0 that will result).
>>>
>>> I am not sure I understand what is the ask here. The code assumes that
>>> there is a member sfde_func_start_address, and that there are num_fdes
>>> entries of type typeof(sfd_funcdesc[0]). It also assumes that
>>> sfde_func_start_address is the relocated field.
>>
>> My understanding is that in the code above buf_offset would need to
>> further have offsetof(sframe_func_desc_entry, sfde_func_start_address)
>> added in, to account for sfde_func_start_address not being the first
>> field of the struct. That could be done explicitly, or it could be
>> done by writing this code in a way closer to what get_encoded_value()
>> does (where clearly a "delta" is being applied).
>>
>>>> Along these lines, may I also ask that the sizeof() there become
>>>> sizeof (*fde), to allow easily making that connection as well?
>>>
>>> At some point there will be an SFrame V3. Generally speaking, the way I
>>> am thinking about it is that libsframe will have the ability to decode
>>> both SFrame V2 and SFrame V3, but the in-memory representation will
>>> always be SFrame V3.
>>>
>>> So when we will try to dump an SFrame V2 section with relas (when say
>>> SFrame V3 will be the current version), libsframe will have an SFrame V3
>>> section in the sframe_decoder_ctx object at this point. The
>>> relocations, however, were on an SFrame V2 section contents. Hence, the
>>> fixups will need to do a sizeof (sframe_func_desc_entry_v2) or sizeof
>>> (sframe_func_desc_entry_v3) depending on the version. This is why I
>>> chose to write a sizeof (sframe_func_desc_entry).
>>>
>>> Perhaps what I say above is a moot point currently as SFrame V2 is all
>>> there is, but the code will eventually evolve to something that will use
>>> sizeof (sframe_func_desc_entry_v2) or sizeof
>>> (sframe_func_desc_entry_v3)...
>>
>> Hmm, I see. I consider this unfortunate, though. May I ask that you still
>> you sizeof (*fde) for the time being, and we check our options at the
>> point where v3 will become a thing?
>>
>
> OK.
>
> I now have:
>
> /* Although not the case currently, but it is safer to account for
> sfde_func_start_address not being the first field of the on-disk
> representation of the packed struct sframe_func_desc_entry. */
> buf_offset += offsetof (sframe_func_desc_entry,
> sfde_func_start_address);
> while (i < num_fdes)
> {
> fde = &dctx->sfd_funcdesc[i];
> fde->sfde_func_start_address += buf_offset;
> buf_offset += sizeof (*fde);
> i++;
> }
>
>
I (and simultaneously Jens) later realized that we have a thinko in
GAS/ld.bfd that should be fixed, after which this [Patch 1/2] will not
be necessary.
[Patch 2/2] in this series is included in the new series "[RFC 0/4] Fix
relocatable links with SFrame section"
Thanks
More information about the Binutils
mailing list