[4/8] s390: Represent FP/RA saved in register in SFrame

  GCC on s390x, when in a leaf function, can be observed to save the
frame pointer (FP) and/or return address (RA) register in a floating-
point registers (FPR) instead of on the stack.  This is declared using
the following CFI directive:

  .cfi_register <fp/ra-regnum>, <fpr-regnum>

SFrame cannot represent the FP and/or RA being saved in another
register.  It does only track the CFA base register (SP/FP), CFA offset
from CFA base register, and FP and RA save area offsets from CFA.

On s390x the FP and/or RA are only saved in another FPR when in a leaf
function.  That is a function that does not call any other function.
Therefore it can ever only be the topmost function in a call chain.
An unwinder by default has access to all registers of the function that
is the topmost on the call stack.  Therefore no further information
is required to restore FP/RA from the FPR.

Represent FP/RA saved in another register on s390, by encoding the
DWARF register number shifted by one to the left with the least-
significant bit set in the offset as follows:

  offset = (regnum << 1) | 1

The use of the least-significant bit of the offset as indication is
possible, as the stack pointer (SP), the CFA, and any register save
area slots are 8-byte aligned according to the s390x ELF ABI:
- The stack pointer (SP) "shall maintain an 8-byte alignment". [1]
- The CFA is defined as SP at call site +160. [2]
- Pointers and 8-byte integers, such as general register values, must
  be 8-byte aligned. [3]
SFrame FP and RA stack offsets must therefore always be a multiple of
8 on s390x.  Note that for the same reason the DWARF data alignment
factor is -8 on s390x (see DWARF2_CIE_DATA_ALIGNMENT).

Add s390x-specific SFrame (error) tests for FP/RA saved in FPRs in leaf
function.

[1]: s390x ELF ABI, sections "Register Roles" and "Stack Frame
     Allocation", https://github.com/IBM/s390x-abi/releases
[2]: s390x ELF ABI, commit 4e38ad9c8a88 ("Document the CFA"),
     https://github.com/IBM/s390x-abi/commit/4e38ad9c8a88
[3]: s390x ELF ABI, section "Fundamental Types", table "Scalar types",
     https://github.com/IBM/s390x-abi/releases

include/
	* sframe.h (SFRAME_S390_OFFSET_IS_REGNUM): New S390-specific
	macro to test whether an SFrame FP/RA offset is a DWARF register
	number.
	(SFRAME_S390_OFFSET_ENCODE_REGNUM): New S390-specific macro to
	encode a DWARF register number into an SFrame FP/RA offset.
	(SFRAME_S390_OFFSET_DECODE_REGNUM): New S390-specific macro to
	decode an SFrame FP/RA offset into a DWARF register number.

gas/
	* gen-sframe.c (s390_sframe_xlate_do_register): New S390-
	specific function.  Uses SFRAME_S390_OFFSET_ENCODE_REGNUM to
	represent FP/RA saved in another register on S390.
	(sframe_xlate_do_register): Invoke s390_sframe_xlate_do_register
	on S390.

libsframe/
	* sframe-dump.c (is_sframe_abi_arch_s390): New helper to test
	whether ABI/arch is S390.
	(dump_sframe_func_with_fres): Use SFRAME_S390_OFFSET_IS_REGNUM
	and SFRAME_S390_OFFSET_DECODE_REGNUM to dump FP/RA saved in
	another register on S390.
	* doc/sframe-spec.texi (S390): Document S390-specific
	representation of FP/RA saved in another register.

gas/testsuite/
	* gas/cfi-sframe/cfi-sframe.exp: Update s390x-specific SFrame
	(error) tests.
	* gas/cfi-sframe/cfi-sframe-s390-fpra-register-err-2.s: Rename
	to ...
	* gas/cfi-sframe/cfi-sframe-s390-fpra-register-err-2.d:
	Likewise.
	* gas/cfi-sframe/cfi-sframe-s390-fpra-register-1.s: This.  Test
	no longer triggers a warning, as SFrame can represent FP and RA
	saved in registers.
	* gas/cfi-sframe/cfi-sframe-s390-fpra-register-1.d: Likewise.
	* gas/cfi-sframe/cfi-sframe-s390-fpra-register-err-1.d: Test now
	triggers a different warning, as SFrame can represent FP and RA
	saved in registers, but not FP without RA saved in register.

Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
 gas/gen-sframe.c                              | 37 +++++++++++++++++++
 .../cfi-sframe-s390-fpra-register-1.d         | 22 +++++++++++
 ...-2.s => cfi-sframe-s390-fpra-register-1.s} |  0
 .../cfi-sframe-s390-fpra-register-err-1.d     |  4 +-
 .../cfi-sframe-s390-fpra-register-err-2.d     | 15 --------
 gas/testsuite/gas/cfi-sframe/cfi-sframe.exp   |  2 +-
 include/sframe.h                              | 13 +++++++
 libsframe/doc/sframe-spec.texi                | 21 ++++++++---
 libsframe/sframe-dump.c                       | 24 +++++++++++-
 9 files changed, 113 insertions(+), 25 deletions(-)
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-fpra-register-1.d
 rename gas/testsuite/gas/cfi-sframe/{cfi-sframe-s390-fpra-register-err-2.s => cfi-sframe-s390-fpra-register-1.s} (100%)
 delete mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-fpra-register-err-2.d
  

