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

[Jan Beulich]

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.

Jan