[PATCH] gas: sframe: fix PR gas/33756
Indu Bhagat
indu.bhagat@oracle.com
Thu Jan 8 08:19:22 GMT 2026
On 1/7/26 5:04 AM, Jens Remus wrote:
> On 1/7/2026 9:11 AM, Indu Bhagat via Binutils wrote:
>> Previously, sframe_xlate_do_remember_state () relied exclusively on
>> last_fre (the last fully constructed Frame Row Entry). At the start of
>> a function (before an advance in location), last_fre is NULL. The
>> intent of DW_CFA_remember_state, however, is to simply snapshot the
>> state of the call frame information at the time. In terms of SFrame
>> generation, this means that we should simply look at cur_fre (the
>> scratchpad FRE currently being built from initial CFI instructions),
>> should last_fre be NULL.
>>
>> Remove the common-empty-1.s which assumed that .cfi_startproc,
>> immediately followed by a .cfi_remember_state is not present out in the
>> wild. Its best to not make such an assumption, as such a sequence is
>> valid CFI, FWIW.
>>
>> Initial CFI are arch-specific, so add a new testcase for x86_64 to check
>> .cfi_remember_state handling for SFrame is sensible.
>
> I'll send a respective s390x test case in a separate patch.
>
>>
>> gas/
>> PR gas/33756
>> * gen-sframe.c (sframe_xlate_do_remember_state): Use cur_fre if
>> last_fre is not set.
>> gas/testsuite/
>> * gas/cfi-sframe/cfi-sframe.exp: Adjust common-empty-1 test to
>> be repurposed as cfi-sframe-x86_64-pr33756.
>> * gas/cfi-sframe/common-empty-1.d: Removed.
>> * gas/cfi-sframe/common-empty-1.s: Removed.
>> * gas/cfi-sframe/cfi-sframe-x86_64-pr33756.d: New test.
>> * gas/cfi-sframe/cfi-sframe-x86_64-pr33756.s: New test.
>
> Thanks for fixing!
>
>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>
>> @@ -1349,21 +1349,25 @@ sframe_xlate_do_register (struct sframe_xlate_ctx *xlate_ctx,
>> static int
>> sframe_xlate_do_remember_state (struct sframe_xlate_ctx *xlate_ctx)
>> {
>> - const struct sframe_row_entry *last_fre = xlate_ctx->last_fre;
>> + if (!xlate_ctx->remember_fre)
>> + xlate_ctx->remember_fre = sframe_row_entry_new ();
>>
>> - /* If there is no FRE state to remember, nothing to do here. Return
>> - early with non-zero error code, this will cause no SFrame stack trace
>> - info for the function involved. */
>> - if (!last_fre)
>> + const struct sframe_row_entry *prev_fre = xlate_ctx->last_fre;
>
> One could argue whether the local variables should be defined at the top
> of the function.
>
>> + /* If there is no previous saved FRE, use the state so far for the snapshot.
>
> I found "previous saved FRE" kind of confusing, but don't have a good
> alternative.
>
>> + .cfi_startproc followed by .cfi_remember_state is a valid sequence. */
>> + if (!prev_fre)
>> + prev_fre = xlate_ctx->cur_fre;
>> + /* If there is no previous FRE state to remember, nothing to do here. Return
>> + early with non-zero error code, this will cause no SFrame stack trace info
>> + for the function involved. */
>> + if (!prev_fre)
>> {
>> as_warn (_("no SFrame FDE emitted; "
>> ".cfi_remember_state without prior SFrame FRE state"));
>> return SFRAME_XLATE_ERR_INVAL;
>> }
>>
>> - if (!xlate_ctx->remember_fre)
>> - xlate_ctx->remember_fre = sframe_row_entry_new ();
>> - sframe_row_entry_initialize (xlate_ctx->remember_fre, last_fre);
>> + sframe_row_entry_initialize (xlate_ctx->remember_fre, prev_fre);
>>
>> return SFRAME_XLATE_OK;
>> }
>
> This needs to be simplified as follows. sframe_xlate_do_advance_loc
> allocates a new current FRE and initializes it with the state of the
> previous FRE. So always remembering the current FRE should be the
> right thing if I am not mistaken:
>
You're right. The fix was buggy with it taking last_fre as the first
candidate.
I've fixed it, and also added your s390x testcase.
As for the testcase for the exact pattern you mention below: In the
SFrame V3 patch series, there is a
gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-4.s testcase with this
pattern (which will need to be "fixed" too after the fix for PR
gas/33756 is in.
Thanks for reviewing. Will post V2 soon.
> @@ -1349,14 +1349,11 @@ sframe_xlate_do_register (struct sframe_xlate_ctx *xlate_ctx,
> static int
> sframe_xlate_do_remember_state (struct sframe_xlate_ctx *xlate_ctx)
> {
> + const struct sframe_row_entry *prev_fre = xlate_ctx->cur_fre;
> +
> if (!xlate_ctx->remember_fre)
> xlate_ctx->remember_fre = sframe_row_entry_new ();
>
> - const struct sframe_row_entry *prev_fre = xlate_ctx->last_fre;
> - /* If there is no previous saved FRE, use the state so far for the snapshot.
> - .cfi_startproc followed by .cfi_remember_state is a valid sequence. */
> - if (!prev_fre)
> - prev_fre = xlate_ctx->cur_fre;
> /* If there is no previous FRE state to remember, nothing to do here. Return
> early with non-zero error code, this will cause no SFrame stack trace info
> for the function involved. */
>
> Only with this change GAS' DWARF CFI and SFrame generation behave
> truly the same for both of the following variants:
>
> foo:
> .cfi_startproc
> .cfi_def_cfa_offset 32
> .long 0
> .cfi_def_cfa_offset 64
> .cfi_remember_state
> .long 0
> .cfi_def_cfa_offset 16
> .long 0
> .long 0
> .cfi_restore_state
> .long 0
> .cfi_endproc
>
> and
>
> foo:
> .cfi_startproc
> .cfi_def_cfa_offset 32
> .long 0
> .cfi_remember_state
> .cfi_def_cfa_offset 64
> .long 0
> .cfi_def_cfa_offset 16
> .long 0
> .long 0
> .cfi_restore_state
> .long 0
> .cfi_endproc
>
> Otherwise SFrame differs from DWARF CFI in the first case as follows,
> because it would remember the previous FRE instead of the current FRE:
>
> .eh_frame:
>
> 00000018 000000000000001c 0000001c FDE cie=00000000 pc=0000000000000000..0000000000000014
> LOC CFA ra
> 0000000000000000 rsp+32 c-8
> 0000000000000004 rsp+64 c-8
> 0000000000000008 rsp+16 c-8
> 0000000000000010 rsp+64 c-8 <-- DWARF CFI has +64
>
> .sframe:
>
> func idx [0]: pc = 0x0, size = 20 bytes
> STARTPC CFA FP RA
> 0000000000000000 sp+32 u f
> 0000000000000004 sp+64 u f
> 0000000000000008 sp+16 u f
> 0000000000000010 sp+32 u f <-- SFrame has +32 (because it remembered the previous FRE)
>
More information about the Binutils
mailing list