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

[Indu Bhagat]

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)
     {
       _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.