[PATCH v3 05/11] s390: Represent FP without RA saved in SFrame

[Jens Remus]

Hello Indu,

thank you for the review and feedback!

On 06.07.2025 01:16, Indu Bhagat via Binutils wrote:
> On 6/27/25 4:08 AM, Jens Remus wrote:
>> If an architecture uses both SFrame RA and FP tracking SFrame assumes
>> that the RA offset is the 2nd offset and the FP offset is the 3rd offset
>> following a SFrame FRE.  An architecture does not necessarily need to
>> save both on the stack (or in register) at the same time or even at all.
>> SFrame cannot represent FP without RA saved on stack (or in a register),
>> since it cannot distinguish whether the 2nd offset is the RA or FP
>> offset.
>>
>> For s390x use an invalid SFrame RA offset from CFA value of zero as
>> padding to represent the FP being saved when the RA is not saved.  This
>> aligns with the existing invalid SFrame fixed RA offset from CFA value
>> of zero.  In a stack tracer this then also naturally falls into place,
>> as it can skip restoring the RA in the topmost frame, if both the fixed
>> RA offset (from SFrame header) and the RA offset (from FDE) are zero,
>> without any need to test architecture-specific flags.
>>
>> include/
>>     * sframe.h (SFRAME_FRE_RA_OFFSET_INVALID): New define.  Used as
>>     padding offset.
>>     * sframe-api.h (sframe_fre_get_ra_offset): Add comment that for
>>     s390x an offset value of SFRAME_FRE_RA_OFFSET_INVALID indicates
>>     that the RA is not saved.
>>
>> gas/
>>     * gen-sframe.c (get_fre_num_offsets): For s390x account padding
>>     RA offset, if FP without RA saved.
>>     (sframe_get_fre_offset_size): Likewise.
>>     (output_sframe_row_entry): For s390x write a padding RA offset,
>>     if FP without RA needs to be represented.
>>     (sframe_do_fde): Enable FP without RA saved to be represented
>>     on s390x.
>>
>> libsframe/
>>     * sframe.c (sframe_fre_get_ra_offset): Add comment that for
>>     s390x an offset value of SFRAME_FRE_RA_OFFSET_INVALID indicates
>>     that the RA is not saved.
>>     * sframe-dump.c (dump_sframe_func_with_fres): Treat invalid
>>     RA offsets as if they were undefined.  Display them as "U"
>>     to distinguish them.
>>     * doc/sframe-spec.texi (s390x): Document s390x-specific use of
>>     SFRAME_FRE_RA_OFFSET_INVALID to represent FP without RA saved.
>>
>> gas/testsuite/
>>     * gas/cfi-sframe/cfi-sframe.exp: Rename s390x-specific tests.
>>     * gas/cfi-sframe/cfi-sframe-s390x-fpra-offset-err-1.s: Rename
>>     to ...
>>     * cfi-sframe/cfi-sframe-s390x-fpra-offset-err-1.d: Likewise.
>>     * gas/cfi-sframe/cfi-sframe-s390x-fpra-offset-2.s: This.
>>     * gas/cfi-sframe/cfi-sframe-s390x-fpra-offset-2.d: Likewise.
>>     Update test verification pattern accordingly.
>>     * cfi-sframe/cfi-sframe-s390x-fpra-register-err-1.s: Rename
>>     to ...
>>     * cfi-sframe/cfi-sframe-s390x-fpra-register-err-1.d: Likewise.
>>     * gas/cfi-sframe/cfi-sframe-s390x-fpra-register-2.s: This.
>>     * gas/cfi-sframe/cfi-sframe-s390x-fpra-register-2.d: Likewise.
>>     Update test verification pattern accordingly.
>>
>> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> 
> One comment below for the spec changes in V3.
> 
> Otherwise, looks good to me.
> Reviewed-by: Indu Bhagat <indu.bhagat@oracle.com>

>> diff --git a/libsframe/doc/sframe-spec.texi b/libsframe/doc/sframe-spec.texi

>> @@ -141,6 +141,9 @@ DWARF register number.
>>   FP/RA offset.
>>   @item SFRAME_S390X_OFFSET_DECODE_REGNUM: Decode a DWARF register number from
>>   an FP/RA offset.
>> +@item SFRAME_FRE_RA_OFFSET_INVALID: Invalid RA offset value (like
>> +SFRAME_CFA_FIXED_RA_INVALID).  Used on s390x as padding offset to represent
>> +FP without RA saved.
>>   @end itemize
>>   @end itemize
>>   @@ -870,12 +873,13 @@ The (64-bit) s390x ELF ABI does not mandate the precise location in a function
>>   where the return address (RA) and frame pointer (FP) are saved, if at all.
>>   Hence the need to track RA in the SFrame stack trace format.  As RA is being
>>   tracked in this ABI, the second stack offset is always used to locate the RA
>> -stack slot, by interpreting it as: RA = CFA + offset2.  RA remains unchanged,
>> -if the offset is not available.  It is recommended that a stack tracer
>> -implements a check to ensure that RA remains unchanged only for the topmost
>> -frame only.  The third stack offset is used to locate the FP stack slot, by

Used your below suggestion in patch 03/11 "s390: Initial support to
generate .sframe from CFI directives in assembler" to reword the above.

>> -interpreting it as: FP = CFA + offset3.  FP remains unchanged, if the offset is
>> -not available.
>> +stack slot, by interpreting it as: RA = CFA + offset2, unless the offset has a
>> +value of @code{SFRAME_FRE_RA_OFFSET_INVALID}.  RA remains unchanged, if the
>> +offset is not available or has a value of @code{SFRAME_FRE_RA_OFFSET_INVALID}.
>> +It is recommended that a stack tracer implements a check to ensure that RA
>> +remains unchanged only for the topmost frame only.  The third stack offset is
> 
> Two 'only' in one sentence look off.  Remove one.  Or if you find the
> following reads better:
> 
> Stack tracers are recommended to validate that the "unchanged RA"
> pattern, when present, is seen only for the topmost stack frame.

Ok.

>> +used to locate the FP stack slot, by interpreting it as: FP = CFA + offset3.
>> +FP remains unchanged, if the offset is not available.
>>     In leaf functions the RA and FP may be saved in other registers, such as
>>   floating-point registers (FPRs), instead of on the stack.  To represent this
>> @@ -899,6 +903,7 @@ Hence, in summary:
>>   @item 1 @tab CFA = @code{BASE_REG} + offset1
>>   @item 2 @tab RA stack slot = CFA + offset2, if (offset2 & 1 == 0)
>>              @*RA register number = offset2 >> 1, if (offset2 & 1 == 1)
>> +           @*RA not saved if (offset2 == @code{SFRAME_FRE_RA_OFFSET_INVALID})
>>   @item 3 @tab FP stack slot = CFA + offset3, if (offset3 & 1 == 0)
>>              @*FP register number = offset3 >> 1, if (offset3 & 1 == 1)
>>   @end multitable

Regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303)
+49-7031-16-1128 Office
jremus@de.ibm.com

IBM

IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/