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

[Indu Bhagat]

On 6/30/25 11:17 PM, Jan Beulich wrote:
> On 01.07.2025 06:17, Indu Bhagat wrote:
>> On 6/30/25 12:43 AM, Jan Beulich wrote:
>>> 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.
>> OK. I thought about what I intend to achieve vs what the code does.  You
>> are right.  I am thinking the following will work:
>>
>>     /* Check that all SFrame sections being linked have the 'data encoding'
>>        related flags set.  The implementation does not support updating these
>>        data encodings on the fly.  */
>>     ectx_flags = sframe_encoder_get_flags (sfe_ctx);
>>     if ((dctx_flags & ectx_flags & SFRAME_F_LD_MUSTHAVE_FLAGS)
>>         != SFRAME_F_LD_MUSTHAVE_FLAGS)
> Oh, of course - my earlier code snippet wasn't quite right.
> 
>>       {
>>         _bfd_error_handler
>>           (_("input SFrame sections with unexpected data encoding prevent"
>>             " .sframe generation"));
>>         return false;
>>       }
>>
>> I let the ectx_flags remain in the check, I think it is harmless and
>> will atleast help detect errors at dev time if ectx_flags has any
>> unintended state. I thought about doing a
>>
>>      uint8_t tflags = dctx_flags & ~SFRAME_F_FDE_SORTED;
>>      tflags |= SFRAME_F_LD_MUSTHAVE_FLAGS;
>>      sframe_encode (..., tflags, ...)
>>
>> instead of the current:
>>
>>      uint8_t tflags = dctx_flags & ~SFRAME_F_FDE_SORTED;
>>      tflags |= SFRAME_F_FDE_FUNC_START_PCREL;
>>      sframe_encode (..., tflags, ...)
>>
>> I think latter is better as it is explicit in the way flags are set for
>> ectx.
> I fear I didn't quite get where exactly you mean to have this.

This is done when setting up the encoder context the first time in 
_bfd_elf_merge_section_sframe ().