[V2 18/36] [SFrame-V3] gas: sframe: output new FDE type SFRAME_FDE_TYPE_FLEX
Indu Bhagat
indu.bhagat@oracle.com
Fri Jan 9 08:21:06 GMT 2026
On 1/8/26 7:43 AM, Jens Remus wrote:
> On 1/7/2026 9:42 AM, Indu Bhagat wrote:
>> This patch updates the routines for emission of the new FDE type
>> SFRAME_FDE_TYPE_FLEX in the SFrame output section. The support for
>> generating these flex FDEs themselves is added in a subsequent commit.
>>
>> For flex FDE type, output_sframe_row_entry_offsets () emits two
>> offsets for RA tracking, irrespective of the ABI/arch, i.e.,
>> irrespecitve of whether RA-tracking is enabled. This is because, for
>> flex FDE type, RA is allowed "non-standard" recovery rules,
>> e.g., RA = REG or RA = *(REG+offset). In a follow-up patch, when
>> handling for .cfi_register RA, REG is added, emission code will be
>> patched up accodingly.
>>
>> TBD - gas/gen-sframe.c code can be improved ?
>>
>> gas/
>> * gen-sframe.c (get_udata_size_in_bytes): Get size of unsigned
>> int data in bytes.
>> (get_fre_num_offsets): Get number of offsets for
>> SFRAME_FDE_TYPE_FLEX fde type as well (new argument).
>> (output_sframe_row_entry_offsets): Output the SFrame FREs for
>> two different cases (fde type SFRAME_FDE_TYPE_FLEX or the
>> default).
>> (output_sframe_row_entry): Update caller.
>> (output_sframe_funcdesc): Emit func type in func_info2 field.
>> (sframe_xlate_ctx_init): Initialize flex_p in translation
>> context object.
>> (sframe_xlate_ctx_finalize): Likewise.
>> (sframe_row_entry_initialize): Copy over new data members too.
>> * gen-sframe.h (struct sframe_row_entry): Add tracking for CFA
>> dereferencing, frame pointer dereferencing.
>> (struct sframe_func_entry): Add tracking for flexible FDE type.
>> (struct sframe_xlate_ctx): Likewise.
>>
>> ---
>> [Changes in V1]
>> - Rename the FDE type to SFRAME_FDE_TYPE_FLEX. [Jens]
>> - Fix generation of RA offsets for flex FDE. In RFC, we were emitting
>> two offsets for flex FDE, irrespctive of whether RA tracking location
>> was REG/STACK or NONE. This is now changed to: emit RA padding offset
>> - Bring proper implementation in now and remove FIXME in
>> output_sframe_row_entry_offsets [Jens].
>> - gas: sframe: must reset and init ra_reg appropriately.
>> - Allow reg to be any sized in the offset. In RFC, there was a limit
>> of 5-bits. Now its lifted. The calculation of the `size of offset`
>> is adjusted to account for the minimum necessary size, just like the
>> other offsets.
>> [End of changes in V1]
>>
>> [Changes in V2]
>> - Bring stub resetting ra_reg, ra_deref_p, fp_deref_p to the rightful
>> place (from previous commit) [Indu].
>> - Remove stale comments from output_sframe_row_entry_offsets () that
>> two offsets are emitted for RA irrespective of whether its tracked
>> or not [Indu].
>> - Other minor touch ups.
>> [End of changes in V2]
>
>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>
>> @@ -233,6 +233,22 @@ get_offset_size_in_bytes (offsetT value)
>> return size;
>> }
>>
>> +/* Given an unsigned item, return the size in bytes needed to represent it. */
>> +
>> +static unsigned int
>> +get_udata_size_in_bytes (unsigned int value)
>
> What about unsigned long value and adding another case for 64-bit to
> align to get_offset_size_in_bytes?
>
> else if ((sizeof (unsigned long) > 4) && (value <= UINT64_MAX))
> return 8;
>
SFrame doesnt allow "offsets" of size > 4 bytes.
/* Size of stack frame offsets in an SFrame Frame Row Entry. A single
SFrame FRE has all offsets of the same size. Offset size may vary
across frame row entries. */
#define SFRAME_FRE_OFFSET_1B 0
#define SFRAME_FRE_OFFSET_2B 1
#define SFRAME_FRE_OFFSET_4B 2
>> +{
>> + unsigned int size = 0;
>> +
>> + if (value <= VALUE_8BIT + 1)
>
> While this could be rewritten as (value < VALUE_8BIT) this seems
> wrong to me:
>
> VALUE_8BIT = 0x7F = 127
> VALUE_8BIT + 1 = 0x80 = 128
>
> Shouldn't the test be for (value <= 0xFF)?
>
Yes, it should be. I messed up.
Together with this, we need to define a new access API for reading the
register/control data word as unsigned int.
> Using the proper condition here reveals the broken "Read the Register/
> Control Data as unsigned" logic in a subsequent patch. See my feedback
> there.
>
> Is there a reason to use VALUE_{8|16}BIT instead of the standard
> UINT8_MAX and UINT16_MAX constants?
>
> Same for get_offset_size_in_bytes(), which could use the standard
> INT{8|16|32|64}_MIN and INT{8|16|32|64}_MAX constants.
>
I am not sure why I didnt use the INTXX_MIN and INTXX_MAX back then. I
see that stdint.h is fine to use in gas.
> Or are there targets where these are not defined (e.g. 32-bit ones
> lacking INT64_{MIN|MAX} and UINT64_MAX?
>
IIUC, this should be fine too.
>> + size = 1;
>> + else if (value <= VALUE_16BIT + 1)
>
> Dito.
>
>> + size = 2;
>> + else
>> + size = 4;
>> +
>> + return size;
>> +}
>> #define SFRAME_FRE_OFFSET_FUNC_MAP_INDEX_1B 0 /* SFRAME_FRE_OFFSET_1B. */
>> #define SFRAME_FRE_OFFSET_FUNC_MAP_INDEX_2B 1 /* SFRAME_FRE_OFFSET_2B. */
>> #define SFRAME_FRE_OFFSET_FUNC_MAP_INDEX_4B 2 /* SFRAME_FRE_OFFSET_4B. */
>> @@ -380,19 +396,36 @@ get_fre_base_reg_id (const struct sframe_row_entry *sframe_fre)
>> /* Get number of offsets necessary for the SFrame Frame Row Entry. */
>>
>> static unsigned int
>> -get_fre_num_offsets (const struct sframe_row_entry *sframe_fre)
>> +get_fre_num_offsets (const struct sframe_row_entry *sframe_fre,
>> + bool flex_p)
>> {
>> - /* Atleast 1 must always be present (to recover CFA). */
>> - unsigned int fre_num_offsets = 1;
>> + /* For SFRAME_FDE_TYPE_FLEX FDE type, each entity (CFA, FP, RA) may carry up
>> + to two offsets. */
>> + unsigned int count = flex_p ? 2 : 1;
>
> Up to two or exactly two? See my feedback on RA padding location data
> below.
>
Up to two.
Following cases may occur for flex FDEs:
- For ABIs tracking not RA, e.g. AMD64, for some FREs, there may be only
2 "offsets" if only CFA recovery rules are specified.
- If RA recovery rules are available, there will be 4 offsets (2 for
CFA, 2 for RA)
- If no RA recovery rule is applicable, but there is an FP recovery
rule, there will be 5 offsets (2 for CFA, 1 Padding for RA, 2 for FP)
So for RA, there may be 0, 1, or 2 "offsets".
My understanding is that you indicated preference for the above scheme
over the previous RFC where we were always emitting 2 offsets for RA.
In principle, the scheme in V1/V2 series aligns better with how you
approached the matter for default FDEs for s390x.
>>
>> - if (sframe_fre->fp_loc == SFRAME_FRE_ELEM_LOC_STACK)
>> - fre_num_offsets++;
>> - if (sframe_ra_tracking_p ()
>> - && (sframe_fre->ra_loc == SFRAME_FRE_ELEM_LOC_STACK
>> - /* For s390x account padding RA offset, if FP without RA saved. */
>> - || (sframe_get_abi_arch () == SFRAME_ABI_S390X_ENDIAN_BIG
>> - && sframe_fre->fp_loc == SFRAME_FRE_ELEM_LOC_STACK)))
>> + /* CFA offset (or offsets when flex_p) must always be present. */
>> + unsigned int fre_num_offsets = count;
>> +
>> + /* For flexible frames encoding, there will be two offsets for RA (if RA is
>> + being tracked). 1 padding offset otherwise. */
>
> How can a stack tracer differentiate "1 padding offset" from the
> following valid RA location information:
>
> reg_data = SFRAME_V3_FLEX_FDE_REG_ENCODE (0 /* reg */, 0 /* deref_p */, 0 /* reg_p */);
> offset_data = sframe_fre->ra_offset;
>
Above means RA = CFA + offset, i.e., .cfi_val_offset semantics. This is
not representable in flex FDEs.
[Note: this is not RA = *(CFA + offset), for which one will see
SFRAME_V3_FLEX_FDE_REG_ENCODE ( 0, 1, 0)]
Similarly FP = CFA + offset is not representable in flex FDEs.
Such values are simply stack locations.
>> + if (flex_p)
>> + {
>> + if (sframe_fre->ra_loc != SFRAME_FRE_ELEM_LOC_NONE)
>> + fre_num_offsets += count;
>> + else if (sframe_fre->fp_loc != SFRAME_FRE_ELEM_LOC_NONE)
>> + fre_num_offsets += 1;
>> + }
>> + else if (sframe_ra_tracking_p ()
>> + && (sframe_fre->ra_loc != SFRAME_FRE_ELEM_LOC_NONE
>> + /* For s390x account padding RA offset, if FP without RA
>> + saved. */
>> + || (sframe_get_abi_arch () == SFRAME_ABI_S390X_ENDIAN_BIG
>> + && sframe_fre->fp_loc != SFRAME_FRE_ELEM_LOC_NONE)))
>> fre_num_offsets++;
>> +
>> + if (sframe_fre->fp_loc != SFRAME_FRE_ELEM_LOC_NONE)
>> + fre_num_offsets += count;
>> +
>> return fre_num_offsets;
>> }
>
>> @@ -588,39 +649,91 @@ sframe_fde_free (struct sframe_func_entry *sframe_fde)
>> size FRE_OFFSET_SIZE. Write out the offsets in order - CFA, RA, FP. */
>>
>> static unsigned int
>> -output_sframe_row_entry_offsets (const struct sframe_func_entry *sframe_fde ATTRIBUTE_UNUSED,
>> +output_sframe_row_entry_offsets (const struct sframe_func_entry *sframe_fde,
>> const struct sframe_row_entry *sframe_fre,
>> unsigned int fre_offset_size)
>> -
>> {
>> unsigned int fre_write_offsets = 0;
>>
>> unsigned int idx = sframe_fre_offset_func_map_index (fre_offset_size);
>> gas_assert (idx < SFRAME_FRE_OFFSET_FUNC_MAP_INDEX_MAX);
>>
>> - /* Write out the offsets in order - cfa, fp, ra. */
>> - fre_offset_func_map[idx].out_func (sframe_fre->cfa_offset);
>> - fre_write_offsets++;
>> -
>> - if (sframe_ra_tracking_p ())
>> + if (sframe_fde->fde_flex_p)
>> {
>> - if (sframe_fre->ra_loc == SFRAME_FRE_ELEM_LOC_STACK)
>> + /* SFrame FDE of type SFRAME_FDE_TYPE_FLEX. */
>> + /* Output CFA related FRE offsets. */
>> + uint32_t reg_data
>> + = SFRAME_V3_FLEX_FDE_REG_ENCODE (sframe_fre->cfa_base_reg,
>> + sframe_fre->cfa_deref_p,
>> + 1 /* reg_p. */);
>> + offsetT offset_data = sframe_fre->cfa_offset;
>> + fre_offset_func_map[idx].out_func (reg_data);
>> + fre_offset_func_map[idx].out_func (offset_data);
>> + fre_write_offsets += 2;
>> +
>> + bool reg_p = false;
>> + if (sframe_fre->ra_loc != SFRAME_FRE_ELEM_LOC_NONE)
>> {
>> - fre_offset_func_map[idx].out_func (sframe_fre->ra_offset);
>> - fre_write_offsets++;
>> + /* Output RA related FRE offsets. */
>> + reg_p = sframe_fre->ra_loc == SFRAME_FRE_ELEM_LOC_REG;
>> + reg_data = SFRAME_V3_FLEX_FDE_REG_ENCODE (sframe_fre->ra_reg,
>> + sframe_fre->ra_deref_p,
>> + reg_p);
>> + offset_data = sframe_fre->ra_offset;
>> + fre_offset_func_map[idx].out_func (reg_data);
>> + fre_offset_func_map[idx].out_func (offset_data);
>> + fre_write_offsets += 2;
>> }
>> - /* For s390x write padding RA offset, if FP without RA saved. */
>> - else if (sframe_get_abi_arch () == SFRAME_ABI_S390X_ENDIAN_BIG
>> - && sframe_fre->fp_loc == SFRAME_FRE_ELEM_LOC_STACK)
>> + else if (sframe_fre->fp_loc != SFRAME_FRE_ELEM_LOC_NONE)
>> {
>> + /* If RA is not in REG/STACK, emit RA padding if there are more
>> + offsets to follow. Note that, emitting
>> + SFRAME_FRE_RA_OFFSET_INVALID is equivalent to emitting
>> + SFRAME_V3_FLEX_FDE_REG_ENCODE (0, 0, 0). */
>> fre_offset_func_map[idx].out_func (SFRAME_FRE_RA_OFFSET_INVALID);
>> - fre_write_offsets++;
>> + fre_write_offsets += 1;
>
> Shouldn't two SFRAME_FRE_RA_OFFSET_INVALID be written? One for reg_data
> and another for offset_data?
>
> reg_data = SFRAME_V3_FLEX_FDE_REG_ENCODE (0, 0, 0);
> offset_data = SFRAME_FRE_RA_OFFSET_INVALID;
>
> Otherwise a stack tracer needs to cope with flex FDEs where a FRE's
> tracked location item can consist of either one or two "offsets", at
> least for RA.
Yes, it does make the life of stack tracer burdened some additional
conditionals to figure out the exact index to read data RA/FP data from.
But as you pointed out for the RFC series, emitted two offsets always
will be a more bytes emitted than the minimum necessary.
How would a reg_data value of
> SFRAME_V3_FLEX_FDE_REG_ENCODE (0, 0, 0) indicate that there is no
> offset_data?
The same way we used SFRAME_FRE_RA_OFFSET_INVALID as padding ?
If you introduce a fourth tracked item (e.g. SP), how
> would the single padding "offset" for RA and FP work?
>
SFRAME_V3_FLEX_FDE_REG_ENCODE (0, 0, 0) can be used as invalid FP
offset/padding too. This is because the following:
fp_reg_data = SFRAME_V3_FLEX_FDE_REG_ENCODE (0, 0, 0)
fp_offset = XX
means:
FP = CFA + offset [not FP = *(CFA + offset)]
The former is not representable in flex FDE (and should not be necessary
for CFA, FP, SP, or RA if I am not mistaken). The latter will have
SFRAME_V3_FLEX_FDE_REG_ENCODE (0, 1, 0) to indicate deref_p.
IOW, flex FDE do not represent .cfi_val_offset still.
>> + }
>> +
>> + if (sframe_fre->fp_loc != SFRAME_FRE_ELEM_LOC_NONE)
>> + {
>> + /* Output FP related FRE offsets. */
>> + reg_p = sframe_fre->fp_loc == SFRAME_FRE_ELEM_LOC_REG;
>> + reg_data = SFRAME_V3_FLEX_FDE_REG_ENCODE (sframe_fre->fp_reg,
>> + sframe_fre->fp_deref_p,
>> + reg_p);
>> + offset_data = sframe_fre->fp_offset;
>> + fre_offset_func_map[idx].out_func (reg_data);
>> + fre_offset_func_map[idx].out_func (offset_data);
>> + fre_write_offsets += 2;
>> }
>> }
>> - if (sframe_fre->fp_loc == SFRAME_FRE_ELEM_LOC_STACK)
>> + else
>> {
>> - fre_offset_func_map[idx].out_func (sframe_fre->fp_offset);
>> + /* SFrame FDE of type SFRAME_FDE_TYPE_DEFAULT. */
>> + /* Output CFA related FRE offsets. */
>> + fre_offset_func_map[idx].out_func (sframe_fre->cfa_offset);
>> fre_write_offsets++;
>> +
>> + if (sframe_ra_tracking_p ())
>> + {
>> + if (sframe_fre->ra_loc == SFRAME_FRE_ELEM_LOC_STACK)
>> + {
>> + fre_offset_func_map[idx].out_func (sframe_fre->ra_offset);
>> + fre_write_offsets++;
>> + }
>> + /* For s390x write padding RA offset, if FP without RA saved. */
>> + else if (sframe_get_abi_arch () == SFRAME_ABI_S390X_ENDIAN_BIG
>> + && sframe_fre->fp_loc == SFRAME_FRE_ELEM_LOC_STACK)
>> + {
>> + fre_offset_func_map[idx].out_func (SFRAME_FRE_RA_OFFSET_INVALID);
>> + fre_write_offsets++;
>> + }
>> + }
>> + if (sframe_fre->fp_loc == SFRAME_FRE_ELEM_LOC_STACK)
>> + {
>> + fre_offset_func_map[idx].out_func (sframe_fre->fp_offset);
>> + fre_write_offsets++;
>> + }
>> }
>>
>> return fre_write_offsets;
>> @@ -1255,12 +1380,15 @@ sframe_xlate_do_offset (struct sframe_xlate_ctx *xlate_ctx,
>> {
>> sframe_fre_set_fp_track (cur_fre, cfi_insn->u.ri.offset);
>> cur_fre->fp_reg = SFRAME_FRE_REG_INVALID;
>> + cur_fre->fp_deref_p = true;
>
> Why did you decide to set this here instead of in
> sframe_fre_set_fp_track()? Shouldn't sframe_fre_set_fp_track actually
> do both, reset fp_reg and set fp_deref_p, as it already sets ra_loc to
> SFRAME_FRE_ELEM_LOC_STACK?
>
I wanted to see what abstraction/API will work eventually, until then I
continued with explicit assignments everywhere. I planned to clean this
up later.
>> cur_fre->merge_candidate = false;
>> }
>> else if (sframe_ra_tracking_p ()
>> && cfi_insn->u.ri.reg == SFRAME_CFA_RA_REG)
>> {
>> sframe_fre_set_ra_track (cur_fre, cfi_insn->u.ri.offset);
>> + cur_fre->ra_reg = SFRAME_FRE_REG_INVALID;
>> + cur_fre->ra_deref_p = true;
>
> Same for sframe_fre_set_ra_track()?
>
Same as above.
>> cur_fre->merge_candidate = false;
>> }
>> /* This is used to track changes to non-rsp registers, skip all others
> Regards,
> Jens
More information about the Binutils
mailing list