[PATCH v2 03/11] s390: Initial support to generate .sframe from CFI directives in assembler
Jens Remus
jremus@linux.ibm.com
Tue Jun 3 13:11:10 GMT 2025
Hello Indu,
thank you for the review feedback!
On 02.06.2025 22:35, Indu Bhagat via Binutils wrote:
> On 5/27/25 4:07 AM, Jens Remus wrote:
>> This introduces initial support to generate .sframe from CFI directives
>> in assembler on s390 64-bit (s390x). Due to SFrame V2 format
>> limitations it has the following limitations, some of them getting
>> addressed by subsequent patches, which cause generation of SFrame FDE
>> to be skipped:
>>
>> - SFrame FP/RA tracking only supports register contents being saved on
>> the stack (i.e. .cfi_offset). It does not support FP/RA register
>> contents being saved in other registers (i.e. .cfi_register). GCC on
>> s390x can be observed to save the FP/RA register contents in floating-
>> point registers, but only in leaf functions.
>>
>
> Nit: it may be worth adding a statement here, something that says that
> a subsequent commit addresses this issue. I see you mention that in
> the cover letter, but mentioning here in the git commit log will help
> establish the connection for posterity.
>
> This issue is detailed further and resolved in a later commit [s390:
> Represent FP/RA saved in register in SFrame].
Makes sense.
>
>> - SFrame FP/RA tracking cannot represent FP without RA saved. This is
>> because the format assumes SFrame FDE offset2 to be the RA offset, if
>> there are two offsets, and offset3 to be the FP offset, if there are
>> three offsets. There is no mean to distinguish whether offset2 is the
>> RA or FP offset, if there are only two offsets.
>>
>
> Similarly:
>
> This issue is detailed further and resolved in a later commit [s390:
> Represent FP without RA saved in SFrame].
Sure.
>> diff --git a/gas/config/tc-s390.c b/gas/config/tc-s390.c
>> index b84613479725..cf863f7a538f 100644
>> --- a/gas/config/tc-s390.c
>> +++ b/gas/config/tc-s390.c
>> @@ -24,6 +24,8 @@
>> #include "subsegs.h"
>> #include "dwarf2dbg.h"
>> #include "dw2gencfi.h"
>> +#include "sframe.h"
>> +#include "gen-sframe.h"
>> #include "opcode/s390.h"
>> #include "elf/s390.h"
>> @@ -97,6 +99,17 @@ const char FLT_CHARS[] = "dD";
>> /* The dwarf2 data alignment, adjusted for 32 or 64 bit. */
>> int s390_cie_data_alignment;
>> +/* Register numbers used for SFrame stack trace info. */
>> +
>> +/* Designated stack pointer DWARF register number according to s390x ELF ABI. */
>> +const unsigned int s390_sframe_cfa_sp_reg = 15;
>> +
>> +/* Preferred frame pointer DWARF register number according to s390x ELF ABI. */
>> +const unsigned int s390_sframe_cfa_fp_reg = 11;
>> +
>> +/* Designated return-address DWARF register number according to s390x ELF ABI. */
>
> Nit: "return address" ? (without the -)
Ok. I'll grep for the dash-variants to see if I missed to change any
further.
>
>> +const unsigned int s390_sframe_cfa_ra_reg = DWARF2_DEFAULT_RETURN_COLUMN;
>> +
>> /* The target specific pseudo-ops which we support. */
>> /* Define the prototypes for the pseudo-ops */
>> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390x-1.s b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390x-1.s
>> new file mode 100644
>> index 000000000000..e5c69b9813c4
>> --- /dev/null
>> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390x-1.s
>> @@ -0,0 +1,37 @@
>> + .cfi_sections .sframe
>> + .cfi_startproc
>> + stmg %r6,%r15,48(%r15)
>> + .cfi_offset 6, -112
>> + .cfi_offset 7, -104
>> + .cfi_offset 8, -96
>> + .cfi_offset 9, -88
>> + .cfi_offset 10, -80
>> + .cfi_offset 11, -72
>> + .cfi_offset 12, -64
>> + .cfi_offset 13, -56
>> + .cfi_offset 14, -48
>> + .cfi_offset 15, -40
>> + lay %r15,-160(%r15)
>> + .cfi_def_cfa_offset 320
>> + lgr %r11,%r15
>> + .cfi_def_cfa_register 11
>> + lay %r15,-128(%r15)
>> +.Lreturn:
>> + lmg %r6,%r15,160+48(%r11)
>> + .cfi_remember_state
>> + .cfi_restore 15
>> + .cfi_restore 14
>> + .cfi_restore 13
>> + .cfi_restore 12
>> + .cfi_restore 11
>> + .cfi_restore 10
>> + .cfi_restore 9
>> + .cfi_restore 8
>> + .cfi_restore 7
>> + .cfi_restore 6
>> + .cfi_def_cfa 15, 160
>> + br %r14
>> + .cfi_restore_state
>> + lay %r15,-128(%r15)
>
> Nit: space separator between lay and %r15, instead of tab in the rest of this file. This repeats as a pattern in the rest of the tests too.
Good catch! Will fix in all tests.
>
>> + j .Lreturn
>> + .cfi_endproc
>> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp b/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
>> index 341a56a9eab2..df48cd143fd6 100644
>> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
>> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
>> @@ -64,7 +64,8 @@ proc gas_x86_64_check { } {
>> }
>> # common tests
>> -if { ([istarget "x86_64-*-*"] || [istarget "aarch64*-*-*"]) \
>> +if { ([istarget "x86_64-*-*"] || [istarget "aarch64*-*-*"] ||
>
> Nit: Follow the "No Trailing operator" rule here too?
Ok.
>
>> + [istarget "s390x-*-*"]) \
>> && [gas_sframe_check] } then {
>> global ASFLAGS
>> diff --git a/libsframe/doc/sframe-spec.texi b/libsframe/doc/sframe-spec.texi
>> index ae115705ca73..62040c3fa64f 100644
>> --- a/libsframe/doc/sframe-spec.texi
>> +++ b/libsframe/doc/sframe-spec.texi
>> @@ -79,8 +79,8 @@ The SFrame stack trace information is provided in a loaded section, known as the
>> @code{.sframe} section. When available, the @code{.sframe} section appears in
>> a new segment of its own, PT_GNU_SFRAME.
>> -The SFrame format is currently supported only for select ABIs, namely, AMD64
>> -and AAPCS64.
>> +The SFrame format is currently supported only for select ABIs, namely, AMD64,
>> +AAPCS64, and s390x.
>>
>
> Sorry, but I just realised this recently: I would also recommend you
> put an entry in the section "Changes from Version 1 to Version 2" to
> add s390x support to the specification.
>
> Something like:
>
> Add a new ABI/arch identifier SFRAME_ABI_S390X_ENDIAN_BIG. Other
> backward compatible changes including helper definitions related to
> CFA offset adjustment on s390x, and encoding and decoding of FP/RA
> save in register on s390x have been added incrementally to SFrame
> version 2 only.
>
> I leave this to your best judgement on whether to include this now or
> later as a separate commit after the series.
Sure, makes sense, will do.
>> +@section s390x
>> +
>> +A stack tracer implementation must initialize the SP to the designated SP
>> +register value, the FP to the preferred FP register value, and the RA to the
>> +designated RA register value in the topmost stack frame of the callchain. This
>> +is required, as either the SP or FP is used as CFA base register and as the FP
>> +and/or RA are not necessarily saved on the stack. For RA this may only be the
>> +case in the topmost stack frame of the callchain. For FP this may be the case
>> +in any stack frame.
>> +
>> +Irrespective of the ABI, the first stack offset is always used to locate the
>> +CFA, by interpreting it as: CFA = @code{BASE_REG} + offset1.
>> +The identification of the @code{BASE_REG} is done by using the
>> +@code{fre_cfa_base_reg_id} field in the SFrame FRE info word.
>> +
>> +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. The third stack offset is used to locate the
>> +FP stack slot, by interpreting it as: FP = CFA + offset3. FP remains unchanged,
>> +if the offset is not available.
>> +It is recommended that a stack tracer implementation performs the required
>> +checks to ensure that RA remains unchanged only for the topmost stack frame
>> +in the callchain.
>> +
>
> I find the last statement difficult to follow. In light of the patches
> 04/11 and 05/11, which allow RA to be saved in say FPR, or RA to be
> not saved on stack respectively, I am not sure I fully grasp the scope
> of the statement "RA remains unchanged only for the topmost stack
> frame in the callchain".
Let me try to explain what I intended to convey. Maybe you can help me
get the wording right.
A stack tracer using SFrame first initializes:
SP = designated SP register content
FP = designated (s390x: preferred) SP register content
RA = designated RA register content, if arch uses RA tracking
Then for each frame it uses the SFrame CFA, FP, and RA tracking info to
determine:
CFA = CFA_base_reg + CFA_offset
SP = CFA [+ SP_val_offset on s390x]
if FP_offset != 0:
FP = restore FP from stack at CFA + FP_offset (or register on s390x)
if RA_offset == 0 && !topmost:
error
if RA_offset != 0:
RA = restore RA from stack at CFA + RA_offset (or register on s390x)
Whether RA may be unchanged (not saved on the stack nor in another
register) is only valid in the topmost frame. For the FP that may
be the case in for any frame, as a frame is not forced to modify
the FP. Saved in another register is also only valid in the topmost
frame, as the stack tracer only then has access to all register values.
>
>> +Given the nature of things, the number of stack offsets seen on s390x per
>> +SFrame FRE is either 1, 2, or 3.
>> +
>> +Hence, in summary:
>> +
>> +@multitable {Offset ID} {Interpretation in s390x in X}
>> +@headitem Offset ID @tab Interpretation in s390x
>> +@item 1 @tab CFA = @code{BASE_REG} + offset1
>> +@item 2 @tab RA = CFA + offset2
>> +@item 3 @tab FP = CFA + offset3
>> +@end multitable
>> +
>> +The s390x ELF ABI defines the CFA as stack pointer (SP) at call site +160. The
>> +SP can therefore be obtained using the SP value offset from CFA
>> +@code{SFRAME_S390X_SP_VAL_OFFSET} of -160 as follows:
>> +SP = CFA + @code{SFRAME_S390X_SP_VAL_OFFSET}
Thanks and 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/
More information about the Binutils
mailing list