[PATCH 1/2] libsframe: add new dump_sframe_reloc

[Jan Beulich]

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

Along these lines, may I also ask that the sizeof() there become
sizeof (*fde), to allow easily making that connection as well?

Jan