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

[Jan Beulich]

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.

Jan