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

[Indu Bhagat]

On 6/30/25 11:32 PM, Jan Beulich wrote:
> On 01.07.2025 08:30, Jan Beulich wrote:
>> On 01.07.2025 08:19, Indu Bhagat wrote:
>>> 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 ().
>>
>> Hmm, but shouldn't the decoder set flags based on the input it means to
>> consume?
> 
> Oh, sorry (noticed just after hitting "send") - I don't understand why I
> thought I read "decode" anywhere. All is fine.
> 

Hi Jan,

Thanks for reviewing, As this is the only patch that hasnt been ACKed 
yet.  I will post a V5 of just this one shortly.

Thanks
Indu