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

[Indu Bhagat]

On 1/8/26 4:42 AM, Jens Remus wrote:
> 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.
> 

Ah ok. Yeah, for now I have switched to your proposed version below.

>>
>>>> +    {
>>>> +      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?
> 

The setting of cur_fre->ra_loc to SFRAME_FRE_ELEM_LOC_STACK is 
superfluous (sframe_fre_set_ra_track internally does that too).  Its 
there because of my slight preference to keep it stated explicitly, as I 
was intending to abstract the setting of XX_reg, XX_loc, XX_deref_p via 
an API of some kind.  I will likely get to that later sometime.

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

OK, sure.

Thanks