[PATCH 1/2] libsframe: add new dump_sframe_reloc
Indu Bhagat
indu.bhagat@oracle.com
Wed Feb 26 23:54:44 GMT 2025
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++;
}
More information about the Binutils
mailing list