[PATCH v4 1/4] gas: ld: libsframe: Support for SFrame FDEs without any FREs

[Indu Bhagat]

On 7/29/25 2:06 AM, Jens Remus wrote:
> Hello Indu,
> 
> thank you for the feedback!
> 
> On 7/29/2025 8:49 AM, Indu Bhagat wrote:
>> On 7/25/25 9:23 AM, Jens Remus wrote:
>>> Allow SFrame sections without any FREs, that can occur if they solely
>>> contain FDEs without any FREs.  For FDEs without and FREs set the
>>> offset to the first FRE to zero.
>>>
>>> libsframe/
>>>      * sframe.c (sframe_encoder_write_sframe): Allow SFrame sections
>>>      without any FREs.  For FDEs without any FREs set the offset to
>>>      the first FRE to zero.
>>>
>>> gas/
>>>      * gen-sframe.c (output_sframe_funcdesc): For FDEs without any
>>>      FREs set the offset to the first FRE to zero.
>>>
>>> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
>>> ---
>>>
>>> Notes (jremus):
>>>       Changes in V4:
>>>       - Use ehp->sfh_num_fres instead of adding a new API to get the total
>>>         number of FREs. (Indu)
>>>       - Squash related assembler change from V3.
>>>       - Reword commit subject and message.
>>>            Changes in V3:
>>>       - New patch.
>>>            This patch is strictly no longer required.  Posting again to keep a
>>>       copy for future reference, even if we decide not to make this change
>>>       now.  For the time being we might prefer to error out in the linker
>>>       when running into SFrame FDEs without any FREs.
>>>
>>
>> Whats the reason to prefer to error out for SFrame FDE without any FRE ?
> 
> Because that is currently not used and therefore should not occur.
> 
> But if you envision that a FRE without FDE might be useful in the future
> to convey information about a range of PCs, then we should enable that
> now, and document it it in the SFrame specification, so that other SFrame
> implementations (e.g. in LLVM assembler and linker) do so as well.
> 

Yeah, I think stack tracers should not falter if they see an SFrame FDE 
with no FREs and DTRT, so lets allow such FDEs.  To be clear, there is 
no necessity right away for such FDEs to be generated, so documenting 
this is not necessary ATM.  When we do add some function-level 
information that needs to be carried in such SFrame FDEs, we can add 
that to the spec in context of the added information.

>>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> 
>>> @@ -651,12 +651,18 @@ output_sframe_funcdesc (symbolS *start_of_fre_section,
>>>                      sfde_func_size));
>>>        /* Offset to the first frame row entry.  */
>>> -  exp.X_op = O_subtract;
>>> -  exp.X_add_symbol = fre_symbol; /* Minuend.  */
>>> -  exp.X_op_symbol = start_of_fre_section; /* Subtrahend.  */
>>> -  exp.X_add_number = 0;
>>> -  emit_expr (&exp, sizeof_member (sframe_func_desc_entry,
>>> -                  sfde_func_start_fre_off));
>>> +  if (sframe_fde->num_fres == 0)
>>> +    /* SFrame FDEs without any FREs have a FRE offset of zero.  */
> 
> This comment should be in sync with the one below.  At least for this
> patch.
> 
>>> +    out_four (0);
>>> +  else
>>> +    {
>>> +      exp.X_op = O_subtract;
>>> +      exp.X_add_symbol = fre_symbol; /* Minuend.  */
>>> +      exp.X_op_symbol = start_of_fre_section; /* Subtrahend.  */
>>> +      exp.X_add_number = 0;
>>> +      emit_expr (&exp, sizeof_member (sframe_func_desc_entry,
>>> +                      sfde_func_start_fre_off));
>>> +    }
>>>        /* Number of FREs.  */
>>>      out_four (sframe_fde->num_fres);
>>> diff --git a/libsframe/sframe.c b/libsframe/sframe.c
> 
>>> @@ -1880,7 +1880,7 @@ sframe_encoder_write_sframe (sframe_encoder_ctx *encoder)
>>>         - buffers must be malloc'd by the caller.  */
>>>      if ((contents == NULL) || (buf_size < hdr_size))
>>>        return sframe_set_errno (&err, SFRAME_ERR_BUF_INVAL);
>>> -  if (fr_info == NULL)
>>> +  if (ehp->sfh_num_fres > 0 && fr_info == NULL)
> 
> This relaxes the check to allow the pointer to the SFrame encoder'
> global list of FREs to be NULL, if the total FRE count is zero.  This
> could occur, if there are only FDEs without any FREs.
> 
>>>        return sframe_set_errno (&err, SFRAME_ERR_FRE_INVAL);
>>>        /* Write out the FRE table first.
>>> @@ -1903,6 +1903,13 @@ sframe_encoder_write_sframe (sframe_encoder_ctx *encoder)
>>>          fre_type = sframe_get_fre_type (fdep);
>>>          num_fres = fdep->sfde_func_num_fres;
>>>    +      /* FDEs without any FDEs have a FRE offset of zero.  */
>>
>> Perhaps reword for clarity:
>>
>> For FDES without any FREs, set sfde_func_start_fre_off to zero.
> 
> Wouldn't the comment then just duplicate the following line(s) of code?
> What about:
> 
>        /* FDEs without any FREs have an offset to first FRE of zero.  */
> 
>>> +      if (num_fres == 0)
>>> +    fdep->sfde_func_start_fre_off = 0;
>>> +
>>
>> Ah I see the adding funcdesc via sframe_encoder_add_funcdesc sets the
>> sfde_func_start_fre_off.  BTW, I am curious if not doing so shows up
>> as a problem somewhere ?
> 
> How would sfde_func_start_fre_off otherwise get a value, if
> sframe_encoder_add_funcdesc would not set it?  Or did you mean to not
> reset sfde_func_start_fre_off to zero if the FDE's FRE count is zero?
> 

No, I did not mean to say the latter.  I was sort of mumbling to myself 
and was wondering if you hit some error without that stub.

Resetting sfde_func_start_fre_off is a good thing to do, so all good.

> To me it looks libsframe expects sframe_encoder_add_funcdesc to be
> immediately followed by the sframe_encoder_add_fre calls for that FDE.
> It does not allow out of order sframe_encoder_add_fre calls for
> any previously added FDE but the last.  Or did I miss something?
> 

Yes this is correct.

>>> +      if (num_fres > 0 && fr_info == NULL)
>>> +    return sframe_set_errno (&err, SFRAME_ERR_FRE_INVAL);
>>> +
>>
>> Why check again per FDE ?
> 
> This is a consistency check for the total FRE count being zero but a FDE
> erroneously having a non-zero FRE count.  This should not occur.  We can
> omit it or make it an assertion.
> 

I see you prefer to keep in V5. Its fine. We can keep it.

>>
>>>          for (j = 0; j < num_fres; j++)
>>>        {
>>>          fre_index = global + j;
> Regards,
> Jens