[PATCH 1/2] libsframe: add new dump_sframe_reloc

[Indu Bhagat]

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.

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

Thanks