[PATCH,V4 6/9] bfd: gas: ld: libsframe: adopt new encoding for FDE func start addr field

[Jan Beulich]

On 30.06.2025 09:27, Indu Bhagat wrote:
> On 6/27/25 12:45 AM, Jan Beulich wrote:
>> On 19.06.2025 08:24, Indu Bhagat wrote:
>>> +  ectx_flags = sframe_encoder_get_flags (sfe_ctx);
>>> +  if ((dctx_flags ^ ectx_flags) & SFRAME_F_LD_MUSTHAVE_FLAGS)
>>
>> For this kind of check, then name of the new constant is misleading. The
>> "MUSTHAVE" in there suggests every input (or every caller) has to set all
>> of those flags. Question is whether you don't really mean here what the
>> name says:
>>
>>    if (!(dctx_flags & ectx_flags & SFRAME_F_LD_MUSTHAVE_FLAGS))
>>
>> ? Of course ...
>>
>>> +    {
>>> +      _bfd_error_handler
>>> +	(_("input SFrame sections with different FDE encoding prevent"
>>> +	  " .sframe generation"));
>>
>> ... the wording here suggests the ^ you have is meant. IOW its pretty
>> confusing right now. It's further odd (to me at least) that both decoder
>> and encoder flags would be related to the input section: Isn't the
>> encoder dealing with the output?
>>
> 
> (Yes, encoder is dealing with the output.)
> 
> Hmm.  I want to check that all input sections have the set of "data 
> encoding" related flags.  At this time, SFRAME_F_FDE_FUNC_START_PCREL is 
> the only flag in this category.  This is to say that the linker at this 
> time will not update the encoding of data on the fly, and hence the set 
> of "data encoding" related flags must match with the input decoder objects.
> 
> Given that we initialize ectx_flags with the first decoder context flags
>         uint8_t tflags = dctx_flags & ~SFRAME_F_FDE_SORTED;
>         tflags |= SFRAME_F_FDE_FUNC_START_PCREL;
>         sframe_encode (..., tflags, ...)
> 
> Perhaps its better to not create a new SFRAME_F_LD_MUSTHAVE_FLAGS and 
> continue to use SFRAME_F_FDE_FUNC_START_PCREL for now, until the point 
> another flag shows similar property, and it helps coming up with an 
> appropriate name ?  I can think of SFRAME_F_LD_REQUIRED_FLAGS, but not 
> sure if this helps with the confusion.

Well, REQUIRED isn't any better when it comes to the check above. What
that check does is make sure the flags in input (decoder) and output
(encoder) are the same. It does not check that the flags are all set
everywhere, which is what "must have" or "required" mean to me. To
answer your question - I think such a constant is still wanted right
away. Just that it needs to be named and used in a consistent way.

>>> @@ -1691,15 +1723,23 @@ sframe_encoder_add_funcdesc_v2 (sframe_encoder_ctx *encoder,
>>>   static int
>>>   sframe_sort_funcdesc (sframe_encoder_ctx *encoder)
>>>   {
>>> -  sframe_header *ehp;
>>> +  sframe_header *ehp = sframe_encoder_get_header (encoder);
>>>   
>>> -  ehp = sframe_encoder_get_header (encoder);
>>>     /* Sort and write out the FDE table.  */
>>>     sf_fde_tbl *fd_info = encoder->sfe_funcdesc;
>>>     if (fd_info)
>>>       {
>>> +      for (unsigned int i = 0; i < fd_info->count; i++)
>>> +	fd_info->entry[i].sfde_func_start_address
>>> +	  += sframe_encoder_get_offsetof_fde_start_addr (encoder, i, NULL);
>>> +
>>>         qsort (fd_info->entry, fd_info->count,
>>>   	     sizeof (sframe_func_desc_entry), fde_func);
>>> +
>>> +      for (unsigned int i = 0; i < fd_info->count; i++)
>>> +	fd_info->entry[i].sfde_func_start_address
>>> +	  -= sframe_encoder_get_offsetof_fde_start_addr (encoder, i, NULL);
>>
>> Especially when there are many entries but they are nearly sorted already,
>> isn't this adding undue overhead? The problem being that, as it looks, in
>> fde_func() you have no way to get hold of "encoder". Question nevertheless
>> is whether there isn't another way of doing the address adjustment as
>> needed in fde_func() rather than globally (and twice) here.
>>
> 
> I think qsort_r may be helpful as it allows an additional argument (Then 
> we can use keep section->vma in the encoder object and we can do a 
> &fde->func_start_addr - section->vma to calculate the offset for an 
> FDE).  BTW, libctf uses qsort_r, and over time has addressed portability 
> concerns with it (few commits I saw were 
> 6b22174ff163b828a2069f22353018ca97fc0fb4 and 
> a0486bac41d6ce47f27795a5abbca5cc53ddba00, there may be more).
> 
> My first instinct was to not bring the additional implementation in 
> libsframe too, and have it addressed like above...

You're the maintainer, so you get to judge. I was merely pointing out a
possible issue; it may well be clear with the wider context you have that
this isn't going to be a big deal in practice.

Jan