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

[Jan Beulich]

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?

Jan