[V2 26/36] [SFrame-V3] gas: sframe: testsuite: handle .cfi_offset for RA

[Indu Bhagat]

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.

>> +	{
>> +	  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.)

>> +	  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;


>>        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.