[V2 26/36] [SFrame-V3] gas: sframe: testsuite: handle .cfi_offset for RA
Jens Remus
jremus@linux.ibm.com
Thu Jan 8 12:42:41 GMT 2026
On 1/8/2026 9:11 AM, Indu Bhagat wrote:
> On 1/7/26 7:20 AM, Jens Remus wrote:
>> On 1/7/2026 9:42 AM, Indu Bhagat wrote:
>>> With the introduction of flex FDE type, handling .cfi_offset for
>>> RA needs adjustment.
>>>
>>> On architectures like x86_64, the return address (RA) is typically saved
>>> at a fixed offset from the CFA. Previous versions of the SFrame format
>>> assumed this fixed offset was invariant for the entire function on such
>>> architectures. Consequently, GAS would warn and suppress SFrame
>>> generation if it encountered a .cfi_offset directive for the RA
>>> register that deviated from this fixed default.
>>>
>>> SFrame V3 introduces "Flex FDEs" which allows tracking the RA location
>>> explicitly even on architectures where it is usually fixed.
>>>
>>> This patch updates sframe_xlate_do_offset () to leverage Flex FDEs. When
>>> processing a .cfi_offset for the RA register:
>>> - The check for non-representable RA offsets is relaxed. If the ABI
>>> supports Flex FDEs (SFrame V3), GAS proceeds instead of issuing a
>>> warning.
>>> - For ABIs without explicit RA tracking (like AMD64), if the RA
>>> offset differs from the default fixed offset, the FDE is marked as a
>>> Flex FDE, and the new stack location is recorded.
>>> - Logic is added to detect when the RA is restored to its standard
>>> fixed offset. In this case, the tracking state is reset (ra_loc set
>>> to SFRAME_FRE_ELEM_LOC_NONE), deferring to the ABI's default fixed RA
>>> offset behavior.
>>>
>>> gas/
>>> * gen-sframe.c (sframe_xlate_do_offset): Support .cfi_offset for RA
>>> by switching to Flex FDEs when necessary.
>>> gas/testsuite/
>>> * gas/cfi-sframe/cfi-sframe-x86_64-6.d: New test.
>>> * gas/cfi-sframe/cfi-sframe-x86_64-6.s: New test to check
>>> transition of location of REG_RA from register to CFA-8 (default
>>> location on AMD64). Flex FDE in effect.
>>> * gas/cfi-sframe/cfi-sframe.exp: Add new test.
>>
>> Reviewed-by: Jens Remus <jremus@linux.ibm.com>
>>
>>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>>
>>> @@ -1390,8 +1390,10 @@ sframe_xlate_do_offset (struct sframe_xlate_ctx *xlate_ctx,
>>> /* For ABIs not tracking RA, the return address is expected to be in a
>>> specific location. Explicit manourvering to a different offset (than the
>>> - default offset) is non-representable in SFrame. */
>>> - if (!sframe_ra_tracking_p () && cfi_insn->u.ri.reg == SFRAME_CFA_RA_REG
>>> + default offset) is non-representable in SFrame, unless flex FDE generation
>>> + is supported for the ABI. */
>>> + if (!sframe_support_flex_fde_p () && !sframe_ra_tracking_p ()
>>> + && cfi_insn->u.ri.reg == SFRAME_CFA_RA_REG
>>> && cfi_insn->u.ri.offset != sframe_cfa_ra_offset ())
>>> {
>>> as_warn (_("no SFrame FDE emitted; %s register %u in .cfi_offset"),
>>> @@ -1409,13 +1411,42 @@ sframe_xlate_do_offset (struct sframe_xlate_ctx *xlate_ctx,
>>> cur_fre->fp_deref_p = true;
>>> cur_fre->merge_candidate = false;
>>> }
>>> - else if (sframe_ra_tracking_p ()
>>> - && cfi_insn->u.ri.reg == SFRAME_CFA_RA_REG)
>>> + /* Either the ABI has enabled RA tracking, in which case we must process the
>>> + DW_CFA_offset opcode for REG_RA like usual. Or if the ABI has not enabled
>>> + RA tracking, but flex FDE generation is supported, dintinguish between
>>
>> Nit: s/dintinguish/distinguish/
>>
>>> + whether its time to reset the ra tracking state or not. */
>>
>> Nit: s/ra/RA/
>>
>>> + else if (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;
>>> - cur_fre->merge_candidate = false;
>>> + if (!sframe_ra_tracking_p ())
>>> + {
>>> + /* RA is restored to its standard fixed offset. */
>>
>> Maybe: s/RA/RA tracking info/
>>
>> The comment is kind of confusing as what it states is not performed
>> unconditionally. Maybe change as follows:
>>
>> /* Reset RA tracking info, if fixed offset. */
>>
>>> + if (cfi_insn->u.ri.offset == sframe_cfa_ra_offset ())
>>> + {
>>> + cur_fre->ra_reg = SFRAME_FRE_REG_INVALID;
>>> + cur_fre->ra_loc = SFRAME_FRE_ELEM_LOC_NONE;
>>> + cur_fre->ra_deref_p = false;
>>> + cur_fre->merge_candidate = false;
>>> + }
>>> + /* If flex FDE is supported, update the ra tracking info. */
>>
>> Nit: s/ra/RA/
>>
>>> + else if (sframe_support_flex_fde_p ())
>>> + {
>>> + sframe_fre_set_ra_track (cur_fre, cfi_insn->u.ri.offset);
>>> + cur_fre->ra_reg = SFRAME_FRE_REG_INVALID;
>>> + cur_fre->ra_loc = SFRAME_FRE_ELEM_LOC_STACK;
>>> + cur_fre->ra_deref_p = true;
>>> + cur_fre->merge_candidate = false;
>>> +
>>> + xlate_ctx->flex_p = true;
>>> + }
>>> + }
>>> + else if (sframe_ra_tracking_p ())
>>
>> Can't this be simplified to:
>>
>> else
>> or
>> else /* sframe_ra_tracking_p () */
>>
>
> Yes, this is doable.
>
>> If the sframe_ra_tracking_p () case is handled first then the else
>> would be close to the if condition:
>>
>> if (!sframe_ra_tracking_p ())
>> {
>> sframe_fre_set_ra_track (cur_fre, cfi_insn->u.ri.offset);
>> cur_fre->ra_reg = SFRAME_FRE_REG_INVALID;
>> cur_fre->ra_loc = SFRAME_FRE_ELEM_LOC_STACK;
>> cur_fre->ra_deref_p = true;
>> cur_fre->merge_candidate = false;
>> }
>> else
>> {
>> ...
>> }
>>
>
> I dont understand this part though.
Instead of:
if (!sframe_ra_tracking_p ())
{
... long block for disabled RA tracking ...
}
else
{
... short block for enabled RA tracking ...
}
better do:
if (sframe_ra_tracking_p ())
{
... short block for enabled RA tracking ...
}
else
{
... long block for disabled RA tracking ...
}
That way when a reader of the code reaches "else" the "if" condition is
in the vincinity of a few lines close by. But that is irrelevant if you
chose my rewrite of the whole block below at the bottom instead.
>
>>> + {
>>> + sframe_fre_set_ra_track (cur_fre, cfi_insn->u.ri.offset);
>>> + cur_fre->ra_reg = SFRAME_FRE_REG_INVALID;
>>> + cur_fre->ra_loc = SFRAME_FRE_ELEM_LOC_STACK;
>>
>> If I am not mistaken this was not part of the old implementation. Why
>> is that required?
>>
>
> Correct. In the RFC implmentation, we were not handling a few scenarios:
>
> Case 1:
> - RA tracking not enabled, e.g., AMD64, which uses fixed RA offset
> - RA offset != fixed RA offset shows up
> => we should switch to flex FDE and record the RA offset
>
> Case 2:
> - RA tracking not enabled, e.g., AMD64, which uses fixed RA offset
> - flex FDE in effect
> - RA offset = fixed RA offset shows up
> => we should record that RA tracking info is now the same as the default
> ra_reg = SFRAME_FRE_REG_INVALID;
> ra_loc = SFRAME_FRE_ELEM_LOC_NONE;
> ra_deref_p = false;
> By doing so, we will emit no more offsets after CFA offsets when the RA recovery is the default rule and FP is not tracked either.
>
> See testcase gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-6.d for testing this "transition" of RA offset for AMD64.
>
> (PS: I have made thinkos here, I appreciate you checking.)
This is in the RA tracking case. My comments made that hard to follow.
I was referring to the old code prior to this single patch (not the
whole series, neither a previous version of the series):
- 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;
- cur_fre->merge_candidate = false;
...
+ else if (sframe_ra_tracking_p ())
+ {
+ sframe_fre_set_ra_track (cur_fre, cfi_insn->u.ri.offset);
+ cur_fre->ra_reg = SFRAME_FRE_REG_INVALID;
+ cur_fre->ra_loc = SFRAME_FRE_ELEM_LOC_STACK;
This one line above is now added for the RA tracking case. Why?
+ cur_fre->ra_deref_p = true;
+ cur_fre->merge_candidate = false;
+ }
>
>>> + cur_fre->ra_deref_p = true;
>>> + cur_fre->merge_candidate = false;
>>> + }
>>> }
>>> /* This is used to track changes to non-rsp registers, skip all others
>>
>> Unrelated to this patch: What are non-rsp registers?
>>
>
> I think this comment meant to say that all others except FP/RA are OK to
> skip (with a previous comment saying "Ignore SP reg, as it can be recovered
> from the CFA tracking info."). For REG_SP, as you indicated in review on
> another patch, I will later address .cfi_offset REG_SP,XX for non-s390x in
> a separate patch.
>
> As for the comment above, I think we should remove it because it is
> confusing:
>
> - /* This is used to track changes to non-rsp registers, skip all others
> - except FP / RA for now. */
> +
> return SFRAME_XLATE_OK;
Ok. Or just say:
/* Skip all other registers. */
To make clear that this return may be reached for other (call saved)
registers than SP, FP, and RA.
>
>
>>> except FP / RA for now. */
>> Maybe the SFRAME_CFA_RA_REG case can be simplified as follows:
>>
>> else if (cfi_insn->u.ri.reg == SFRAME_CFA_RA_REG)
>> {
>> if (!sframe_ra_tracking_p ()
>> && cfi_insn->u.ri.offset == sframe_cfa_ra_offset ())
>> {
>> /* Reset RA tracking info to fixed offset. */
>> cur_fre->ra_reg = SFRAME_FRE_REG_INVALID;
>> cur_fre->ra_loc = SFRAME_FRE_ELEM_LOC_NONE;
>> cur_fre->ra_deref_p = false;
>> cur_fre->merge_candidate = false;
>> }
>> else
>> {
>> sframe_fre_set_ra_track (cur_fre, cfi_insn->u.ri.offset);
>> cur_fre->ra_reg = SFRAME_FRE_REG_INVALID;
>> cur_fre->ra_loc = SFRAME_FRE_ELEM_LOC_STACK;
>> cur_fre->ra_deref_p = true;
>> cur_fre->merge_candidate = false;
>>
>> if (!sframe_ra_tracking_p () && sframe_support_flex_fde_p ())
>> xlate_ctx->flex_p = true;
>> }
>> }
>>
>
> OK.
>
>> The following common statements could be moved from the inner to the
>> outer clause. I left them duplicated as I found that more readable.
>>
>> cur_fre->ra_reg = SFRAME_FRE_REG_INVALID;
>> cur_fre->merge_candidate = false;
>>
>
> Yes, I prefer the duplicated version too for now.
Regards,
Jens
--
Jens Remus
Linux on Z Development (D3303)
jremus@de.ibm.com / jremus@linux.ibm.com
IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Ehningen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/
More information about the Binutils
mailing list