[PATCH 3/8] s390: Initial support to generate .sframe from CFI directives in assembler
Jens Remus
jremus@linux.ibm.com
Mon Apr 28 08:38:37 GMT 2025
Hello Indu!
On 17.04.2025 11:40, Jens Remus wrote:
> On 16.04.2025 08:13, Indu Bhagat wrote:
>> On 4/2/25 9:11 AM, Jens Remus wrote:
>
>>> This introduces initial support to generate .sframe from CFI directives
>>> in assembler on 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.
>>>
>>> - 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.
>>> > - SFrame assumes a dedicated FP register number. The s390x ELF ABI [1]
>>> does only designate register 11 as preferred FP register number. In
>>> general GCC and Clang on s390x use register 11 as frame pointer.
>>> GCC on s390x can be observed to use register 14 as frame pointer in
>>> the stack clash protector in the function prologue.
>>
>> For stack clash protection, is the code sequence amenable to usage of
>> r11 instead ?
>
> That would be possible. But I understand that with SFrame V3 [4] there
> are plans to support non-SP/FP CFA base registers for topmost frames.
> This would then make a compiler change unnecessary.
>
> Note that the issue with the stack clash protector can only occur if all
> of the following conditions are met:
> - Compile with option -fstack-clash-protection
> - Function is non-leaf
> - Function allocates a lot of static stack space, so that the allocation
> loop in single page increments at a time is not unrolled
>
> [4]: SFrame V3 TODOs,
> https://sourceware.org/binutils/wiki/sframe/sframev3todo
>
>>
>>> glibc on s390x contains hand-written assembler code that uses
>>> register 12 as frame pointer.
>>>
>>> This s390x support is largely based on the AArch64 support from commit
>>> b52c4ee46657 ("gas: generate .sframe from CFI directives").
>>>
>>> The SFrame ABI/arch identifier SFRAME_ABI_S390_ENDIAN_BIG is introduced
>>> for s390/s390x and added to the SFrame format specification.
>>>
>>> The s390x ELF ABI [1] specifies the following C calling conventions for
>>> s390x architecture:
>>> - Register 15 is the stack pointer (SP).
>>> - Register 14 contains the return address (RA) at function entry.
>>> - There is no dedicated frame pointer register. Register 11 is the
>>> preferred frame pointer (FP). [2] GCC and Clang in general use
>>> register 11 as frame pointer.
>>
>> I guess saying that "Register 11 is the preferred frame pointer (FP)"
>> is somewhat confusing to me. Especially when I read it in conjunction
>> with the above comment around code generated for stack clash protector
>> mode and...
>
> The s390x ELF ABI does not designate any specific register to be used as
> FP register. But GCC (except for the stack clash protector) and Clang
> do only use register 11 as FP (as far as observed). We cannot amend
> the ABI to retrospectively designate a specific register as FP register.
> But we did amend it to say that register 11 is the preferred one, which
> happens to be the one commonly used by GCC and Clang, indicating that
> this should be used in preference over any other choices.
>
>>
>>> - The CFA is defined as SP at call site +160. [3] The SP at call site
>>> can therefore be derived from the CFA using a SP value offset from CFA
>>> of -160.
>>>
>>> The s390x ELF ABI [1] does not assign any standard save slot to each
>>> register in the register save area of a stack frame. Neither the
>>> return address (RA, r14) nor preferred frame pointer (FP, r11)
>>> necessarily need to be saved. Therefore SFrame RA and FP tracking is
>>> used.
>>>
>>> Support for SFrame is only enabled for z/Architecture with 64-bit
>>> addressing mode (s390x). It is disabled for z/Architecture with 32-bit
>>> addressing mode and ESA/390 (s390).
>>>
>>> s390x-specific SFrame assembler and linker tests are added, including
>>> error tests for use of a non-preferred frame pointer (FP) register and
>>> specification of a non-default return address (RA) register.
>>>
>>> [1]: s390x ELF ABI, https://github.com/IBM/s390x-abi/releases
>>> [2]: s390x ELF ABI, commit f00421825979 ("Add information about the frame
>>> pointer register"),
>>> https://github.com/IBM/s390x-abi/commit/f00421825979
>>
>> ...this.
>>
>> This commit suggests r11 as "Commonly used as frame pointer", but at
>> the same time also adds "If a frame pointer is maintained, it should
>> be stored in \texttt{r11}".
>>
>> The latter seems to suggest that r11 is now _the_ frame pointer
>> register, not just "Commonly used as frame pointer" or the "preferred
>> frame pointer". I find the language shifting in terms of the implied
>> strictness, and hence, confusing.
>
> I think "should" needs to be interpreted in RFC 2119 [5] terminology:
>
>> SHOULD This word, or the adjective "RECOMMENDED", mean that there
>> may exist valid reasons in particular circumstances to ignore a
>> particular item, but the full implications must be understood and
>> carefully weighed before choosing a different course.
>
> [5]: RFC 2119: Key words for use in RFCs to Indicate Requirement Levels,
> https://datatracker.ietf.org/doc/html/rfc2119
>
> Adding Andreas A. as one of the s390x ELF ABI authors.
>
>>
>> IOW, we do want r11 to be the designated frame pointer, so that code
>> generated by conforming compiler also works well for stack tracing
>> using SFrame.
>
> So far we refrained from adding an appendix "SFrame considerations" (or
> similar) to the s390x ELF ABI, which could specify that register 11 must
> be used for SFrame compatibility.
>
>>> [3]: s390x ELF ABI, commit 4e38ad9c8a88 ("Document the CFA"),
>>> https://github.com/IBM/s390x-abi/commit/4e38ad9c8a88
>>>
>>> include/
>>> * sframe.h: Add reference to s390x architecture in comments.
>>> (FRAME_ABI_S390_ENDIAN_BIG): Define SFrame ABI/arch identifier
>>
>> Typo: SFRAME_ABI_S390_ENDIAN_BIG
>
> Fixed.
>
>>
>>> for s390x.
>>> (SFRAME_S390_SP_VAL_OFFSET): Define s390x-specific SP value
>>> offset from CFA.
>
>>> gas/
>>> * config/tc-s390.h: s390x support to generate .sframe from CFI
>>> directives in assembler.
>>> (support_sframe_p): Define.
>>> (SFRAME_CFA_SP_REG, SFRAME_CFA_FP_REG, SFRAME_CFA_RA_REG):
>>> Define.
>>> (sframe_ra_tracking_p): Define.
>>> (sframe_cfa_ra_offset): Define.
>>> (sframe_get_abi_arch): Define.
>>> * config/tc-s390.c: s390x support to generate .sframe from CFI
>>> directives in assembler.
>>> (s390_sframe_cfa_sp_reg, s390_sframe_cfa_fp_reg,
>>> s390_sframe_cfa_ra_reg): New. Initialize to DWARF register
>>> numbers of stack pointer (SP, r15), preferred frame pointer
>>> (FP, r11), and return address (RA, r14) registers.
>>> (s390_support_sframe_p): New function. Return true if s390x.
>>> (s390_sframe_ra_tracking_p): New function. Return true.
>>> (s390_sframe_cfa_ra_offset): New function. Return
>>> SFRAME_CFA_FIXED_RA_INVALID.
>>> (s390_sframe_get_abi_arch): New function. Return
>>> SFRAME_ABI_S390_ENDIAN_BIG if s390x, otherwise zero.
>>> * gen-sframe.c: Add reference to s390x architecture in comments.
>>> (sframe_xlate_do_val_offset): Add support for s390x-specific
>>> SFRAME_S390_SP_VAL_OFFSET.
>>> * NEWS: Add news entry.
>>>
>>
>> I think adding an entry in binutils/NEWS is also a good idea to
>> announce SFrame for s390x in general.
>>
>> Perhaps as another patch by itself if you like.
>
> Good idea! I have added a separate patch. Not sure whether the update
> to gas/NEWS should also be moved to the end of the series, so that it
> no longer needs to state "initial support".
>
>>> diff --git a/gas/config/tc-s390.c b/gas/config/tc-s390.c
>>> index a0cfeead2535..91593b2e8503 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. */
>>> +
>>> +/* Stack-pointer DWARF register number according to s390x ELF ABI. */
>>> +unsigned int s390_sframe_cfa_sp_reg = 15;
>>> +
>>> +/* Frame-pointer DWARF register number accoring to s390x GCC/LLVM convention. */
>>
>> I was going to say - Not just convention anymore. Its as per the ABI.
>>
>> But I am not sure anymore, given the fuzzy strictness level in the
>> language used in commit log and the ABI commit.
>>
>>> +unsigned int s390_sframe_cfa_fp_reg = 11;
>>> +
>>> +/* Return-address DWARF register number according to s390x ELF ABI. */
>>> +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 */
>
> Changed as follows for the next version of the series:
>
> diff --git a/gas/config/tc-s390.c b/gas/config/tc-s390.c
> @@ -101,13 +101,13 @@ int s390_cie_data_alignment;
>
> /* Register numbers used for SFrame stack trace info. */
>
> -/* Stack-pointer DWARF register number according to s390x ELF ABI. */
> +/* Designated stack pointer DWARF register number according to s390x ELF ABI. */
> unsigned int s390_sframe_cfa_sp_reg = 15;
>
> -/* Frame-pointer DWARF register number accoring to s390x GCC/LLVM convention. */
> +/* Preferred frame pointer DWARF register number according to s390x ELF ABI. */
> unsigned int s390_sframe_cfa_fp_reg = 11;
>
> -/* Return-address DWARF register number according to s390x ELF ABI. */
> +/* Designated return-address DWARF register number according to s390x ELF ABI. */
> unsigned int s390_sframe_cfa_ra_reg = DWARF2_DEFAULT_RETURN_COLUMN;
>
>>> @@ -2867,6 +2880,48 @@ tc_s390_regname_to_dw2regnum (char *regname)
>>> return regnum;
>>> }
>>> +/* Whether SFrame stack trace info is supported. */
>>> +
>>> +bool
>>> +s390_support_sframe_p (void)
>>> +{
>>> + /* At this time, SFrame is supported for s390x (64-bit) only. */
>>> + return (s390_arch_size == 64);
>>> +}
>>> +
>>> +/* Specify if RA tracking is needed. */
>>> +
>>> +bool
>>> +s390_sframe_ra_tracking_p (void)
>>> +{
>>> + return true;
>>> +}
>>> +
>>> +/* Specify the fixed offset to recover RA from CFA.
>>> + (useful only when RA tracking is not needed). */
>>> +
>>> +offsetT
>>> +s390_sframe_cfa_ra_offset (void)
>>> +{
>>> + return (offsetT) SFRAME_CFA_FIXED_RA_INVALID;
>>> +}
>>> +
>>> +/* Get the abi/arch indentifier for SFrame. */
>>
>> Typo: identifier
>
> Fixed both occurrences. I will also send a patch for x86 and AArch64.
>
>>
>>> +
>>> +unsigned char
>>> +s390_sframe_get_abi_arch (void)
>
>>> diff --git a/gas/config/tc-s390.h b/gas/config/tc-s390.h
>
>>> +/* The abi/arch indentifier for SFrame. */
>>
>> Typo: identifier
>
> Fixed (see above).
>
>>> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-1.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-1.d
>>> new file mode 100644
>
>>> @@ -0,0 +1,23 @@
>>> +#objdump: --sframe=.sframe
>>> +#name: SFrame generation on s390
>>
>> Using the string "s390x" will be more appropriate IMO. For all the
>> applicable tests.
>
> I tend to agree, as these tests are specific to SFrame support for the
> s390x ELF ABI. It might then also make sense to rename the tests file
> names. I need to discuss with Andreas K., our Binutils s390 maintainer,
> whether and where I should use s390 or s390x or how we could reword it
> otherwise.
>
>>> diff --git a/include/sframe.h b/include/sframe.h
>
>>> @@ -93,6 +93,7 @@ extern "C"
>>> #define SFRAME_ABI_AARCH64_ENDIAN_BIG 1 /* AARCH64 big endian. */
>>> #define SFRAME_ABI_AARCH64_ENDIAN_LITTLE 2 /* AARCH64 little endian. */
>>> #define SFRAME_ABI_AMD64_ENDIAN_LITTLE 3 /* AMD64 little endian. */
>>> +#define SFRAME_ABI_S390_ENDIAN_BIG 4 /* S390 big endian. */
>>
>> I think being more precise and using "S390X" is better. What is the
>> reason to say S390 instead of S390X ?
>
> The term "s390" is commonly used to distinguish our architecture from
> the others, such as x86 and Power. More specifically, "s390" refers to
> our 32-bit ELF ABI, while "s390x" refers to our 64-bit ELF ABI.
>
> My thought was that "S390" in SFRAME_ABI_S390_ENDIAN_BIG identifies our
> architecture and that the 32-bit s390 or 64-bit s390x ELF ABI could be
> distinguished by the ELF Identification EI_CLASS as follows:
>
> - 32-bit s390 ABI : SFRAME_ABI_S390_ENDIAN_BIG with ELFCLASS32
> - 64-bit s390x ABI: SFRAME_ABI_S390_ENDIAN_BIG with ELFCLASS64
>
> That is also reflected in s390_support_sframe_p:
>
> bool
> s390_support_sframe_p (void)
> {
> /* At this time, SFrame is supported for s390x (64-bit) only. */
> return (s390_arch_size == 64);
> }
>
>>> /* SFrame FRE types. */
>>> #define SFRAME_FRE_TYPE_ADDR1 0
>>> @@ -190,7 +191,7 @@ typedef struct sframe_func_desc_entry
>>> - 2-bits: Unused.
>>> ------------------------------------------------------------------------
>>> | Unused | PAC auth A/B key (aarch64) | FDE type | FRE type |
>>> - | | Unused (amd64) | | |
>>> + | | Unused (amd64, s390) | | |
>>> ------------------------------------------------------------------------
>>> 8 6 5 4 0 */
>>> uint8_t sfde_func_info;
>>> @@ -248,7 +249,7 @@ typedef struct sframe_fre_info
>>> - 1 bit: Mangled RA state bit (aarch64 only).
>>> ----------------------------------------------------------------------------------
>>> | Mangled-RA (aarch64) | Size of offsets | Number of offsets | base_reg |
>>> - | Unused (amd64) | | | |
>>> + | Unused (amd64, s390) | | | |
>>> ----------------------------------------------------------------------------------
>>> 8 7 5 1 0
>>> @@ -274,7 +275,7 @@ typedef struct sframe_fre_info
>>> /* SFrame Frame Row Entry definitions.
>>> - Used for both AMD64 and AARCH64.
>>> + Used for AMD64, AARCH64, and S390.
>>
>> "s390x" ?
>>
>>> An SFrame Frame Row Entry is a self-sufficient record which contains
>>> information on how to generate the stack trace for the specified range of
>>> @@ -342,6 +343,10 @@ typedef struct sframe_frame_row_entry_addr4
>>> #define SFRAME_FRE_TYPE_ADDR4_LIMIT \
>>> (1ULL << ((SFRAME_FRE_TYPE_ADDR4 * 2) * 8))
>>> +/* On S390, the CFA is defined as SP at call site + 160. Therefore the
>>> + SP value offset from CFA is -160. */
>>> +#define SFRAME_S390_SP_VAL_OFFSET -160
>>> +
>>
>> I think its better to use s390x rather than s390 as these are two
>> different ABIs. SFrame support is for s390x only.
>
> I'll check with Andreas and get back with a v2.
>
>> Is there any plan to later add support for s390 too ?
>
> No. We expect support for 32-bit (s390 ELF ABI) to be deprecated. The
> mayor Linux distributions are only available in 64-bit (s390x ELF ABI)
> and they have begun phasing out support for 32-bit on s390x (e.g. no
> libgcc nor glibc 32-bit compat packages or even no 32-bit syscalls).
>
>>> diff --git 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 s390.
>>
>> s390x?
>
> Technically this patch only implements SFrame support for the "s390x ELF
> ABI", which also has "s390x" in its name, so that would be correct.
>
> I guess need to revisit every occurrence of s390/s390x (case insensitive)
> and come to an agreement with Andreas K. how to word it.
>
> Partially this may depend on whether you consider the introduction of
> SFRAME_ABI_S390_ENDIAN_BIG for our common architecture and therefore
> both s390 and s390x ELF ABIs acceptable.
I checked with Andreas K. and we would like to propose the following
given the following background:
The term "s390" is commonly used to distinguish our architecture from
the others, such as x86 and Power. More specifically, "s390" refers to
our 32-bit ELF ABI, while "s390x" refers to our 64-bit ELF ABI. We
expect support for 32-bit (s390 ELF ABI) to be deprecated. The mayor
Linux distributions are only available in 64-bit (s390x ELF ABI) and
they have begun phasing out support for 32-bit on s390x (e.g. no libgcc
nor glibc 32-bit compat packages or even no 32-bit compat syscalls).
As the 32-bit s390 ABI will certainly have no relevance in the future
and the term "s390x" is not widely used in Binutils, we would prefer to
use "s390" in names of macros, functions, and tests (incl. file names).
In the SFrame format specification we would use S390, with a note that
only the 64-bit s390x ELF ABI is implemented. One exception would be
the linker, where "s390x" is already (unneccessarily) used for the PLT
entries. We may cleanup this once the 32-bit support would be
deprecated.
Would it then also be fine with you to use FRAME_ABI_S390_ENDIAN_BIG as
SFrame ABI/architecture identifier? The ABI would technically be
identifyable by the ELF class (32-bit vs. 64-bit), although we do not
expect the 32-bit s390 ELF ABI to get ever implemented.
#define SFRAME_ABI_S390_ENDIAN_BIG 4 /* S390 64-bit big endian. */
Please let me know what you think, so that I can move forward with
making the appropriate changes and send a V2 for review.
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