[PATCH 1/2] libsframe: add new dump_sframe_reloc

[Indu Bhagat]

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.

> 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)...