On 4/2/25 9:11 AM, Jens Remus wrote:
> GCC on s390x, when in a leaf function, can be observed to save the
> frame pointer (FP) and/or return address (RA) register in a floating-
> point registers (FPR) instead of on the stack.  This is declared using
> the following CFI directive:
> 
>    .cfi_register <fp/ra-regnum>, <fpr-regnum>
> 
> SFrame cannot represent the FP and/or RA being saved in another
> register.  It does only track the CFA base register (SP/FP), CFA offset
> from CFA base register, and FP and RA save area offsets from CFA.
> 
> On s390x the FP and/or RA are only saved in another FPR when in a leaf
> function.  That is a function that does not call any other function.
> Therefore it can ever only be the topmost function in a call chain.
> An unwinder by default has access to all registers of the function that
> is the topmost on the call stack.  Therefore no further information
> is required to restore FP/RA from the FPR.
> 
> Represent FP/RA saved in another register on s390, by encoding the
> DWARF register number shifted by one to the left with the least-
> significant bit set in the offset as follows:
> 
>    offset = (regnum << 1) | 1
> 

I wonder if it makes sense to just save the three bits for any future 
use by doing this now:

     offset = (regnum << 3) | 1

I am still undecided.

> The use of the least-significant bit of the offset as indication is
> possible, as the stack pointer (SP), the CFA, and any register save
> area slots are 8-byte aligned according to the s390x ELF ABI:
> - The stack pointer (SP) "shall maintain an 8-byte alignment". [1]
> - The CFA is defined as SP at call site +160. [2]
> - Pointers and 8-byte integers, such as general register values, must
>    be 8-byte aligned. [3]
> SFrame FP and RA stack offsets must therefore always be a multiple of
> 8 on s390x.  Note that for the same reason the DWARF data alignment
> factor is -8 on s390x (see DWARF2_CIE_DATA_ALIGNMENT).
> 
> Add s390x-specific SFrame (error) tests for FP/RA saved in FPRs in leaf
> function.
> 
> [1]: s390x ELF ABI, sections "Register Roles" and "Stack Frame
>       Allocation", https://github.com/IBM/s390x-abi/releases
> [2]: s390x ELF ABI, commit 4e38ad9c8a88 ("Document the CFA"),
>       https://github.com/IBM/s390x-abi/commit/4e38ad9c8a88
> [3]: s390x ELF ABI, section "Fundamental Types", table "Scalar types",
>       https://github.com/IBM/s390x-abi/releases
> 
> include/
> 	* sframe.h (SFRAME_S390_OFFSET_IS_REGNUM): New S390-specific
> 	macro to test whether an SFrame FP/RA offset is a DWARF register
> 	number.
> 	(SFRAME_S390_OFFSET_ENCODE_REGNUM): New S390-specific macro to
> 	encode a DWARF register number into an SFrame FP/RA offset.
> 	(SFRAME_S390_OFFSET_DECODE_REGNUM): New S390-specific macro to
> 	decode an SFrame FP/RA offset into a DWARF register number.
> 
> gas/
> 	* gen-sframe.c (s390_sframe_xlate_do_register): New S390-
> 	specific function.  Uses SFRAME_S390_OFFSET_ENCODE_REGNUM to
> 	represent FP/RA saved in another register on S390.
> 	(sframe_xlate_do_register): Invoke s390_sframe_xlate_do_register
> 	on S390.
> 
> libsframe/
> 	* sframe-dump.c (is_sframe_abi_arch_s390): New helper to test
> 	whether ABI/arch is S390.
> 	(dump_sframe_func_with_fres): Use SFRAME_S390_OFFSET_IS_REGNUM
> 	and SFRAME_S390_OFFSET_DECODE_REGNUM to dump FP/RA saved in
> 	another register on S390.
> 	* doc/sframe-spec.texi (S390): Document S390-specific
> 	representation of FP/RA saved in another register.
> 

I did think for a bit: what sframe_fre_get_fp_offset () and 
sframe_fre_get_ra_offset () in libsframe/sframe.c ?

On more thought, I think its okay for these APIs to return the "raw 
data", and a stack tracer for s390x can do the right thing if the LSB is 
1 etc.  However, I have not yet looked at the stacktracer fixes (in the 
branch we shared sometime ago), to be able to comment if one way is 
clunker than other.  Thoughts ?

Perhaps we atleast add some comments, if you see appropriate, in the 
above APIs.  Something  along the lines that for s390x, for top most 
functions, this data may be for a save to register.  The API just 
returns the raw data in the offset.  Perhaps in the function-level 
comment if you like.  This is just to keep things clear as sometimes 
folks may refer to libsframe APIs when implementing their own SFrame 
based stack tracer.

