[PATCH 1/2] libsframe: add new dump_sframe_reloc
Jan Beulich
jbeulich@suse.com
Wed Feb 26 07:14:10 GMT 2025
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?
Jan
More information about the Binutils
mailing list