> gas/testsuite/
> 	* gas/cfi-sframe/cfi-sframe.exp: Update s390x-specific SFrame
> 	(error) tests.
> 	* gas/cfi-sframe/cfi-sframe-s390-fpra-register-err-2.s: Rename
> 	to ...
> 	* gas/cfi-sframe/cfi-sframe-s390-fpra-register-err-2.d:
> 	Likewise.
> 	* gas/cfi-sframe/cfi-sframe-s390-fpra-register-1.s: This.  Test
> 	no longer triggers a warning, as SFrame can represent FP and RA
> 	saved in registers.
> 	* gas/cfi-sframe/cfi-sframe-s390-fpra-register-1.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-s390-fpra-register-err-1.d: Test now
> 	triggers a different warning, as SFrame can represent FP and RA
> 	saved in registers, but not FP without RA saved in register.
> 
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
>   gas/gen-sframe.c                              | 37 +++++++++++++++++++
>   .../cfi-sframe-s390-fpra-register-1.d         | 22 +++++++++++
>   ...-2.s => cfi-sframe-s390-fpra-register-1.s} |  0
>   .../cfi-sframe-s390-fpra-register-err-1.d     |  4 +-
>   .../cfi-sframe-s390-fpra-register-err-2.d     | 15 --------
>   gas/testsuite/gas/cfi-sframe/cfi-sframe.exp   |  2 +-
>   include/sframe.h                              | 13 +++++++
>   libsframe/doc/sframe-spec.texi                | 21 ++++++++---
>   libsframe/sframe-dump.c                       | 24 +++++++++++-
>   9 files changed, 113 insertions(+), 25 deletions(-)
>   create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-fpra-register-1.d
>   rename gas/testsuite/gas/cfi-sframe/{cfi-sframe-s390-fpra-register-err-2.s => cfi-sframe-s390-fpra-register-1.s} (100%)
>   delete mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-fpra-register-err-2.d
> 
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index 4d00c4be0a80..fb3a0342b1ee 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -1149,6 +1149,39 @@ sframe_xlate_do_val_offset (const struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_U
>     return SFRAME_XLATE_OK;
>   }
>   
> +/* S390-specific translate DW_CFA_register into SFrame context.
> +   Return SFRAME_XLATE_OK if success.  */
> +
> +static int
> +s390_sframe_xlate_do_register (struct sframe_xlate_ctx *xlate_ctx,
> +			       struct cfi_insn_data *cfi_insn)
> +{
> +  /* The scratchpad FRE currently being updated with each cfi_insn
> +     being interpreted.  This FRE eventually gets linked in into the
> +     list of FREs for the specific function.  */
> +  struct sframe_row_entry *cur_fre = xlate_ctx->cur_fre;
> +
> +  gas_assert (cur_fre);
> +
> +  /* Change the rule for the register indicated by the register number to
> +     be the specified register.  Encode the register number as offset by
> +     shifting it to the left by one and setting the least-significant bit
> +     (LSB).  The LSB can be used to differentiate offsets from register
> +     numbers, as offsets from CFA are always a multiple of -8 on s390x.  */
> +  if (cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
> +    sframe_fre_set_bp_track (cur_fre,
> +			     SFRAME_S390_OFFSET_ENCODE_REGNUM (cfi_insn->u.rr.reg2));
> +#ifdef SFRAME_FRE_RA_TRACKING
> +  else if (sframe_ra_tracking_p ()
> +	   && cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG)
> +    sframe_fre_set_ra_track (cur_fre,
> +			     SFRAME_S390_OFFSET_ENCODE_REGNUM (cfi_insn->u.rr.reg2));
> +#endif
> +
> +  /* Safe to skip.  */
> +  return SFRAME_XLATE_OK;
> +}
> +
>   /* Translate DW_CFA_register into SFrame context.
>      Return SFRAME_XLATE_OK if success.  */
>   
> @@ -1156,6 +1189,10 @@ static int
>   sframe_xlate_do_register (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>   			  struct cfi_insn_data *cfi_insn)
>   {
> +  /* Conditionally invoke S390-specific implementation.  */
> +  if (sframe_get_abi_arch () == SFRAME_ABI_S390_ENDIAN_BIG)
> +    return s390_sframe_xlate_do_register (xlate_ctx, cfi_insn);
> +
>     /* Previous value of register1 is register2.  However, if the specified
>        register1 is not interesting (FP or RA reg), the current DW_CFA_register
>        instruction can be safely skipped without sacrificing the asynchronicity of
> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-fpra-register-1.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-fpra-register-1.d
> new file mode 100644
> index 000000000000..736869834d8b
> --- /dev/null
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-fpra-register-1.d
> @@ -0,0 +1,22 @@
> +#objdump: --sframe=.sframe
> +#name: SFrame generation on s390 - FP and RA registers saved in FPR registers
> +#...
> +Contents of the SFrame section .sframe:
> +
> +  Header :
> +
> +    Version: SFRAME_VERSION_2
> +    Flags: NONE
> +    Num FDEs: 1
> +    Num FREs: 5
> +
> +  Function Index :
> +
> +    func idx \[0\]: pc = 0x0, size = 26 bytes
> +    STARTPC +CFA +FP +RA +
> +    0+0000 +sp\+160 +u +u +
> +    0+0004 +sp\+160 +u +r16 +
> +    0+0008 +sp\+160 +r17 +r16 +
> +    0+0014 +sp\+160 +u +r16 +
> +    0+0018 +sp\+160 +u +u +
> +#pass
> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-fpra-register-err-2.s b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-fpra-register-1.s
> similarity index 100%
> rename from gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-fpra-register-err-2.s
> rename to gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-fpra-register-1.s
> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-fpra-register-err-1.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-fpra-register-err-1.d
> index ca6f9df729d1..6baaf59271b1 100644
> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-fpra-register-err-1.d
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-fpra-register-err-1.d
> @@ -1,6 +1,6 @@
> -#name: SFrame generation on s390 - FP and RA saved in register
> +#name: SFrame generation on s390 - FP without RA saved in register
>   #as: --gsframe
> -#warning: skipping SFrame FDE; FP register 11 in .cfi_register
> +#warning: skipping SFrame FDE; FP without RA on stack
>   #objdump: --sframe=.sframe
>   #...
>   Contents of the SFrame section .sframe:
> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-fpra-register-err-2.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-fpra-register-err-2.d
> deleted file mode 100644
> index 670146c8eb14..000000000000
> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-fpra-register-err-2.d
> +++ /dev/null
> @@ -1,15 +0,0 @@
> -#name: SFrame generation on s390 - FP and RA saved in register
> -#as: --gsframe
> -#warning: skipping SFrame FDE; RA register 14 in .cfi_register
> -#objdump: --sframe=.sframe
> -#...
> -Contents of the SFrame section .sframe:
> -
> -  Header :
> -
> -    Version: SFRAME_VERSION_2
> -    Flags: NONE
> -    Num FDEs: 0
> -    Num FREs: 0
> -
> -#pass
> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp b/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
> index 04ffbde260aa..13b2e236bb46 100644
> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
> @@ -116,6 +116,6 @@ if { [istarget "s390x*-*-*"] && [gas_sframe_check] } then {
>       run_dump_test "cfi-sframe-s390-err-3"
>       run_dump_test "cfi-sframe-s390-fpra-offset-1"
>       run_dump_test "cfi-sframe-s390-fpra-offset-err-1"
> +    run_dump_test "cfi-sframe-s390-fpra-register-1"
>       run_dump_test "cfi-sframe-s390-fpra-register-err-1"
> -    run_dump_test "cfi-sframe-s390-fpra-register-err-2"
>   }
> diff --git a/include/sframe.h b/include/sframe.h
> index 684aab811f8d..a790f4078121 100644
> --- a/include/sframe.h
> +++ b/include/sframe.h
> @@ -347,6 +347,19 @@ typedef struct sframe_frame_row_entry_addr4
>      SP value offset from CFA is -160.  */
>   #define SFRAME_S390_SP_VAL_OFFSET		-160
>   
> +/* On S390, the FP and RA registers can be saved either on the stack or,
> +   in case of leaf functions, in registers.  Store DWARF register numbers
> +   encoded as offset by using the least-significant bit (LSB) as indicator:
> +   - LSB=0: Stack offset.  The s390x ELF ABI mandates that stack register
> +     slots must be 8-byte aligned.
> +   - LSB=1: DWARF register number shifted to the left by one.  */
> +#define SFRAME_S390_OFFSET_IS_REGNUM(offset) \
> +  ((offset) & 1)
> +#define SFRAME_S390_OFFSET_ENCODE_REGNUM(regnum) \
> +  (((regnum) << 1) | 1)
> +#define SFRAME_S390_OFFSET_DECODE_REGNUM(offset) \
> +  ((offset) >> 1)
> +
>   #ifdef	__cplusplus
>   }
>   #endif
> diff --git a/libsframe/doc/sframe-spec.texi b/libsframe/doc/sframe-spec.texi
> index eba497b4d371..5bd027828e3f 100644
> --- a/libsframe/doc/sframe-spec.texi
> +++ b/libsframe/doc/sframe-spec.texi
> @@ -848,16 +848,27 @@ tracked in this ABI, the second stack offset is always used to locate the RA
>   stack slot, by interpreting it as: RA = CFA + offset2.  The third stack offset
>   is used to locate the FP stack slot, by interpreting it as: FP = CFA + offset3.
>   
> -Given the nature of things, the number of stack offsets seen on S390 per
> -SFrame FRE is either 1, 2, or 3.
> +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
> +in the SFrame stack trace format the DWARF register number is encoded as
> +RA/FP offset using the least-significant bit (LSB) as indication:
> +offset = (regnum << 1) | 1.  A LSB of zero indicates a stack slot offset.
> +A LSB of one indicates a DWARF register number, which is interpreted as:
> +regnum = offset >> 1.  Given the nature of leaf functions, this can only occur
> +in the topmost frame during stack tracing.
> +

Can we also weave in something along the lines of:

It is recommended that a stack tracer implementation performs the 
required sanity checks to ensure that restoring FP and RA from the said 
register locations is done only for top-most stack frames in the callchain.

> +Given the nature of things, the number of stack offsets and/or register numbers
> +seen on S390 per SFrame FRE is either 1, 2, or 3.
>   
>   Hence, in summary:
>   
> -@multitable {Offset ID} {Interpretation in S390 in X}
> +@multitable @columnfractions .15 .85
>   @headitem Offset ID @tab Interpretation in S390
>   @item 1 @tab CFA = @code{BASE_REG} + offset1
> -@item 2 @tab RA = CFA + offset2
> -@item 3 @tab FP = CFA + offset3
> +@item 2 @tab RA stack slot = CFA + offset2, if (offset2 & 1 == 0)
> +           @*RA register number = offset2 >> 1, if (offset2 & 1 == 1)
> +@item 3 @tab FP stack slot = CFA + offset3, if (offset3 & 1 == 0)
> +           @*FP register number = offset3 >> 1, if (offset3 & 1 == 1)
>   @end multitable
>   
>   On S390, specifically s390x, the CFA is defined as stack pointer (SP) at call
> diff --git a/libsframe/sframe-dump.c b/libsframe/sframe-dump.c
> index 1fa508d9bad2..f86b5c09ddc5 100644
> --- a/libsframe/sframe-dump.c
> +++ b/libsframe/sframe-dump.c
> @@ -40,6 +40,14 @@ is_sframe_abi_arch_aarch64 (sframe_decoder_ctx *sfd_ctx)
>     return aarch64_p;
>   }
>   
> +/* Return TRUE if the SFrame section is associated with the s390 ABIs.  */
> +
> +static bool
> +is_sframe_abi_arch_s390 (sframe_decoder_ctx *sfd_ctx)
> +{
> +  return sframe_decoder_get_abi_arch (sfd_ctx) == SFRAME_ABI_S390_ENDIAN_BIG;
> +}
> +
>   static void
>   dump_sframe_header (sframe_decoder_ctx *sfd_ctx)
>   {
> @@ -175,7 +183,13 @@ dump_sframe_func_with_fres (sframe_decoder_ctx *sfd_ctx,
>   
>         /* Dump SP/FP info.  */
>         if (err[1] == 0)
> -	sprintf (temp, "c%+d", fp_offset);
> +	{
> +	  if (is_sframe_abi_arch_s390 (sfd_ctx)
> +	      && SFRAME_S390_OFFSET_IS_REGNUM (fp_offset))
> +	    sprintf (temp, "r%d", SFRAME_S390_OFFSET_DECODE_REGNUM (fp_offset));
> +	  else
> +	    sprintf (temp, "c%+d", fp_offset);
> +	}
>         else
>   	strcpy (temp, "u");
>         printf ("%-10s", temp);
> @@ -187,7 +201,13 @@ dump_sframe_func_with_fres (sframe_decoder_ctx *sfd_ctx,
>   	  != SFRAME_CFA_FIXED_RA_INVALID)
>   	strcpy (temp, "f");
>         else if (err[2] == 0)
> -	sprintf (temp, "c%+d", ra_offset);
> +	{
> +	  if (is_sframe_abi_arch_s390 (sfd_ctx)
> +	      && SFRAME_S390_OFFSET_IS_REGNUM (ra_offset))
> +	    sprintf (temp, "r%d", SFRAME_S390_OFFSET_DECODE_REGNUM (ra_offset));
> +	  else
> +	    sprintf (temp, "c%+d", ra_offset);
> +	}
>         else
>   	strcpy (temp, "u");
>
  

Hello Indu,

thank you for your review feedback!

On 17.04.2025 20:50, Indu Bhagat wrote:
> On 4/2/25 9:11 AM, Jens Remus wrote:
>> GCC on s390x, when in a leaf function, can be observed to save the
>> frame pointer (FP) and/or return address (RA) register in a floating-
>> point registers (FPR) instead of on the stack.  This is declared using
>> the following CFI directive:
>>
>>    .cfi_register <fp/ra-regnum>, <fpr-regnum>
>>
>> SFrame cannot represent the FP and/or RA being saved in another
>> register.  It does only track the CFA base register (SP/FP), CFA offset
>> from CFA base register, and FP and RA save area offsets from CFA.
>>
>> On s390x the FP and/or RA are only saved in another FPR when in a leaf
>> function.  That is a function that does not call any other function.
>> Therefore it can ever only be the topmost function in a call chain.
>> An unwinder by default has access to all registers of the function that
>> is the topmost on the call stack.  Therefore no further information
>> is required to restore FP/RA from the FPR.
>>
>> Represent FP/RA saved in another register on s390, by encoding the
>> DWARF register number shifted by one to the left with the least-
>> significant bit set in the offset as follows:
>>
>>    offset = (regnum << 1) | 1
>>
> 
> I wonder if it makes sense to just save the three bits for any future use by doing this now:
> 
>      offset = (regnum << 3) | 1
> 
> I am still undecided.

While that would save two bits for future use as offset specific flags
it would have the downside of preventing the use of signed 8-bit SFrame
offsets in those FREs:

GCC can be observed to save the FP and RA in FPR on s390x.  The lowest
FPR DWARF register number on s390x is 16 (for f0).  16 << 3 | 1 = 129.
129 does not fit into a signed 8-bit integer.

Not sure what else we might want to store in offsets.  Do you see any
use for SFrame V3 support of non-SP/FP CFA base registers in the topmost
frame?

>> The use of the least-significant bit of the offset as indication is
>> possible, as the stack pointer (SP), the CFA, and any register save
>> area slots are 8-byte aligned according to the s390x ELF ABI:
>> - The stack pointer (SP) "shall maintain an 8-byte alignment". [1]
>> - The CFA is defined as SP at call site +160. [2]
>> - Pointers and 8-byte integers, such as general register values, must
>>    be 8-byte aligned. [3]
>> SFrame FP and RA stack offsets must therefore always be a multiple of
>> 8 on s390x.  Note that for the same reason the DWARF data alignment
>> factor is -8 on s390x (see DWARF2_CIE_DATA_ALIGNMENT).
>>
>> Add s390x-specific SFrame (error) tests for FP/RA saved in FPRs in leaf
>> function.
>>
>> [1]: s390x ELF ABI, sections "Register Roles" and "Stack Frame
>>       Allocation", https://github.com/IBM/s390x-abi/releases
>> [2]: s390x ELF ABI, commit 4e38ad9c8a88 ("Document the CFA"),
>>       https://github.com/IBM/s390x-abi/commit/4e38ad9c8a88
>> [3]: s390x ELF ABI, section "Fundamental Types", table "Scalar types",
>>       https://github.com/IBM/s390x-abi/releases
>>
>> include/
>>     * sframe.h (SFRAME_S390_OFFSET_IS_REGNUM): New S390-specific
>>     macro to test whether an SFrame FP/RA offset is a DWARF register
>>     number.
>>     (SFRAME_S390_OFFSET_ENCODE_REGNUM): New S390-specific macro to
>>     encode a DWARF register number into an SFrame FP/RA offset.
>>     (SFRAME_S390_OFFSET_DECODE_REGNUM): New S390-specific macro to
>>     decode an SFrame FP/RA offset into a DWARF register number.
>>
>> gas/
>>     * gen-sframe.c (s390_sframe_xlate_do_register): New S390-
>>     specific function.  Uses SFRAME_S390_OFFSET_ENCODE_REGNUM to
>>     represent FP/RA saved in another register on S390.
>>     (sframe_xlate_do_register): Invoke s390_sframe_xlate_do_register
>>     on S390.
>>
>> libsframe/
>>     * sframe-dump.c (is_sframe_abi_arch_s390): New helper to test
>>     whether ABI/arch is S390.
>>     (dump_sframe_func_with_fres): Use SFRAME_S390_OFFSET_IS_REGNUM
>>     and SFRAME_S390_OFFSET_DECODE_REGNUM to dump FP/RA saved in
>>     another register on S390.
>>     * doc/sframe-spec.texi (S390): Document S390-specific
>>     representation of FP/RA saved in another register.
>>
> 
> I did think for a bit: what sframe_fre_get_fp_offset () and
> sframe_fre_get_ra_offset () in libsframe/sframe.c ?
> 
> On more thought, I think its okay for these APIs to return the "raw
> data", and a stack tracer for s390x can do the right thing if the LSB
> is 1 etc.  However, I have not yet looked at the stacktracer fixes (in
> the branch we shared sometime ago), to be able to comment if one way
> is clunker than other.  Thoughts ?

Having them return an "error" indication that the FP/RA offset is not
available, as it is actually a register number, does not seem to make
the API easier to use.

What I thought of, but assumed would require to bump to SFrame V3 API,
would be something as follows:

struct sframe_reg_save_info {
   enum sframe_reg_save_loc {
     REG_SAVE_NONE,
     REG_SAVE_STACK,
     REG_SAVE_REG,
   } loc;
   union {
     int32_t offset;
     int32_t regnum;
   } u;
};

void
sframe_fre_get_fp_offset (sframe_decoder_ctx *dctx,
			  sframe_frame_row_entry *fre,
			  struct sframe_reg_save_info *fp_save_info,
			  int *errp);

The generic implementation (for x86-64 and AArch64) would then be:

{
   int32_t offset;
   offset = sframe_fre_get_fp_offset (dctx, fre, errp);
   if (*errp == 0)
     {
       fp_save_info->loc = REG_SAVE_STACK;
       fp_save_info->u.offset = offset;
     }
   else
     fp_save_info->loc = REG_SAVE_NONE;
}

The s390x-specific implementation would then handle the case where the
offset is actually an encoded DWARF register number and return that
respectively.

As this only adds new API functions it could perhaps also be introduced
now to libsframe and used in libsframe/sframe-dump.c.  What are your
thoughts?

> Perhaps we atleast add some comments, if you see appropriate, in the
> above APIs.  Something  along the lines that for s390x, for top most
> functions, this data may be for a save to register.  The API just
> returns the raw data in the offset.  Perhaps in the function-level
> comment if you like.  This is just to keep things clear as sometimes
> folks may refer to libsframe APIs when implementing their own SFrame
> based stack tracer.

Sure, will add a comment.


>> diff --git a/libsframe/doc/sframe-spec.texi b/libsframe/doc/sframe-spec.texi
>> index eba497b4d371..5bd027828e3f 100644
>> --- a/libsframe/doc/sframe-spec.texi
>> +++ b/libsframe/doc/sframe-spec.texi
>> @@ -848,16 +848,27 @@ tracked in this ABI, the second stack offset is always used to locate the RA
>>   stack slot, by interpreting it as: RA = CFA + offset2.  The third stack offset
>>   is used to locate the FP stack slot, by interpreting it as: FP = CFA + offset3.
>> -Given the nature of things, the number of stack offsets seen on S390 per
>> -SFrame FRE is either 1, 2, or 3.
>> +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
>> +in the SFrame stack trace format the DWARF register number is encoded as
>> +RA/FP offset using the least-significant bit (LSB) as indication:
>> +offset = (regnum << 1) | 1.  A LSB of zero indicates a stack slot offset.
>> +A LSB of one indicates a DWARF register number, which is interpreted as:
>> +regnum = offset >> 1.  Given the nature of leaf functions, this can only occur
>> +in the topmost frame during stack tracing.
>> +
> 
> Can we also weave in something along the lines of:
> 
> It is recommended that a stack tracer implementation performs the
> required sanity checks to ensure that restoring FP and RA from the
> said register locations is done only for top-most stack frames in the
> callchain.

Ok, will do.

>> +Given the nature of things, the number of stack offsets and/or register numbers
>> +seen on S390 per SFrame FRE is either 1, 2, or 3.
>>   Hence, in summary:
>> -@multitable {Offset ID} {Interpretation in S390 in X}
>> +@multitable @columnfractions .15 .85
>>   @headitem Offset ID @tab Interpretation in S390
>>   @item 1 @tab CFA = @code{BASE_REG} + offset1
>> -@item 2 @tab RA = CFA + offset2
>> -@item 3 @tab FP = CFA + offset3
>> +@item 2 @tab RA stack slot = CFA + offset2, if (offset2 & 1 == 0)
>> +           @*RA register number = offset2 >> 1, if (offset2 & 1 == 1)
>> +@item 3 @tab FP stack slot = CFA + offset3, if (offset3 & 1 == 0)
>> +           @*FP register number = offset3 >> 1, if (offset3 & 1 == 1)
>>   @end multitable
>>   On S390, specifically s390x, the CFA is defined as stack pointer (SP) at call

Regards,
Jens
  

On 4/22/25 4:28 AM, Jens Remus wrote:
> Hello Indu,
> 
> thank you for your review feedback!
> 
> On 17.04.2025 20:50, Indu Bhagat wrote:
>> On 4/2/25 9:11 AM, Jens Remus wrote:
>>> GCC on s390x, when in a leaf function, can be observed to save the
>>> frame pointer (FP) and/or return address (RA) register in a floating-
>>> point registers (FPR) instead of on the stack.  This is declared using
>>> the following CFI directive:
>>>
>>>    .cfi_register <fp/ra-regnum>, <fpr-regnum>
>>>
>>> SFrame cannot represent the FP and/or RA being saved in another
>>> register.  It does only track the CFA base register (SP/FP), CFA offset
>>> from CFA base register, and FP and RA save area offsets from CFA.
>>>
>>> On s390x the FP and/or RA are only saved in another FPR when in a leaf
>>> function.  That is a function that does not call any other function.
>>> Therefore it can ever only be the topmost function in a call chain.
>>> An unwinder by default has access to all registers of the function that
>>> is the topmost on the call stack.  Therefore no further information
>>> is required to restore FP/RA from the FPR.
>>>
>>> Represent FP/RA saved in another register on s390, by encoding the
>>> DWARF register number shifted by one to the left with the least-
>>> significant bit set in the offset as follows:
>>>
>>>    offset = (regnum << 1) | 1
>>>
>>
>> I wonder if it makes sense to just save the three bits for any future 
>> use by doing this now:
>>
>>      offset = (regnum << 3) | 1
>>
>> I am still undecided.
> 
> While that would save two bits for future use as offset specific flags
> it would have the downside of preventing the use of signed 8-bit SFrame
> offsets in those FREs:
> 
> GCC can be observed to save the FP and RA in FPR on s390x.  The lowest
> FPR DWARF register number on s390x is 16 (for f0).  16 << 3 | 1 = 129.
> 129 does not fit into a signed 8-bit integer.
> 

OK. Dropping the idea of << 3 makes more sense then.

> Not sure what else we might want to store in offsets.  Do you see any
> use for SFrame V3 support of non-SP/FP CFA base registers in the topmost
> frame?
> 

Nothing yet.

>>> The use of the least-significant bit of the offset as indication is
>>> possible, as the stack pointer (SP), the CFA, and any register save
>>> area slots are 8-byte aligned according to the s390x ELF ABI:
>>> - The stack pointer (SP) "shall maintain an 8-byte alignment". [1]
>>> - The CFA is defined as SP at call site +160. [2]
>>> - Pointers and 8-byte integers, such as general register values, must
>>>    be 8-byte aligned. [3]
>>> SFrame FP and RA stack offsets must therefore always be a multiple of
>>> 8 on s390x.  Note that for the same reason the DWARF data alignment
>>> factor is -8 on s390x (see DWARF2_CIE_DATA_ALIGNMENT).
>>>
>>> Add s390x-specific SFrame (error) tests for FP/RA saved in FPRs in leaf
>>> function.
>>>
>>> [1]: s390x ELF ABI, sections "Register Roles" and "Stack Frame
>>>       Allocation", https://github.com/IBM/s390x-abi/releases
>>> [2]: s390x ELF ABI, commit 4e38ad9c8a88 ("Document the CFA"),
>>>       https://github.com/IBM/s390x-abi/commit/4e38ad9c8a88
>>> [3]: s390x ELF ABI, section "Fundamental Types", table "Scalar types",
>>>       https://github.com/IBM/s390x-abi/releases
>>>
>>> include/
>>>     * sframe.h (SFRAME_S390_OFFSET_IS_REGNUM): New S390-specific
>>>     macro to test whether an SFrame FP/RA offset is a DWARF register
>>>     number.
>>>     (SFRAME_S390_OFFSET_ENCODE_REGNUM): New S390-specific macro to
>>>     encode a DWARF register number into an SFrame FP/RA offset.
>>>     (SFRAME_S390_OFFSET_DECODE_REGNUM): New S390-specific macro to
>>>     decode an SFrame FP/RA offset into a DWARF register number.
>>>
>>> gas/
>>>     * gen-sframe.c (s390_sframe_xlate_do_register): New S390-
>>>     specific function.  Uses SFRAME_S390_OFFSET_ENCODE_REGNUM to
>>>     represent FP/RA saved in another register on S390.
>>>     (sframe_xlate_do_register): Invoke s390_sframe_xlate_do_register
>>>     on S390.
>>>
>>> libsframe/
>>>     * sframe-dump.c (is_sframe_abi_arch_s390): New helper to test
>>>     whether ABI/arch is S390.
>>>     (dump_sframe_func_with_fres): Use SFRAME_S390_OFFSET_IS_REGNUM
>>>     and SFRAME_S390_OFFSET_DECODE_REGNUM to dump FP/RA saved in
>>>     another register on S390.
>>>     * doc/sframe-spec.texi (S390): Document S390-specific
>>>     representation of FP/RA saved in another register.
>>>
>>
>> I did think for a bit: what sframe_fre_get_fp_offset () and
>> sframe_fre_get_ra_offset () in libsframe/sframe.c ?
>>
>> On more thought, I think its okay for these APIs to return the "raw
>> data", and a stack tracer for s390x can do the right thing if the LSB
>> is 1 etc.  However, I have not yet looked at the stacktracer fixes (in
>> the branch we shared sometime ago), to be able to comment if one way
>> is clunker than other.  Thoughts ?
> 
> Having them return an "error" indication that the FP/RA offset is not
> available, as it is actually a register number, does not seem to make
> the API easier to use.
> 
> What I thought of, but assumed would require to bump to SFrame V3 API,
> would be something as follows:
> 
> struct sframe_reg_save_info {
>    enum sframe_reg_save_loc {
>      REG_SAVE_NONE,
>      REG_SAVE_STACK,
>      REG_SAVE_REG,
>    } loc;
>    union {
>      int32_t offset;
>      int32_t regnum;
>    } u;
> };
> 
> void
> sframe_fre_get_fp_offset (sframe_decoder_ctx *dctx,
>                sframe_frame_row_entry *fre,
>                struct sframe_reg_save_info *fp_save_info,
>                int *errp);
> 
> The generic implementation (for x86-64 and AArch64) would then be:
> 
> {
>    int32_t offset;
>    offset = sframe_fre_get_fp_offset (dctx, fre, errp);
>    if (*errp == 0)
>      {
>        fp_save_info->loc = REG_SAVE_STACK;
>        fp_save_info->u.offset = offset;
>      }
>    else
>      fp_save_info->loc = REG_SAVE_NONE;
> }
> 
> The s390x-specific implementation would then handle the case where the
> offset is actually an encoded DWARF register number and return that
> respectively.
> 
> As this only adds new API functions it could perhaps also be introduced
> now to libsframe and used in libsframe/sframe-dump.c.  What are your
> thoughts?
> 

BTW, libsframe versioning is doable if you want to add a new API. 
libsframe version update does not imply a SFrame version bump.

On second thought, I think its okay to continue to have the current 
interface and return the "raw data" for s390x.  Ideally, avoiding 
further overhead for every architecture/ABI is sensible (an approach 
that real world stack tracers would want to adopt too), we should 
however add helpful comments.