[RFC PATCH 0/9] s390: Initial support to generate SFrame info from CFI directives in assembler

[RFC PATCH 0/9] s390: Initial support to generate SFrame info from CFI directives in assembler

[Jens Remus]

This patch series adds initial support to the assembler on s390x to
generate SFrame stack trace information from CFI directives. It is
largely based on the respective AArch64 support.

Please be aware that the SFrame support for s390x provided by patch 9
of this series still has some open issues, which need to be addressed.
Any ideas or assistance to overcome the current SFrame limitations
listed below and in the patch description are very welcome.

Patches 1-8 are generic cleanups and enhancements to the generation
of SFrame information in the assembler. Patch 9 adds the initial s390x
support and is purely RFC.


Patches 1-3 are minor cleanups/enhancements to the existing SFrame
support on AArch64 and x86 AMD64.

Patch 4 enables readelf/objdump to dump the SFrame fixed offsets from
CFA to the frame pointer (FP) and return address (RA).

Patch 5 enhances an SFrame warning message to print the human readable
DWARF call frame instruction name.

Patch 6 and 7 resolve issues that cause the assembler to either generate
bad SFrame FDE or to silently skip it. Both issues would be triggered by
s390-specific SFrame error test cases introduced by patch 9.

Patch 8 adds verbose assembler warning messages when generation of
SFrame FDE is skipped.

Patch 9 adds initial support to the assembler on s390x to generate
SFrame stack trace information from CFI directives. Due to differences
in the s390x ELF ABI [1] compared to the AArch64 and x68 AMD64 ELF ABIs
and the simplified assumptions of the current SFrame format V2 there
are several unresolved issues (see also patch description):

- GCC on s390x may save the frame pointer (FP; r11) and/or return
  address (RA; r14) registers in other registers (e.g. floating-point
  registers) instead of saving them on the stack. SFrame currently only
  supports FP/RA tracking on the stack.
  This can be reproduced by using a small inline assembler statement,
  that specifies r11 (FP) and/or r14 (RA) as output operand(s). It can
  also be observed when compiling a minimal C program using GCC without
  optimizations.

- The s390x ELF ABI does not specify a frame pointer register number.
  Currently GCC and clang both mostly use register r11, but GCC can
  also be observed to use r14. On s390x a compiler may choose about
  any register number for that purpose and declare it using CFI
  directives.
  This can be observed when compiling glibc, as parts of its hand-
  written assembler uses register r12 as frame pointer.

- GCC on s390x may copy the return address (RA) from register r14 to
  another register and even use that to return to the caller.
  Effectively this is just a specialization of the first bullet.

If my understanding of the current SFrame format is not wrong I assume
the following enhancements to SFrame would be required to fully support
the s390x architecture:

- Tracking of the CFA base pointer register number specified in CFI
  directives .cfi_def_cfa and .cfi_def_cfa_register.

- Tracking of SP, FP, and RA register contents being saved in other
  registers instead of on the stack, as specified by CFI directive
  .cfi_register.

- Both would effectively require support in SFrame to track all of the
  17 registers specified as saved in the s390x ELF ABI (r6-r13, r15,
  and f8-f15), since SP, FP, and RA could be saved in one of those and
  thus would need to be restored during unwinding.

- Potentially optional, if the use in glibc could be removed:
  Tracking of SP, FP, and RA register contents being CFA+offset, as
  specified by .cfi_val_offset.

[1] ELF ABI s390x Supplement:
    https://github.com/IBM/s390x-abi/releases

Changes v1 -> v2:
- Resolved a regression reported by Linaro-TCWG-CI on AArch64 in one of
  the generic ld SFrame test cases. The test case contained a
  .cfi_def_cfa directive, specifying a CFA base register number that is
  not necessarily a SFrame SP/FP register number on all architectures.
  This caused the changes from patch 6 to skip SFrame FDE generation on
  AArch64 (and s390x aswell) with a warning, causing the test case to
  fail.

Thanks and regards,
Jens

Jens Remus (9):
  x86: Remove unused SFrame CFI RA register variable
  aarch64: Align SFrame terminology in comments to specs and x86
  sframe: Enhance comments for SFRAME_CFA_*_REG macros
  readelf/objdump: Dump SFrame CFA fixed FP and RA offsets
  gas: Print DWARF call frame insn name in SFrame warning message
  gas: Skip SFrame FDE if CFI specifies non-FP/SP base register
  gas: Warn if SFrame FDE is skipped due to non-default return column
  gas: User readable warnings if SFrame FDE is not generated
  s390: Initial support to generate .sframe from CFI directives in
    assembler

 gas/config/tc-aarch64.h                       |   6 +-
 gas/config/tc-i386.c                          |   1 -
 gas/config/tc-i386.h                          |   4 +-
 gas/config/tc-s390.c                          |  55 +++++++
 gas/config/tc-s390.h                          |  31 ++++
 gas/gen-sframe.c                              | 146 ++++++++++++++++--
 gas/gen-sframe.h                              |   2 +
 .../gas/cfi-sframe/cfi-sframe-common-1.d      |   2 +
 .../gas/cfi-sframe/cfi-sframe-common-2.d      |   2 +
 .../gas/cfi-sframe/cfi-sframe-common-3.d      |   2 +
 .../gas/cfi-sframe/cfi-sframe-common-4.d      |   2 +
 .../gas/cfi-sframe/cfi-sframe-common-5.d      |   2 +
 .../gas/cfi-sframe/cfi-sframe-common-6.d      |   2 +
 .../gas/cfi-sframe/cfi-sframe-common-7.d      |   2 +
 .../gas/cfi-sframe/cfi-sframe-common-8.d      |   2 +
 .../gas/cfi-sframe/cfi-sframe-s390-1.d        |  23 +++
 .../gas/cfi-sframe/cfi-sframe-s390-1.s        |  37 +++++
 .../gas/cfi-sframe/cfi-sframe-s390-2.d        |  23 +++
 .../gas/cfi-sframe/cfi-sframe-s390-2.s        |  37 +++++
 .../gas/cfi-sframe/cfi-sframe-s390-err-1.d    |  15 ++
 .../gas/cfi-sframe/cfi-sframe-s390-err-1.s    |  37 +++++
 .../gas/cfi-sframe/cfi-sframe-s390-err-2.d    |  15 ++
 .../gas/cfi-sframe/cfi-sframe-s390-err-2.s    |  37 +++++
 .../gas/cfi-sframe/cfi-sframe-s390-err-3.d    |  15 ++
 .../gas/cfi-sframe/cfi-sframe-s390-err-3.s    |   6 +
 .../gas/cfi-sframe/cfi-sframe-s390-err-4.d    |  15 ++
 .../gas/cfi-sframe/cfi-sframe-s390-err-4.s    |   5 +
 .../gas/cfi-sframe/cfi-sframe-x86_64-1.d      |   1 +
 gas/testsuite/gas/cfi-sframe/cfi-sframe.exp   |  13 +-
 gas/testsuite/gas/cfi-sframe/common-empty-1.d |   4 +-
 gas/testsuite/gas/cfi-sframe/common-empty-2.d |   4 +-
 gas/testsuite/gas/cfi-sframe/common-empty-3.d |   3 +
 .../gas/scfi/x86_64/scfi-cfi-sections-1.d     |   2 +
 .../gas/scfi/x86_64/scfi-dyn-stack-1.d        |   2 +
 include/sframe.h                              |   7 +-
 ld/testsuite/ld-sframe/discard.s              |   1 -
 libsframe/doc/sframe-spec.texi                |   8 +-
 libsframe/sframe-dump.c                       |  10 ++
 38 files changed, 549 insertions(+), 32 deletions(-)
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-1.d
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-1.s
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-2.d
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-2.s
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-1.d
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-1.s
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-2.d
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-2.s
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-3.d
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-3.s
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-4.d
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-4.s

-- 
2.40.1

[PATCH v2 1/9] x86: Remove unused SFrame CFI RA register variable

[Jens Remus]

gas/
	* config/tc-i386.c: Remove unused SFrame CFI RA register
	  variable.

Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
 gas/config/tc-i386.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index c56ca4a2b4b8..1a6fd52f229a 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -627,7 +627,6 @@ static int shared = 0;
 unsigned int x86_sframe_cfa_sp_reg;
 /* The other CFA base register for SFrame stack trace info.  */
 unsigned int x86_sframe_cfa_fp_reg;
-unsigned int x86_sframe_cfa_ra_reg;
 
 #endif
 
-- 
2.40.1

[PATCH v2 2/9] aarch64: Align SFrame terminology in comments to specs and x86

[Jens Remus]

Use the term "frame pointer" (FP) instead of "base pointer". This aligns
with the terminology used in the SFrame specification. Additionally it
helps not to confuse "base-pointer register" with the term "BASE_REG"
used in the specification to denote either the SP or FP register.

While at it align the frame-pointer and return address register comments
to the x86 AMD64 ones.

gas/
	* config/tc-aarch64.h: Align SFrame terminology in comments to
	  specs and x86 AMD64.

Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---

Notes (jremus):
    This patch can be dropped, if subsequent patch "sframe: Enhance comments
    for SFRAME_CFA_*_REG macros" gets accepted.

 gas/config/tc-aarch64.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gas/config/tc-aarch64.h b/gas/config/tc-aarch64.h
index 1b8badad9fdc..599d78db7908 100644
--- a/gas/config/tc-aarch64.h
+++ b/gas/config/tc-aarch64.h
@@ -271,11 +271,11 @@ extern bool aarch64_support_sframe_p (void);
 extern unsigned int aarch64_sframe_cfa_sp_reg;
 #define SFRAME_CFA_SP_REG aarch64_sframe_cfa_sp_reg
 
-/* The base-pointer register number for CFA stack trace info.  */
+/* The frame-pointer register number for SFrame stack trace info.  */
 extern unsigned int aarch64_sframe_cfa_fp_reg;
 #define SFRAME_CFA_FP_REG aarch64_sframe_cfa_fp_reg
 
-/* The return address register number for CFA stack trace info.  */
+/* The return address register number for SFrame stack trace info.  */
 extern unsigned int aarch64_sframe_cfa_ra_reg;
 #define SFRAME_CFA_RA_REG aarch64_sframe_cfa_ra_reg
 
-- 
2.40.1

[PATCH v2 3/9] sframe: Enhance comments for SFRAME_CFA_*_REG macros

[Jens Remus]

Specify what the SFRAME_CFA_*_REG register numbers are used for:
- SP (stack pointer): CFA tracking
- FP (frame pointer): CFA and FP tracking
- RA (return address): RA tracking

gas/
	* config/tc-aarch64.h: Enhance comments for SFRAME_CFA_*_REG
	  macros.
	* config/tc-i386.h: Likewise.

Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
 gas/config/tc-aarch64.h | 6 +++---
 gas/config/tc-i386.h    | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/gas/config/tc-aarch64.h b/gas/config/tc-aarch64.h
index 599d78db7908..59c9b5a09ec0 100644
--- a/gas/config/tc-aarch64.h
+++ b/gas/config/tc-aarch64.h
@@ -267,15 +267,15 @@ extern void aarch64_after_parse_args (void);
 extern bool aarch64_support_sframe_p (void);
 #define support_sframe_p aarch64_support_sframe_p
 
-/* The stack-pointer register number for SFrame stack trace info.  */
+/* The stack-pointer register number for CFA tracking.  */
 extern unsigned int aarch64_sframe_cfa_sp_reg;
 #define SFRAME_CFA_SP_REG aarch64_sframe_cfa_sp_reg
 
-/* The frame-pointer register number for SFrame stack trace info.  */
+/* The frame-pointer register number for CFA and FP tracking.  */
 extern unsigned int aarch64_sframe_cfa_fp_reg;
 #define SFRAME_CFA_FP_REG aarch64_sframe_cfa_fp_reg
 
-/* The return address register number for SFrame stack trace info.  */
+/* The return address register number for RA tracking.  */
 extern unsigned int aarch64_sframe_cfa_ra_reg;
 #define SFRAME_CFA_RA_REG aarch64_sframe_cfa_ra_reg
 
diff --git a/gas/config/tc-i386.h b/gas/config/tc-i386.h
index b93799a3b48c..0d6fb002166f 100644
--- a/gas/config/tc-i386.h
+++ b/gas/config/tc-i386.h
@@ -440,11 +440,11 @@ extern bool x86_scfi_callee_saved_p (uint32_t dw2reg_num);
 extern bool x86_support_sframe_p (void);
 #define support_sframe_p x86_support_sframe_p
 
-/* The stack-pointer register number for SFrame stack trace info.  */
+/* The stack-pointer register number for CFA tracking.  */
 extern unsigned int x86_sframe_cfa_sp_reg;
 #define SFRAME_CFA_SP_REG x86_sframe_cfa_sp_reg
 
-/* The frame-pointer register number for SFrame stack trace info.  */
+/* The frame-pointer register number for CFA and FP tracking.  */
 extern unsigned int x86_sframe_cfa_fp_reg;
 #define SFRAME_CFA_FP_REG x86_sframe_cfa_fp_reg
 
-- 
2.40.1

[PATCH v2 4/9] readelf/objdump: Dump SFrame CFA fixed FP and RA offsets

[Jens Remus]

The SFrame format allows architectures to specify fixed offsets from the
CFA, if any, from which the frame pointer (FP) and/or return address
(RA) may be recovered. These offsets are stored in the SFrame header.

For instance the SFrame generation in the assembler for x86 AMD64
specifies a fixed offset from the CFA, from which the return address
(RA) may be recovered.

When dumping the SFrame header, for instance in readelf/objdump with
option --sframe, do also dump the specified fixed offsets from the CFA,
if any, from which the frame pointer (FP) and return address (RA) may
be recovered.

Update the common SFrame test case verification patterns to allow for
the optional dumping of the CFA fixed FP/RA offsets. Update the x86-
specific SFrame and SCFI test case verification patterns to require a
CFA fixed RA offset of -8.

libsframe:
	* sframe-dump.c: Dump CFA fixed FP and RA offsets.

gas/testsuite/
	* gas/cfi-sframe/cfi-sframe-common-1.d: Test for optional fixed
	  FP and RA offsets.
	* gas/cfi-sframe/cfi-sframe-common-2.d: Likewise.
	* gas/cfi-sframe/cfi-sframe-common-3.d: Likewise.
	* gas/cfi-sframe/cfi-sframe-common-4.d: Likewise.
	* gas/cfi-sframe/cfi-sframe-common-5.d: Likewise.
	* gas/cfi-sframe/cfi-sframe-common-6.d: Likewise.
	* gas/cfi-sframe/cfi-sframe-common-7.d: Likewise.
	* gas/cfi-sframe/cfi-sframe-common-8.d: Likewise.
	* gas/cfi-sframe/cfi-sframe-x86_64-1.d: Test for fixed RA
	  offset.
	* gas/cfi-sframe/common-empty-1.d: Test for optional fixed FP
	  and RA offsets.
	* gas/cfi-sframe/common-empty-2.d: Likewise.
	* gas/cfi-sframe/common-empty-3.d: Likewise.
	* gas/scfi/x86_64/scfi-cfi-sections-1.d: Test for SFrame fixed
	  RA offset.
	* gas/scfi/x86_64/scfi-dyn-stack-1.d: Likewise.

Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
 gas/testsuite/gas/cfi-sframe/cfi-sframe-common-1.d  |  2 ++
 gas/testsuite/gas/cfi-sframe/cfi-sframe-common-2.d  |  2 ++
 gas/testsuite/gas/cfi-sframe/cfi-sframe-common-3.d  |  2 ++
 gas/testsuite/gas/cfi-sframe/cfi-sframe-common-4.d  |  2 ++
 gas/testsuite/gas/cfi-sframe/cfi-sframe-common-5.d  |  2 ++
 gas/testsuite/gas/cfi-sframe/cfi-sframe-common-6.d  |  2 ++
 gas/testsuite/gas/cfi-sframe/cfi-sframe-common-7.d  |  2 ++
 gas/testsuite/gas/cfi-sframe/cfi-sframe-common-8.d  |  2 ++
 gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-1.d  |  1 +
 gas/testsuite/gas/cfi-sframe/common-empty-1.d       |  2 ++
 gas/testsuite/gas/cfi-sframe/common-empty-2.d       |  2 ++
 gas/testsuite/gas/cfi-sframe/common-empty-3.d       |  2 ++
 gas/testsuite/gas/scfi/x86_64/scfi-cfi-sections-1.d |  2 ++
 gas/testsuite/gas/scfi/x86_64/scfi-dyn-stack-1.d    |  2 ++
 libsframe/sframe-dump.c                             | 10 ++++++++++
 15 files changed, 37 insertions(+)

diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-1.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-1.d
index 32577f31860e..5f4ae00747de 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-1.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-1.d
@@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
 
     Version: SFRAME_VERSION_2
     Flags: NONE
+#?    CFA fixed FP offset: \-?\d+
+#?    CFA fixed RA offset: \-?\d+
     Num FDEs: 1
     Num FREs: 1
 
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-2.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-2.d
index 3e3f74dbe424..ded8c450a942 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-2.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-2.d
@@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
 
     Version: SFRAME_VERSION_2
     Flags: NONE
+#?    CFA fixed FP offset: \-?\d+
+#?    CFA fixed RA offset: \-?\d+
     Num FDEs: 1
     Num FREs: 1
 
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-3.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-3.d
index 6430d463a891..d23fd9790f63 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-3.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-3.d
@@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
 
     Version: SFRAME_VERSION_2
     Flags: NONE
+#?    CFA fixed FP offset: \-?\d+
+#?    CFA fixed RA offset: \-?\d+
     Num FDEs: 1
     Num FREs: 1
 
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-4.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-4.d
index 319ff96cce2a..ca559bd0a029 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-4.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-4.d
@@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
 
     Version: SFRAME_VERSION_2
     Flags: NONE
+#?    CFA fixed FP offset: \-?\d+
+#?    CFA fixed RA offset: \-?\d+
     Num FDEs: 1
     Num FREs: 3
 
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-5.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-5.d
index 82d34973ddde..ee82053e13db 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-5.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-5.d
@@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
 
     Version: SFRAME_VERSION_2
     Flags: NONE
+#?    CFA fixed FP offset: \-?\d+
+#?    CFA fixed RA offset: \-?\d+
     Num FDEs: 1
     Num FREs: 3
 
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-6.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-6.d
index fe6917c70800..9d54b98552bf 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-6.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-6.d
@@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
 
     Version: SFRAME_VERSION_2
     Flags: NONE
+#?    CFA fixed FP offset: \-?\d+
+#?    CFA fixed RA offset: \-?\d+
     Num FDEs: 1
     Num FREs: 3
 
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-7.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-7.d
index 39724d9cdf19..2b7fe3aec8f4 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-7.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-7.d
@@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
 
     Version: SFRAME_VERSION_2
     Flags: NONE
+#?    CFA fixed FP offset: \-?\d+
+#?    CFA fixed RA offset: \-?\d+
     Num FDEs: 1
     Num FREs: 3
 
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-8.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-8.d
index c0a0e627fad9..d654e1d0bcd4 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-8.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-8.d
@@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
 
     Version: SFRAME_VERSION_2
     Flags: NONE
+#?    CFA fixed FP offset: \-?\d+
+#?    CFA fixed RA offset: \-?\d+
     Num FDEs: 1
     Num FREs: 2
 
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-1.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-1.d
index ae36c21b3b7c..c8b5e6adfea0 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-1.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-1.d
@@ -8,6 +8,7 @@ Contents of the SFrame section .sframe:
 
     Version: SFRAME_VERSION_2
     Flags: NONE
+    CFA fixed RA offset: \-8
     Num FDEs: 1
     Num FREs: 4
 
diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-1.d b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
index b133b15b051d..125612ff841f 100644
--- a/gas/testsuite/gas/cfi-sframe/common-empty-1.d
+++ b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
@@ -9,6 +9,8 @@ Contents of the SFrame section .sframe:
 
     Version: SFRAME_VERSION_2
     Flags: NONE
+#?    CFA fixed FP offset: \-?\d+
+#?    CFA fixed RA offset: \-?\d+
     Num FDEs: 0
     Num FREs: 0
 
diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-2.d b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
index c5bc8594f1b7..59328fc1033f 100644
--- a/gas/testsuite/gas/cfi-sframe/common-empty-2.d
+++ b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
@@ -9,6 +9,8 @@ Contents of the SFrame section .sframe:
 
     Version: SFRAME_VERSION_2
     Flags: NONE
+#?    CFA fixed FP offset: \-?\d+
+#?    CFA fixed RA offset: \-?\d+
     Num FDEs: 0
     Num FREs: 0
 
diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-3.d b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
index df0b19ee1bd1..5914c620760d 100644
--- a/gas/testsuite/gas/cfi-sframe/common-empty-3.d
+++ b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
@@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
 
     Version: SFRAME_VERSION_2
     Flags: NONE
+#?    CFA fixed FP offset: \-?\d+
+#?    CFA fixed RA offset: \-?\d+
     Num FDEs: 0
     Num FREs: 0
 
diff --git a/gas/testsuite/gas/scfi/x86_64/scfi-cfi-sections-1.d b/gas/testsuite/gas/scfi/x86_64/scfi-cfi-sections-1.d
index 5962980256c1..b0cf8f4f5212 100644
--- a/gas/testsuite/gas/scfi/x86_64/scfi-cfi-sections-1.d
+++ b/gas/testsuite/gas/scfi/x86_64/scfi-cfi-sections-1.d
@@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
 
     Version: SFRAME_VERSION_2
     Flags: NONE
+#?    CFA fixed FP offset: \-?\d+
+#?    CFA fixed RA offset: \-?\d+
     Num FDEs: 1
     Num FREs: 5
 
diff --git a/gas/testsuite/gas/scfi/x86_64/scfi-dyn-stack-1.d b/gas/testsuite/gas/scfi/x86_64/scfi-dyn-stack-1.d
index b51546af1494..4a449957187b 100644
--- a/gas/testsuite/gas/scfi/x86_64/scfi-dyn-stack-1.d
+++ b/gas/testsuite/gas/scfi/x86_64/scfi-dyn-stack-1.d
@@ -9,6 +9,8 @@ Contents of the SFrame section .sframe:
 
     Version: SFRAME_VERSION_2
     Flags: NONE
+#?    CFA fixed FP offset: \-?\d+
+#?    CFA fixed RA offset: \-?\d+
     Num FDEs: 1
     Num FREs: 4
 
diff --git a/libsframe/sframe-dump.c b/libsframe/sframe-dump.c
index 42a086a5691f..493d052ce91f 100644
--- a/libsframe/sframe-dump.c
+++ b/libsframe/sframe-dump.c
@@ -47,6 +47,8 @@ dump_sframe_header (sframe_decoder_ctx *sfd_ctx)
   uint8_t flags;
   char *flags_str;
   const char *ver_str = NULL;
+  int8_t cfa_fixed_fp_offset;
+  int8_t cfa_fixed_ra_offset;
   const sframe_header *header = &(sfd_ctx->sfd_header);
 
   /* Prepare SFrame section version string.  */
@@ -82,12 +84,20 @@ dump_sframe_header (sframe_decoder_ctx *sfd_ctx)
   else
     strcpy (flags_str, "NONE");
 
+  /* CFA fixed FP and RA offsets.  */
+  cfa_fixed_fp_offset = header->sfh_cfa_fixed_fp_offset;
+  cfa_fixed_ra_offset = header->sfh_cfa_fixed_ra_offset;
+
   const char* subsec_name = "Header";
   printf ("\n");
   printf ("  %s :\n", subsec_name);
   printf ("\n");
   printf ("    Version: %s\n", ver_str);
   printf ("    Flags: %s\n", flags_str);
+  if (cfa_fixed_fp_offset != SFRAME_CFA_FIXED_FP_INVALID)
+    printf ("    CFA fixed FP offset: %d\n", cfa_fixed_fp_offset);
+  if (cfa_fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID)
+    printf ("    CFA fixed RA offset: %d\n", cfa_fixed_ra_offset);
   printf ("    Num FDEs: %d\n", sframe_decoder_get_num_fidx (sfd_ctx));
   printf ("    Num FREs: %d\n", header->sfh_num_fres);
 
-- 
2.40.1

[PATCH v2 5/9] gas: Print DWARF call frame insn name in SFrame warning message

[Jens Remus]

SFrame generation prints the DWARF call frame instruction opcode in
hexadecimal. Leverage get_DW_CFA_name to additionally print the
DWARF call frame instruction name in human readable form, while also
respecting fake CFI types. Use "(unknown)", if the DWARF call frame
instruction name is not known.

This changes the following assembler SFrame generation warning message
as follows:

Old:
Warning: skipping SFrame FDE due to DWARF CFI op 0x<hexnum>

New:
Warning: skipping SFrame FDE due to DWARF CFI op <name> (0x<hexnum>)

gas/
	* gen-sframe.c (sframe_get_cfi_name): New function to get the
	  DWARF call frame instruction name for a DWARF call frame
	  instruction opcode.
	  (sframe_do_cfi_insn): Use sframe_get_cfi_name to print the
	  DWARF call frame instruction name for the DWARF call frame
	  instruction opcode in the warning message.

gas/testsuite/
	* gas/cfi-sframe/common-empty-1.d: Update expected SFrame
	  warning message text for DWARF call frame insn name.
	* gas/cfi-sframe/common-empty-2.d: Likewise.
	* gas/cfi-sframe/cfi-sframe-s390-err-1.l: Likewise.
	* gas/cfi-sframe/cfi-sframe-s390-err-2.l: Likewise.
	* gas/cfi-sframe/cfi-sframe-s390-err-3.l: Likewise.
	* gas/cfi-sframe/cfi-sframe-s390-err-4.l: Likewise.

Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
 gas/gen-sframe.c                              | 49 ++++++++++++++++++-
 gas/testsuite/gas/cfi-sframe/common-empty-1.d |  2 +-
 gas/testsuite/gas/cfi-sframe/common-empty-2.d |  2 +-
 3 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index 75781fc8ccbd..d35baaac54b2 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -1197,6 +1197,46 @@ sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
   return SFRAME_XLATE_OK;
 }
 
+/* Returns the DWARF call frame instruction name or fake CFI name for the
+   specified CFI opcode, or NULL if the value is not recognized.  */
+
+static const char *
+sframe_get_cfi_name (int cfi_opc)
+{
+  const char *cfi_name;
+
+  switch (cfi_opc)
+    {
+      /* Fake CFI type; outside the byte range of any real CFI insn.  */
+      /* See gas/dw2gencfi.h.  */
+      case CFI_adjust_cfa_offset:
+	cfi_name = "CFI_adjust_cfa_offset";
+	break;
+      case CFI_return_column:
+	cfi_name = "CFI_return_column";
+	break;
+      case CFI_rel_offset:
+	cfi_name = "CFI_rel_offset";
+	break;
+      case CFI_escape:
+	cfi_name = "CFI_escape";
+	break;
+      case CFI_signal_frame:
+	cfi_name = "CFI_signal_frame";
+	break;
+      case CFI_val_encoded_addr:
+	cfi_name = "CFI_val_encoded_addr";
+	break;
+      case CFI_label:
+	cfi_name = "CFI_label";
+	break;
+      default:
+	cfi_name = get_DW_CFA_name (cfi_opc);
+    }
+
+  return cfi_name;
+}
+
 /* Process CFI_INSN and update the translation context with the FRE
    information.
 
@@ -1272,7 +1312,14 @@ sframe_do_cfi_insn (struct sframe_xlate_ctx *xlate_ctx,
   /* An error here will cause no SFrame FDE later.  Warn the user because this
      will affect the overall coverage and hence, asynchronicity.  */
   if (err)
-    as_warn (_("skipping SFrame FDE due to DWARF CFI op %#x"), op);
+    {
+      const char *cfi_name = sframe_get_cfi_name (op);
+
+      if (!cfi_name)
+	cfi_name = _("(unknown)");
+      as_warn (_("skipping SFrame FDE due to DWARF CFI op %s (%#x)"),
+	       cfi_name, op);
+    }
 
   return err;
 }
diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-1.d b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
index 125612ff841f..d7756302b559 100644
--- a/gas/testsuite/gas/cfi-sframe/common-empty-1.d
+++ b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
@@ -1,5 +1,5 @@
 #as: --gsframe
-#warning: skipping SFrame FDE due to DWARF CFI op 0xa
+#warning: skipping SFrame FDE due to DWARF CFI op DW_CFA_remember_state \(0xa\)
 #objdump: --sframe=.sframe
 #name: Uninteresting cfi directives generate an empty SFrame section
 #...
diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-2.d b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
index 59328fc1033f..20282c7854e8 100644
--- a/gas/testsuite/gas/cfi-sframe/common-empty-2.d
+++ b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
@@ -1,5 +1,5 @@
 #as: --gsframe
-#warning: skipping SFrame FDE due to DWARF CFI op 0xe
+#warning: skipping SFrame FDE due to DWARF CFI op DW_CFA_def_cfa_offset \(0xe\)
 #objdump: --sframe=.sframe
 #name: SFrame supports only FP/SP based CFA
 #...
-- 
2.40.1

[PATCH v2 6/9] gas: Skip SFrame FDE if CFI specifies non-FP/SP base register

[Jens Remus]

Do not generate SFrame FDE if DWARF CFI directives .cfi_def_cfa or
.cfi_def_cfa_register specify a CFA base register number other than
the architecture-specific stack-pointer (SP) or frame-pointer (FP)
register numbers.

This also causes the assembler to print a warning message, so that
skipping of the SFrame FDE does not occur silently.

Update the generic ld SFrame test case to be architecture independent.
Do not use CFI directive .cfi_def_cfa, as the specified CFA base
register number is not a valid SP/FP register number on all
architectures. An invalid SP/FP register number will now cause the
assembler to print a warning message and skip SFrame FDE generation.
Remove the offending CFI directive, that cannot be coded architecture-
independent, as the test case requires SFrame information to be
generated. This was reported by the Linaro-TCWG-CI for AArch64.

gas/
	* gen-sframe.c: Skip SFrame generation if CFI specifies
	  non-FP/SP base register.

ld/testsuite/
	* ld-sframe/discard.s: Update generic SFrame test case to be
	  architecture independent.

Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---

Notes (jremus):
    v1 -> v2:
    - Update generic SFrame test case to be architecture independent to
      resolve generic ld SFrame test case failure reported by
      Linaro-TCWG-CI for AArch64. It would fail similar on s390x.
    
    Without this patch the assembler would erroneously generate bad SFrame
    information for the s390-specific SFrame error test cases 1 and 2, that
    get introduced by patch "s390: Initial support to generate .sframe from
    CFI directives in assembler".

 gas/gen-sframe.c                 | 13 +++++++++++--
 ld/testsuite/ld-sframe/discard.s |  1 -
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index d35baaac54b2..1269b2b77c54 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -986,7 +986,11 @@ sframe_xlate_do_def_cfa (struct sframe_xlate_ctx *xlate_ctx,
 			       get_dw_fde_start_addrS (xlate_ctx->dw_fde));
   }
   /* Define the current CFA rule to use the provided register and
-     offset.  */
+     offset.  However, if the register is not FP/SP, skip creating
+     SFrame stack trace info for the function.  */
+  if (cfi_insn->u.r != SFRAME_CFA_SP_REG
+      && cfi_insn->u.r != SFRAME_CFA_FP_REG)
+    return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
   sframe_fre_set_cfa_base_reg (cur_fre, cfi_insn->u.ri.reg);
   sframe_fre_set_cfa_offset (cur_fre, cfi_insn->u.ri.offset);
   cur_fre->merge_candidate = false;
@@ -1004,9 +1008,14 @@ sframe_xlate_do_def_cfa_register (struct sframe_xlate_ctx *xlate_ctx,
   struct sframe_row_entry *last_fre = xlate_ctx->last_fre;
   /* Get the scratchpad FRE.  This FRE will eventually get linked in.  */
   struct sframe_row_entry *cur_fre = xlate_ctx->cur_fre;
+
   gas_assert (cur_fre);
   /* Define the current CFA rule to use the provided register (but to
-     keep the old offset).  */
+     keep the old offset).  However, if the register is not FP/SP,
+     skip creating SFrame stack trace info for the function.  */
+  if (cfi_insn->u.r != SFRAME_CFA_SP_REG
+      && cfi_insn->u.r != SFRAME_CFA_FP_REG)
+    return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
   sframe_fre_set_cfa_base_reg (cur_fre, cfi_insn->u.ri.reg);
   sframe_fre_set_cfa_offset (cur_fre, last_fre->cfa_offset);
   cur_fre->merge_candidate = false;
diff --git a/ld/testsuite/ld-sframe/discard.s b/ld/testsuite/ld-sframe/discard.s
index a438b42bffa1..5591a50d486a 100644
--- a/ld/testsuite/ld-sframe/discard.s
+++ b/ld/testsuite/ld-sframe/discard.s
@@ -5,7 +5,6 @@
 foo:
 	.cfi_startproc
 	.cfi_def_cfa_offset 16
-	.cfi_def_cfa 7, 8
 	.cfi_endproc
 
 	.globl _start
-- 
2.40.1

[PATCH v2 7/9] gas: Warn if SFrame FDE is skipped due to non-default return column

[Jens Remus]

Print a warning message if SFrame FDE is skipped due to a non-default
DWARF return column (i.e. return address (RA) register number). This
may be caused by the use of CFI directive .cfi_return_column with a
non-default return address (RA) register number in the processed
assembler source code.

  Warning: skipping SFrame FDE due to non-default DWARF return column

gas/
	* gen-sframe.c: Warn if SFrame FDE is skipped due to non-default
	  DWARF return column.

gas/testsuite/
	* gas/cfi-sframe/common-empty-3.d: Update test case to expect
	  for new warning message when SFrame FDE is skipped due to
	  a non-default DWARF return column.

Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---

Notes (jremus):
    Without this patch the assembler would erroneously generate bad SFrame
    information for the s390-specific SFrame error test case 4, that gets
    introduced by patch "s390: Initial support to generate .sframe from CFI
    directives in assembler".

 gas/gen-sframe.c                              | 8 ++++++--
 gas/testsuite/gas/cfi-sframe/common-empty-3.d | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index 1269b2b77c54..28b49a2a8425 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -1345,7 +1345,10 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
 
   /* If the return column is not RIP, SFrame format cannot represent it.  */
   if (xlate_ctx->dw_fde->return_column != DWARF2_DEFAULT_RETURN_COLUMN)
-    return SFRAME_XLATE_ERR_NOTREPRESENTED;
+    {
+      as_warn (_("skipping SFrame FDE due to non-default DWARF return column"));
+      return SFRAME_XLATE_ERR_NOTREPRESENTED;
+    }
 
   /* Iterate over the CFIs and create SFrame FREs.  */
   for (cfi_insn = dw_fde->data; cfi_insn; cfi_insn = cfi_insn->next)
@@ -1355,7 +1358,8 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
       if (err != SFRAME_XLATE_OK)
 	{
 	  /* Skip generating SFrame stack trace info for the function if any
-	     offending CFI is encountered by sframe_do_cfi_insn ().  */
+	     offending CFI is encountered by sframe_do_cfi_insn ().  Warning
+	     message already printed by sframe_do_cfi_insn ().  */
 	  return err; /* Return the error code.  */
 	}
     }
diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-3.d b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
index 5914c620760d..d17521dd88ea 100644
--- a/gas/testsuite/gas/cfi-sframe/common-empty-3.d
+++ b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
@@ -1,4 +1,5 @@
 #as: --gsframe
+#warning: skipping SFrame FDE due to non-default DWARF return column
 #objdump: --sframe=.sframe
 #name: SFrame supports only default return column
 #...
-- 
2.40.1

[PATCH v2 8/9] gas: User readable warnings if SFrame FDE is not generated

[Jens Remus]

The following generic warning message, which is printed whenever the
assembler skips generation of SFrame FDE, is not very helpful for the
user:

  skipping SFrame FDE due to DWARF CFI op <name> (0x<hexval>)

Whenever possible print meaningful warning messages, when the assembler
skips generation of SFrame FDE:

- skipping SFrame FDE due to .cfi_def_cfa specifying CFA base register
  other than SP or FP (<regno> instead of <SP-regno> or <FP-regno>)
- skipping SFrame FDE due to .cfi_def_cfa_register specifying CFA base
  register other than SP or FP (<regno> instead of <SP-regno> or
  <FP-regno>)
- skipping SFrame FDE due to .cfi_def_cfa_offset without CFA base
  register in effect
- skipping SFrame FDE due to .cfi_def_cfa_offset while CFA base register
  other than SP or FP in effect (<regno> instead of <SP-regno> or
  <FP-regno>)
- skipping SFrame FDE due to .cfi_val_offset specifying {FP|RA} register
  (<regno>)
- skipping SFrame FDE due to .cfi_remember_state without SFrame FRE
  state
- skipping SFrame FDE due to .cfi_register specifying {SP|FP|RA}
  register (<regno>)
- skipping SFrame FDE due to non-default DWARF return column (<regno>
  instead of <DWARF-def-ret-col-regno>)

gas/
	* gen-sframe.h (SFRAME_FRE_BASE_REG_INVAL): New macro for
	  invalid SFrame FRE CFA base register value of -1.
	* gen-sframe.c: User readable warnings if SFrame FDE is not
	  generated.

Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
 gas/gen-sframe.c                              | 96 ++++++++++++++-----
 gas/gen-sframe.h                              |  2 +
 gas/testsuite/gas/cfi-sframe/common-empty-1.d |  2 +-
 gas/testsuite/gas/cfi-sframe/common-empty-2.d |  2 +-
 gas/testsuite/gas/cfi-sframe/common-empty-3.d |  2 +-
 5 files changed, 79 insertions(+), 25 deletions(-)

diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index 28b49a2a8425..339f4412ca05 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -867,7 +867,7 @@ sframe_row_entry_new (void)
   struct sframe_row_entry *fre = XCNEW (struct sframe_row_entry);
   /* Reset cfa_base_reg to -1.  A value of 0 will imply some valid register
      for the supported arches.  */
-  fre->cfa_base_reg = -1;
+  fre->cfa_base_reg = SFRAME_FRE_BASE_REG_INVAL;
   fre->merge_candidate = true;
   /* Reset the mangled RA status bit to zero by default.  We will initialize it in
      sframe_row_entry_initialize () with the sticky bit if set.  */
@@ -922,6 +922,24 @@ sframe_row_entry_initialize (struct sframe_row_entry *cur_fre,
   cur_fre->mangled_ra_p = prev_fre->mangled_ra_p;
 }
 
+/* Return SFrame register name for SP, FP, and RA, or NULL if other.  */
+
+static const char *
+sframe_register_name (unsigned int reg)
+{
+  if (reg == SFRAME_CFA_SP_REG)
+    return "SP";
+  else if (reg == SFRAME_CFA_FP_REG)
+    return "FP";
+#ifdef SFRAME_FRE_RA_TRACKING
+  else if (sframe_ra_tracking_p ()
+	   && (reg == SFRAME_CFA_RA_REG))
+    return "RA";
+#endif
+  else
+    return NULL;
+}
+
 /* Translate DW_CFA_advance_loc into SFrame context.
    Return SFRAME_XLATE_OK if success.  */
 
@@ -990,7 +1008,12 @@ sframe_xlate_do_def_cfa (struct sframe_xlate_ctx *xlate_ctx,
      SFrame stack trace info for the function.  */
   if (cfi_insn->u.r != SFRAME_CFA_SP_REG
       && cfi_insn->u.r != SFRAME_CFA_FP_REG)
-    return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
+    {
+      as_warn (_("skipping SFrame FDE due to .cfi_def_cfa specifying CFA base "
+		 "register other than SP or FP (%u instead of %u or %u)"),
+	       cfi_insn->u.r, SFRAME_CFA_SP_REG, SFRAME_CFA_FP_REG);
+      return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
+    }
   sframe_fre_set_cfa_base_reg (cur_fre, cfi_insn->u.ri.reg);
   sframe_fre_set_cfa_offset (cur_fre, cfi_insn->u.ri.offset);
   cur_fre->merge_candidate = false;
@@ -1015,7 +1038,12 @@ sframe_xlate_do_def_cfa_register (struct sframe_xlate_ctx *xlate_ctx,
      skip creating SFrame stack trace info for the function.  */
   if (cfi_insn->u.r != SFRAME_CFA_SP_REG
       && cfi_insn->u.r != SFRAME_CFA_FP_REG)
-    return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
+    {
+      as_warn (_("skipping SFrame FDE due to .cfi_def_cfa_register specifying "
+		 "CFA base register other than SP or FP (%u instead of %u or %u)"),
+	       cfi_insn->u.r, SFRAME_CFA_SP_REG, SFRAME_CFA_FP_REG);
+      return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
+    }
   sframe_fre_set_cfa_base_reg (cur_fre, cfi_insn->u.ri.reg);
   sframe_fre_set_cfa_offset (cur_fre, last_fre->cfa_offset);
   cur_fre->merge_candidate = false;
@@ -1046,7 +1074,16 @@ sframe_xlate_do_def_cfa_offset (struct sframe_xlate_ctx *xlate_ctx,
       cur_fre->merge_candidate = false;
     }
   else
-    return SFRAME_XLATE_ERR_NOTREPRESENTED;
+    {
+      if (cur_fre->cfa_base_reg == SFRAME_FRE_BASE_REG_INVAL)
+	as_warn (_("skipping SFrame FDE due to .cfi_def_cfa_offset without CFA "
+		   "base register in effect"));
+      else
+	as_warn (_("skipping SFrame FDE due to .cfi_def_cfa_offset while CFA "
+		   "base register other than SP or FP in effect (%u instead of %u or %u)"),
+		 cur_fre->cfa_base_reg, SFRAME_CFA_SP_REG, SFRAME_CFA_FP_REG);
+      return SFRAME_XLATE_ERR_NOTREPRESENTED;
+    }
 
   return SFRAME_XLATE_OK;
 }
@@ -1097,11 +1134,19 @@ sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
      instruction can be safely skipped without sacrificing the asynchronicity of
      stack trace information.  */
   if (cfi_insn->u.r == SFRAME_CFA_FP_REG)
-    return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
+    {
+      as_warn (_("skipping SFrame FDE due to .cfi_val_offset specifying FP register (%u)"),
+	       SFRAME_CFA_FP_REG);
+      return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
+    }
 #ifdef SFRAME_FRE_RA_TRACKING
   else if (sframe_ra_tracking_p ()
 	   && cfi_insn->u.r == SFRAME_CFA_RA_REG)
-    return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
+    {
+      as_warn (_("skipping SFrame FDE due to .cfi_val_offset specifying RA register (%u)"),
+	       SFRAME_CFA_RA_REG);
+      return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
+    }
 #endif
 
   /* Safe to skip.  */
@@ -1120,7 +1165,10 @@ sframe_xlate_do_remember_state (struct sframe_xlate_ctx *xlate_ctx)
      early with non-zero error code, this will cause no SFrame stack trace
      info for the function involved.  */
   if (!last_fre)
-    return SFRAME_XLATE_ERR_INVAL;
+    {
+      as_warn (_("skipping SFrame FDE due to .cfi_remember_state without SFrame FRE state"));
+      return SFRAME_XLATE_ERR_INVAL;
+    }
 
   if (!xlate_ctx->remember_fre)
     xlate_ctx->remember_fre = sframe_row_entry_new ();
@@ -1307,7 +1355,12 @@ sframe_do_cfi_insn (struct sframe_xlate_ctx *xlate_ctx,
 	  || cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG
 #endif
 	  || cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
-	err = SFRAME_XLATE_ERR_NOTREPRESENTED;
+	{
+	  as_warn (_("skipping SFrame FDE due to .cfi_register specifying %s register (%u)"),
+		   sframe_register_name (cfi_insn->u.rr.reg1),
+		   cfi_insn->u.rr.reg1);
+	  err = SFRAME_XLATE_ERR_NOTREPRESENTED;
+	}
       break;
     case DW_CFA_undefined:
     case DW_CFA_same_value:
@@ -1315,21 +1368,19 @@ sframe_do_cfi_insn (struct sframe_xlate_ctx *xlate_ctx,
     default:
       /* Following skipped operations do, however, impact the asynchronicity:
 	  - CFI_escape.  */
-      err = SFRAME_XLATE_ERR_NOTREPRESENTED;
-    }
-
-  /* An error here will cause no SFrame FDE later.  Warn the user because this
-     will affect the overall coverage and hence, asynchronicity.  */
-  if (err)
-    {
-      const char *cfi_name = sframe_get_cfi_name (op);
-
-      if (!cfi_name)
-	cfi_name = _("(unknown)");
-      as_warn (_("skipping SFrame FDE due to DWARF CFI op %s (%#x)"),
-	       cfi_name, op);
+      {
+	const char *cfi_name = sframe_get_cfi_name (op);
+
+	if (!cfi_name)
+	  cfi_name = _("(unknown)");
+	as_warn (_("skipping SFrame FDE due to DWARF CFI op %s (%#x)"),
+		 cfi_name, op);
+        err = SFRAME_XLATE_ERR_NOTREPRESENTED;
+      }
     }
 
+  /* Any error will cause no SFrame FDE later.  The user has already been
+     warned.  */
   return err;
 }
 
@@ -1346,7 +1397,8 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
   /* If the return column is not RIP, SFrame format cannot represent it.  */
   if (xlate_ctx->dw_fde->return_column != DWARF2_DEFAULT_RETURN_COLUMN)
     {
-      as_warn (_("skipping SFrame FDE due to non-default DWARF return column"));
+      as_warn (_("skipping SFrame FDE due to non-default DWARF return column (%u instead of %u)"),
+	       xlate_ctx->dw_fde->return_column, DWARF2_DEFAULT_RETURN_COLUMN);
       return SFRAME_XLATE_ERR_NOTREPRESENTED;
     }
 
diff --git a/gas/gen-sframe.h b/gas/gen-sframe.h
index fbe2fd5d9368..8ed46dbb087b 100644
--- a/gas/gen-sframe.h
+++ b/gas/gen-sframe.h
@@ -24,6 +24,8 @@
 #define SFRAME_FRE_ELEM_LOC_REG		0
 #define SFRAME_FRE_ELEM_LOC_STACK	1
 
+#define SFRAME_FRE_BASE_REG_INVAL	((unsigned int)-1)
+
 /* SFrame Frame Row Entry (FRE).
 
    A frame row entry is a slice of the frame and can be valid for a set of
diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-1.d b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
index d7756302b559..08731b069229 100644
--- a/gas/testsuite/gas/cfi-sframe/common-empty-1.d
+++ b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
@@ -1,5 +1,5 @@
 #as: --gsframe
-#warning: skipping SFrame FDE due to DWARF CFI op DW_CFA_remember_state \(0xa\)
+#warning: skipping SFrame FDE due to \.cfi_remember_state without SFrame FRE state
 #objdump: --sframe=.sframe
 #name: Uninteresting cfi directives generate an empty SFrame section
 #...
diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-2.d b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
index 20282c7854e8..e759cddfcc20 100644
--- a/gas/testsuite/gas/cfi-sframe/common-empty-2.d
+++ b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
@@ -1,5 +1,5 @@
 #as: --gsframe
-#warning: skipping SFrame FDE due to DWARF CFI op DW_CFA_def_cfa_offset \(0xe\)
+#warning: skipping SFrame FDE due to \.cfi_def_cfa_offset without CFA base register in effect
 #objdump: --sframe=.sframe
 #name: SFrame supports only FP/SP based CFA
 #...
diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-3.d b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
index d17521dd88ea..9a46b016a9ba 100644
--- a/gas/testsuite/gas/cfi-sframe/common-empty-3.d
+++ b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
@@ -1,5 +1,5 @@
 #as: --gsframe
-#warning: skipping SFrame FDE due to non-default DWARF return column
+#warning: skipping SFrame FDE due to non-default DWARF return column \(0 instead of \d+\)
 #objdump: --sframe=.sframe
 #name: SFrame supports only default return column
 #...
-- 
2.40.1

[RFC PATCH 9/9] s390: Initial support to generate .sframe from CFI directives in assembler

[Jens Remus]

This introduces initial support to generate .sframe from CFI directives
in assembler on s390x. Due to the following SFrame limitations it is
incomplete and does not work for most real-world applications (i.e.
generation of SFrame FDE is skipped):

- SFrame FP/RA tracking assumes the register contents to be 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
  other registers (e.g. floating-point registers).

- SFrame assumes a static RA register number. While the s390x ELF ABI
  [1] specifies register 14 to contain the return address on entry,
  GCC can be observed to copy the return address to another register
  and may even use that to return. Additionally glibc on s390x has
  two functions (mcount and fentry) that specify register 0 to hold
  the return address. This would either require a change to glibc (if
  possible) or SFrame support to track the RA register number specified
  in CFI directive .cfi_return_column.

- SFrame assumes a static FP register number. The s390x ELF ABI [1]
  does not specify any FP register number. GCC and clang on s390x
  usually use register 11 as frame pointer. GCC on s390x can also be
  observed to use register 14 (e.g. binutils and glibc builds). glibc
  on s390x contains code that uses register 12 for the frame pointer.
  This would require SFrame support to track the SP/FP register number
  specified in CFI directives .cfi_def_cfa and .cfi_def_cfa_register.

- SFrame does not support CFI directive .cfi_val_offset. glibc on s390x
  has two functions (mcount and fentry), that make use of that CFI
  directive. This would either require a change in glibc (if possible)
  or SFrame to support CFI directive .cfi_val_offset.

This s390x support is largely based on the AArch64 support from commit
b52c4ee46657 ("gas: generate .sframe from CFI directives").

SFrame ABI/arch identifier SFRAME_ABI_S390_ENDIAN_BIG is introduced
for s390 and added to the SFrame specification.

According to the s390x ELF ABI [1] the following calling conventions
are used on s390x architecture:
- Register 15 is used as stack pointer (SP). The CFA is initially
  located at offset +160 from the SP.
- Register 14 is used as return address (RA).
- There is no dedicated frame pointer. GCC and LLVM currently use
  register 11 as frame pointer.

The s390x ELF ABI [1] standard stack frame layout has the following
conventions:
- The return address (RA, r15) may be saved at offset -40 from the
  CFA. Unlike x86 AMD64 architecture it is not necessarily saved on
  the stack. Also compilers may use a non-standard stack frame layout,
  such as but not limited to GCC with option -mpacked-stack.
  Therefore SFrame RA tracking is used.
- The potential frame pointer (FP, r11) may be saved at offset -72
  from the CFA. It is not necessarily saved on the stack and compilers
  may use a non-standard stack frame layout (see above).
  Therefore SFrame FP tracking is used.

Support for SFrame is only enabled for z/Architecture (s390x) with
64-bit architecture mode. It is disabled for 32-bit architecture mode
and ESA/390 (s390).

Add s390-specific SFrame test cases. As for the error test cases add
ones that use a non-default frame pointer (FP) register number and ones
that save the return address (RA) in a non-default RA register number.

[1] ELF ABI s390x Supplement:
    https://github.com/IBM/s390x-abi/releases
[2] ELF ABI s390 Supplement:
    https://refspecs.linuxfoundation.org/ELF/zSeries/lzsabi0_s390.html
    https://refspecs.linuxfoundation.org/ELF/zSeries/lzsabi0_s390.pdf

include/
	* sframe.h (SFRAME_ABI_S390_ENDIAN_BIG): Define SFrame ABI/arch
	  identifier for s390x. Reference s390x architecture in comments.

libsframe/
	* doc/sframe-spec.texi: Document SFrame ABI/arch identifier for
	  s390x and reference s390x architecture.

gas/
	* config/tc-s390.h: s390x support to generate .sframe from CFI
	  directives in assembler.
	* config/tc-s390.c: Likewise.
	* gen-sframe.c: Reference s390x architecture in comments.

gas/testsuite/
	* gas/cfi-sframe/cfi-sframe.exp: Enable common SFrame test cases
	  for s390x. Add s390-specific SFrame test cases.
	* gas/cfi-sframe/cfi-sframe-s390-1.s: New s390-specific SFrame
	  test case.
	* gas/cfi-sframe/cfi-sframe-s390-1.d: Likewise.
	* gas/cfi-sframe/cfi-sframe-s390-2.s: Likewise.
	* gas/cfi-sframe/cfi-sframe-s390-2.d: Likewise.
	* gas/cfi-sframe/cfi-sframe-s390-err-1.s: New s390-specific
	  SFrame error test case that uses a non-default register number
	  for the frame pointer.
	* gas/cfi-sframe/cfi-sframe-s390-err-1.l: Likewise.
	* gas/cfi-sframe/cfi-sframe-s390-err-2.s: Likewise.
	* gas/cfi-sframe/cfi-sframe-s390-err-2.l: Likewise.
	* gas/cfi-sframe/cfi-sframe-s390-err-3.s: New s390-specific
	  SFrame error test case that uses a non-default register number
	  to save the return address.
	* gas/cfi-sframe/cfi-sframe-s390-err-3.l: Likewise.
	* gas/cfi-sframe/cfi-sframe-s390-err-4.s: New s390-specific
	  SFrame error test case that used a non-default return column
	  (return address) register number.
	* gas/cfi-sframe/cfi-sframe-s390-err-4.l: Likewise.

Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---

Notes (jremus):
    The SFrame support for s390x provided by this patch still has some open
    issues, which need to be addressed. Any ideas or assistance to overcome
    the SFrame limitations listed in the commit description are very
    welcome.
    
    Note that unlike the AArch64 and x68 AMD64 implementation this s390x
    implementation does statically initialize the architecture-dependent
    variables s390_sframe_cfa_{sp|fp|ra}_reg, which are referenced by the
    SFrame interface macros SFRAME_CFA_{SP|FP|RA}_REG. The other
    implementations do initialize them in md_begin. I verified that all
    occurrences of the macros SFRAME_CFA_{SP|FP|RA}_REG are in context of
    test/read and never write.
    Is there a reason to define the SFrame interface macros
    SFRAME_CFA_{SP|FP|RA}_REG to variables? For instance should the SFrame
    common code be able to modify these? Currently I do not see how it
    would work well, if the architecture-specific code would change these
    at run time. Similar for the predicate sframe_ra_tracking_p.
    
    Example #1: Warnings when compiling binutils with SFrame
    
    $ ../configure CC="gcc -B$HOME/temp/binutils-gcc/bin -Wa,--gsframe" \
      CXX="g++ -B$HOME/temp/binutils-gcc/bin -Wa,--gsframe" \
      --prefix=$HOME/temp/binutils-sframe
    $ make -j $(nprocs) 2>&1 | tee make.log
    $ grep --only-matching "Warning:.*" make.log | sort | uniq -c
          1 Warning: skipping SFrame FDE due to .cfi_def_cfa specifying CFA
            base register other than SP or FP (14 instead of 15 or 11)
        205 Warning: skipping SFrame FDE due to .cfi_register specifying FP
            register (11)
         56 Warning: skipping SFrame FDE due to .cfi_register specifying SP
            register (15)
    
    Example #2: Warnings when compiling glibc with SFrame
    
    $ ../configure CC="gcc -B$HOME/temp/binutils-sframe/bin -Wa,--gsframe" \
      CXX="g++ -B$HOME/temp/binutils-sframe/bin -Wa,--gsframe" \
      --prefix=$HOME/temp/glibc-sframe
    $ make -j $(nprocs) 2>&1 | tee make.log
    $ grep --only-matching "Warning:.*" make.log | sort | uniq -c
          2 Warning: skipping SFrame FDE due to .cfi_def_cfa_register
            specifying CFA base register other than SP or FP (12 instead of
            15 or 11)
          7 Warning: skipping SFrame FDE due to .cfi_def_cfa specifying CFA
            base register other than SP or FP (14 instead of 15 or 11)
        225 Warning: skipping SFrame FDE due to .cfi_register specifying FP
            register (11)
        187 Warning: skipping SFrame FDE due to .cfi_register specifying SP
            register (15)
          2 Warning: skipping SFrame FDE due to DWARF CFI op CFI_escape
            (0x103)
          2 Warning: skipping SFrame FDE due to non-default DWARF return
            column (0 instead of 14)
    
    .cfi_def_cfa 14, ... originates from GCC generated code, which uses
    register %r14 as frame pointer (FP).
    
    .cfi_def_cfa_register 12 originates from hand-written assembler code
    sysdeps/s390/s390-64/dl-trampoline.S, which uses register %r12 as frame
    pointer (FP).
    
    .cfi_return_column 0 and .cfi_escape both originate from hand-written
    assembler code sysdeps/s390/s390-64/s390x-mcount.S, which is compiled
    twice (with and without -DSHARED).
    
    - .cfi_escape 0x14, 0x15, 0x14: Is used to code
      ".cfi_val_offset r15, -160", which would require binutils 2.28+ (glibc
      currently supports a minimum binutils 2.25). This CFI directive states
      that the contents of register %r15 are CFA-160 (not to be confused
      with saved at CFA-160).
    
    - .cfi_return_column 0: The following comment explains why register %r0
      contains the return address:
    
      "The _mcount implementation now has to call __mcount_internal with the
      address of .LP0 as first parameter and the return address as second
      parameter. &.LP0 was loaded to %r1 and the return address is in %r14.
      _mcount may not modify any register.
    
      Alternatively, at the start of each function __fentry__ is called
      using a single
    
              brasl  0,__fentry__
    
      instruction.  In this case %r0 points to the callee, and %r14 points
      to the caller.  These values need to be passed to __mcount_internal
      using the same sequence as for _mcount, so the code below is shared
      between both functions.
      The only major difference is that __fentry__ cannot return through
      %r0, in which the return address is located, because br instruction
      is a no-op with this register.  Therefore %r1, which is clobbered by
      the PLT anyway, is used."
    
      Excerpt of the relevant glibc source code:
    
              .globl C_SYMBOL_NAME(MCOUNT_SYMBOL)
              .type C_SYMBOL_NAME(MCOUNT_SYMBOL), @function
              cfi_startproc
              .align ALIGNARG(4)
      C_LABEL(MCOUNT_SYMBOL)
              cfi_return_column (glue(r, MCOUNT_CALLEE_REG))
              /* Save the caller-clobbered registers.  */
              aghi  %r15,-224
              cfi_adjust_cfa_offset (224)
              /* binutils 2.28+: .cfi_val_offset r15, -160 */
              .cfi_escape \
                      /* DW_CFA_val_offset */ 0x14, \
                      /* r15 */               0x0f, \
                      /* scaled offset */     0x14
              stmg  %r14,%r5,160(%r15)
    
              cfi_offset (r14, -224)
              cfi_offset (r0, -224+16)
      ...

 gas/config/tc-s390.c                          | 55 +++++++++++++++++++
 gas/config/tc-s390.h                          | 31 +++++++++++
 gas/gen-sframe.c                              |  2 +-
 .../gas/cfi-sframe/cfi-sframe-s390-1.d        | 23 ++++++++
 .../gas/cfi-sframe/cfi-sframe-s390-1.s        | 37 +++++++++++++
 .../gas/cfi-sframe/cfi-sframe-s390-2.d        | 23 ++++++++
 .../gas/cfi-sframe/cfi-sframe-s390-2.s        | 37 +++++++++++++
 .../gas/cfi-sframe/cfi-sframe-s390-err-1.d    | 15 +++++
 .../gas/cfi-sframe/cfi-sframe-s390-err-1.s    | 37 +++++++++++++
 .../gas/cfi-sframe/cfi-sframe-s390-err-2.d    | 15 +++++
 .../gas/cfi-sframe/cfi-sframe-s390-err-2.s    | 37 +++++++++++++
 .../gas/cfi-sframe/cfi-sframe-s390-err-3.d    | 15 +++++
 .../gas/cfi-sframe/cfi-sframe-s390-err-3.s    |  6 ++
 .../gas/cfi-sframe/cfi-sframe-s390-err-4.d    | 15 +++++
 .../gas/cfi-sframe/cfi-sframe-s390-err-4.s    |  5 ++
 gas/testsuite/gas/cfi-sframe/cfi-sframe.exp   | 13 ++++-
 include/sframe.h                              |  7 ++-
 libsframe/doc/sframe-spec.texi                |  8 ++-
 18 files changed, 374 insertions(+), 7 deletions(-)
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-1.d
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-1.s
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-2.d
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-2.s
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-1.d
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-1.s
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-2.d
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-2.s
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-3.d
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-3.s
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-4.d
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-4.s

diff --git a/gas/config/tc-s390.c b/gas/config/tc-s390.c
index 09a903aea2db..af0d705b63f7 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"
@@ -83,6 +85,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.  */
+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 */
@@ -2639,6 +2652,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.  */
+
+unsigned char
+s390_sframe_get_abi_arch (void)
+{
+  unsigned char sframe_abi_arch = 0;
+
+  if (s390_support_sframe_p ())
+    {
+      gas_assert (target_big_endian);
+      sframe_abi_arch = SFRAME_ABI_S390_ENDIAN_BIG;
+    }
+
+  return sframe_abi_arch;
+}
+
 void
 s390_elf_final_processing (void)
 {
diff --git a/gas/config/tc-s390.h b/gas/config/tc-s390.h
index cd353045a822..c8f172d8737b 100644
--- a/gas/config/tc-s390.h
+++ b/gas/config/tc-s390.h
@@ -98,3 +98,34 @@ extern int s390_cie_data_alignment;
 extern void s390_elf_final_processing (void);
 
 #define elf_tc_final_processing s390_elf_final_processing
+
+/* SFrame  */
+
+/* Whether SFrame stack trace info is supported.  */
+extern bool s390_support_sframe_p (void);
+#define support_sframe_p s390_support_sframe_p
+
+/* The stack-pointer register number for SFrame stack trace info.  */
+extern unsigned int s390_sframe_cfa_sp_reg;
+#define SFRAME_CFA_SP_REG s390_sframe_cfa_sp_reg
+
+/* The frame-pointer register number for SFrame stack trace info.  */
+extern unsigned int s390_sframe_cfa_fp_reg;
+#define SFRAME_CFA_FP_REG s390_sframe_cfa_fp_reg
+
+/* The return address register number for SFrame stack trace info.  */
+extern unsigned int s390_sframe_cfa_ra_reg;
+#define SFRAME_CFA_RA_REG s390_sframe_cfa_ra_reg
+
+/* Specify if RA tracking is needed.  */
+extern bool s390_sframe_ra_tracking_p (void);
+#define sframe_ra_tracking_p s390_sframe_ra_tracking_p
+
+/* Specify the fixed offset to recover RA from CFA.
+   (useful only when RA tracking is not needed).  */
+extern offsetT s390_sframe_cfa_ra_offset (void);
+#define sframe_cfa_ra_offset s390_sframe_cfa_ra_offset
+
+/* The abi/arch indentifier for SFrame.  */
+unsigned char s390_sframe_get_abi_arch (void);
+#define sframe_get_abi_arch s390_sframe_get_abi_arch
diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index 339f4412ca05..08849cdafa81 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -673,7 +673,7 @@ output_sframe_internal (void)
 #endif
   out_one (fixed_ra_offset);
 
-  /* None of the AMD64, or AARCH64 ABIs need the auxiliary header.
+  /* None of the AMD64, AARCH64, or S390 ABIs need the auxiliary header.
      When the need does arise to use this field, the appropriate backend
      must provide this information.  */
   out_one (0); /* Auxiliary SFrame header length.  */
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
index 000000000000..211804a2309a
--- /dev/null
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-1.d
@@ -0,0 +1,23 @@
+#objdump: --sframe=.sframe
+#name: SFrame generation on s390
+#...
+Contents of the SFrame section .sframe:
+
+  Header :
+
+    Version: SFRAME_VERSION_2
+    Flags: NONE
+    Num FDEs: 1
+    Num FREs: 6
+
+  Function Index :
+
+    func idx \[0\]: pc = 0x0, size = 40 bytes
+    STARTPC +CFA +FP +RA +
+    0+0000 +sp\+160 +u +u +
+    0+0006 +sp\+160 +c\-72 +c\-48 +
+    0+000c +sp\+336 +c\-72 +c\-48 +
+    0+0010 +fp\+336 +c\-72 +c\-48 +
+    0+001c +sp\+160 +u +u +
+    0+001e +fp\+336 +c\-72 +c\-48 +
+#pass
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-1.s b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-1.s
new file mode 100644
index 000000000000..56d8425ae2b1
--- /dev/null
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-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,-176(%r15)
+	.cfi_def_cfa_offset 336
+	lgr	%r11,%r15
+	.cfi_def_cfa_register 11
+	lay	%r15,-128(%r15)
+.Lreturn:
+	lmg	%r6,%r15,224(%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)
+	j	.Lreturn
+	.cfi_endproc
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-2.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-2.d
new file mode 100644
index 000000000000..211804a2309a
--- /dev/null
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-2.d
@@ -0,0 +1,23 @@
+#objdump: --sframe=.sframe
+#name: SFrame generation on s390
+#...
+Contents of the SFrame section .sframe:
+
+  Header :
+
+    Version: SFRAME_VERSION_2
+    Flags: NONE
+    Num FDEs: 1
+    Num FREs: 6
+
+  Function Index :
+
+    func idx \[0\]: pc = 0x0, size = 40 bytes
+    STARTPC +CFA +FP +RA +
+    0+0000 +sp\+160 +u +u +
+    0+0006 +sp\+160 +c\-72 +c\-48 +
+    0+000c +sp\+336 +c\-72 +c\-48 +
+    0+0010 +fp\+336 +c\-72 +c\-48 +
+    0+001c +sp\+160 +u +u +
+    0+001e +fp\+336 +c\-72 +c\-48 +
+#pass
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-2.s b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-2.s
new file mode 100644
index 000000000000..4d58cdaf64a7
--- /dev/null
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-2.s
@@ -0,0 +1,37 @@
+	.cfi_sections .sframe
+	.cfi_startproc
+	stmg	%r6,%r15,48(%r15)
+	.cfi_rel_offset 6, 48
+	.cfi_rel_offset 7, 56
+	.cfi_rel_offset 8, 64
+	.cfi_rel_offset 9, 72
+	.cfi_rel_offset 10, 80
+	.cfi_rel_offset 11, 88
+	.cfi_rel_offset 12, 96
+	.cfi_rel_offset 13, 104
+	.cfi_rel_offset 14, 112
+	.cfi_rel_offset 15, 120
+	lay	%r15,-176(%r15)
+	.cfi_def_cfa_offset 336
+	lgr	%r11,%r15
+	.cfi_def_cfa_register 11
+	lay	%r15,-128(%r15)
+.Lreturn:
+	lmg	%r6,%r15,224(%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)
+	j	.Lreturn
+	.cfi_endproc
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-1.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-1.d
new file mode 100644
index 000000000000..a2724802c04b
--- /dev/null
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-1.d
@@ -0,0 +1,15 @@
+#name: SFrame generation on s390
+#as: --gsframe
+#warning: skipping SFrame FDE due to .cfi_def_cfa_register specifying CFA base register other than SP or FP \(10 instead of 15 or 11\)
+#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-s390-err-1.s b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-1.s
new file mode 100644
index 000000000000..85e36c39d2a3
--- /dev/null
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-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,-176(%r15)
+	.cfi_def_cfa_offset 336
+	lgr	%r10,%r15
+	.cfi_def_cfa_register 10	# non-default frame pointer register
+	lay	%r15,-128(%r15)
+.Lreturn:
+	lmg	%r6,%r15,224(%r10)
+	.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)
+	j	.Lreturn
+	.cfi_endproc
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-2.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-2.d
new file mode 100644
index 000000000000..914f01ad9ffe
--- /dev/null
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-2.d
@@ -0,0 +1,15 @@
+#name: SFrame generation on s390
+#as: --gsframe
+#warning: skipping SFrame FDE due to .cfi_def_cfa specifying CFA base register other than SP or FP \(10 instead of 15 or 11\)
+#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-s390-err-2.s b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-2.s
new file mode 100644
index 000000000000..0bb274a9e991
--- /dev/null
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-2.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,-176(%r15)
+	.cfi_def_cfa_offset 336
+	lgr	%r10,%r15
+	.cfi_def_cfa 10, 336	# non-default frame pointer register
+	lay	%r15,-128(%r15)
+.Lreturn:
+	lmg	%r6,%r15,224(%r10)
+	.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)
+	j	.Lreturn
+	.cfi_endproc
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-3.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-3.d
new file mode 100644
index 000000000000..54a7252f81a3
--- /dev/null
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-3.d
@@ -0,0 +1,15 @@
+#name: SFrame generation on s390
+#as: --gsframe
+#warning: skipping SFrame FDE due to .cfi_register specifying RA register \(14\)
+#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-s390-err-3.s b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-3.s
new file mode 100644
index 000000000000..f0a5bd577304
--- /dev/null
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-3.s
@@ -0,0 +1,6 @@
+	.cfi_sections .sframe
+	.cfi_startproc
+	lgr	%r7,%r14
+	.cfi_register 14, 7	# non-default return address register
+	br	%r7
+	.cfi_endproc
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-4.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-4.d
new file mode 100644
index 000000000000..71769e7cf077
--- /dev/null
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-4.d
@@ -0,0 +1,15 @@
+#name: SFrame generation on s390
+#as: --gsframe
+#warning: skipping SFrame FDE due to non-default DWARF return column \(7 instead of 14\)
+#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-s390-err-4.s b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-4.s
new file mode 100644
index 000000000000..cfdea75e0b95
--- /dev/null
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-s390-err-4.s
@@ -0,0 +1,5 @@
+	.cfi_sections .sframe
+	.cfi_startproc
+	.cfi_return_column 7	# non-default return address register
+	br	%r7
+	.cfi_endproc
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp b/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
index cfae28d39aaf..9411c0e6c9ce 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*-*-*"] ||
+       [istarget "s390x-*-*"]) \
        && [gas_sframe_check] } then {
 
     global ASFLAGS
@@ -99,3 +100,13 @@ if { [istarget "aarch64*-*-*"] && [gas_sframe_check] } then {
     run_dump_test "cfi-sframe-aarch64-2"
     run_dump_test "cfi-sframe-aarch64-pac-ab-key-1"
 }
+
+# s390 specific tests
+if { [istarget "s390x*-*-*"] && [gas_sframe_check] } then {
+    run_dump_test "cfi-sframe-s390-1"
+    run_dump_test "cfi-sframe-s390-2"
+    run_dump_test "cfi-sframe-s390-err-1"
+    run_dump_test "cfi-sframe-s390-err-2"
+    run_dump_test "cfi-sframe-s390-err-3"
+    run_dump_test "cfi-sframe-s390-err-4"
+}
diff --git a/include/sframe.h b/include/sframe.h
index b3d0c2e937d3..90bc92a32f84 100644
--- 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.  */
 
 /* 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.
 
    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
diff --git a/libsframe/doc/sframe-spec.texi b/libsframe/doc/sframe-spec.texi
index d0f99bd698eb..a81faf49f5c1 100644
--- a/libsframe/doc/sframe-spec.texi
+++ b/libsframe/doc/sframe-spec.texi
@@ -74,8 +74,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.
 
 A portion of the SFrame format follows an unaligned on-disk representation.
 Some data structures, however, (namely the SFrame header and the SFrame
@@ -366,6 +366,10 @@ in the format.
 @item @code{SFRAME_ABI_AMD64_ENDIAN_LITTLE}
 @tab 3 @tab AMD64 little-endian
 
+@tindex SFRAME_ABI_S390_ENDIAN_BIG
+@item @code{SFRAME_ABI_S390_ENDIAN_BIG}
+@tab 4 @tab s390 big-endian
+
 @end multitable
 
 The presence of an explicit identification of ABI/arch in SFrame may allow
-- 
2.40.1

Re: [RFC PATCH 0/9] s390: Initial support to generate SFrame info from CFI directives in assembler

[Jens Remus]

Hi,

please excuse that I accidentally dropped the "v2" from the subjects of 
the cover letter and patch 9 in this thread when manually adding the 
"RFC" to both.

Regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303) and z/VSE Support
+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/

Re: [RFC PATCH 0/9] s390: Initial support to generate SFrame info from CFI directives in assembler

[Indu Bhagat]

Hi Jens,

On 2/23/24 09:07, Jens Remus wrote:
> This patch series adds initial support to the assembler on s390x to
> generate SFrame stack trace information from CFI directives. It is
> largely based on the respective AArch64 support.
> 
> Please be aware that the SFrame support for s390x provided by patch 9
> of this series still has some open issues, which need to be addressed.
> Any ideas or assistance to overcome the current SFrame limitations
> listed below and in the patch description are very welcome.
> 
> Patches 1-8 are generic cleanups and enhancements to the generation
> of SFrame information in the assembler. Patch 9 adds the initial s390x
> support and is purely RFC.
> 
> 
> Patches 1-3 are minor cleanups/enhancements to the existing SFrame
> support on AArch64 and x86 AMD64.
> 
> Patch 4 enables readelf/objdump to dump the SFrame fixed offsets from
> CFA to the frame pointer (FP) and return address (RA).
> 
> Patch 5 enhances an SFrame warning message to print the human readable
> DWARF call frame instruction name.
> 
> Patch 6 and 7 resolve issues that cause the assembler to either generate
> bad SFrame FDE or to silently skip it. Both issues would be triggered by
> s390-specific SFrame error test cases introduced by patch 9.
> 
> Patch 8 adds verbose assembler warning messages when generation of
> SFrame FDE is skipped.
> 

Thanks for fixing issues and enhancing SFrame handling in Binutils.

> Patch 9 adds initial support to the assembler on s390x to generate
> SFrame stack trace information from CFI directives. Due to differences
> in the s390x ELF ABI [1] compared to the AArch64 and x68 AMD64 ELF ABIs
> and the simplified assumptions of the current SFrame format V2 there
> are several unresolved issues (see also patch description):
> 
> - GCC on s390x may save the frame pointer (FP; r11) and/or return
>    address (RA; r14) registers in other registers (e.g. floating-point
>    registers) instead of saving them on the stack. SFrame currently only
>    supports FP/RA tracking on the stack.
>    This can be reproduced by using a small inline assembler statement,
>    that specifies r11 (FP) and/or r14 (RA) as output operand(s). It can
>    also be observed when compiling a minimal C program using GCC without
>    optimizations.
> 
> - The s390x ELF ABI does not specify a frame pointer register number.
>    Currently GCC and clang both mostly use register r11, but GCC can
>    also be observed to use r14. On s390x a compiler may choose about
>    any register number for that purpose and declare it using CFI
>    directives.
>    This can be observed when compiling glibc, as parts of its hand-
>    written assembler uses register r12 as frame pointer.
> 
> - GCC on s390x may copy the return address (RA) from register r14 to
>    another register and even use that to return to the caller.
>    Effectively this is just a specialization of the first bullet.
> 
> If my understanding of the current SFrame format is not wrong I assume
> the following enhancements to SFrame would be required to fully support
> the s390x architecture:
> 
> - Tracking of the CFA base pointer register number specified in CFI
>    directives .cfi_def_cfa and .cfi_def_cfa_register.
> 
> - Tracking of SP, FP, and RA register contents being saved in other
>    registers instead of on the stack, as specified by CFI directive
>    .cfi_register.
> 
> - Both would effectively require support in SFrame to track all of the
>    17 registers specified as saved in the s390x ELF ABI (r6-r13, r15,
>    and f8-f15), since SP, FP, and RA could be saved in one of those and
>    thus would need to be restored during unwinding.
> 

While I am in the process of taking a closer look at the patchset, and 
the requirements of the s390x ABI to see how to best support with 
SFrame, I would like to add here that tracking all of callee-saved 
registers will significantly bloat up the .sframe section.  (As you 
already know, the offsets to recover the registers from stack are stated 
explicitly in the format).  Hence, adapting the SFrame format to track 
all callee-saved registers is not recommended.

> - Potentially optional, if the use in glibc could be removed:
>    Tracking of SP, FP, and RA register contents being CFA+offset, as
>    specified by .cfi_val_offset.
> 
> [1] ELF ABI s390x Supplement:
>      https://github.com/IBM/s390x-abi/releases
> 
> Changes v1 -> v2:
> - Resolved a regression reported by Linaro-TCWG-CI on AArch64 in one of
>    the generic ld SFrame test cases. The test case contained a
>    .cfi_def_cfa directive, specifying a CFA base register number that is
>    not necessarily a SFrame SP/FP register number on all architectures.
>    This caused the changes from patch 6 to skip SFrame FDE generation on
>    AArch64 (and s390x aswell) with a warning, causing the test case to
>    fail.
> 
> Thanks and regards,
> Jens
> 
> Jens Remus (9):
>    x86: Remove unused SFrame CFI RA register variable
>    aarch64: Align SFrame terminology in comments to specs and x86
>    sframe: Enhance comments for SFRAME_CFA_*_REG macros
>    readelf/objdump: Dump SFrame CFA fixed FP and RA offsets
>    gas: Print DWARF call frame insn name in SFrame warning message
>    gas: Skip SFrame FDE if CFI specifies non-FP/SP base register
>    gas: Warn if SFrame FDE is skipped due to non-default return column
>    gas: User readable warnings if SFrame FDE is not generated
>    s390: Initial support to generate .sframe from CFI directives in
>      assembler
> 

Re: [PATCH v2 1/9] x86: Remove unused SFrame CFI RA register variable

[Indu Bhagat]

On 2/23/24 09:07, Jens Remus wrote:
> gas/
> 	* config/tc-i386.c: Remove unused SFrame CFI RA register
> 	  variable.
> 

The ChangeLog formatting is a bit off here and in the rest of the 
patches in this series.

The indentation of each line should be such that all content lines up 
below the *, like so:

<TAB>* xyz.c: first line content
<TAB>next line content

Other than that, LGTM.

Thanks
Indu

> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
>   gas/config/tc-i386.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
> index c56ca4a2b4b8..1a6fd52f229a 100644
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -627,7 +627,6 @@ static int shared = 0;
>   unsigned int x86_sframe_cfa_sp_reg;
>   /* The other CFA base register for SFrame stack trace info.  */
>   unsigned int x86_sframe_cfa_fp_reg;
> -unsigned int x86_sframe_cfa_ra_reg;
>   
>   #endif
>   

Re: [PATCH v2 2/9] aarch64: Align SFrame terminology in comments to specs and x86

[Indu Bhagat]

On 2/23/24 09:07, Jens Remus wrote:
> Use the term "frame pointer" (FP) instead of "base pointer". This aligns
> with the terminology used in the SFrame specification. Additionally it
> helps not to confuse "base-pointer register" with the term "BASE_REG"
> used in the specification to denote either the SP or FP register.
> 

Thanks for noticing and fixing these.

> While at it align the frame-pointer and return address register comments
> to the x86 AMD64 ones.
> 
> gas/
> 	* config/tc-aarch64.h: Align SFrame terminology in comments to
> 	  specs and x86 AMD64.
> 

Subsequent lines in the ChangeLog need re-indentation as pointed out in 
review of [Patch 1/9].  I will skip mentioning this for the rest of the 
patches in this series from here.

> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
> 
> Notes (jremus):
>      This patch can be dropped, if subsequent patch "sframe: Enhance comments
>      for SFRAME_CFA_*_REG macros" gets accepted.
> 

Yes, makes sense. Lets drop this patch and keep the next one.

Thanks

>   gas/config/tc-aarch64.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gas/config/tc-aarch64.h b/gas/config/tc-aarch64.h
> index 1b8badad9fdc..599d78db7908 100644
> --- a/gas/config/tc-aarch64.h
> +++ b/gas/config/tc-aarch64.h
> @@ -271,11 +271,11 @@ extern bool aarch64_support_sframe_p (void);
>   extern unsigned int aarch64_sframe_cfa_sp_reg;
>   #define SFRAME_CFA_SP_REG aarch64_sframe_cfa_sp_reg
>   
> -/* The base-pointer register number for CFA stack trace info.  */
> +/* The frame-pointer register number for SFrame stack trace info.  */
>   extern unsigned int aarch64_sframe_cfa_fp_reg;
>   #define SFRAME_CFA_FP_REG aarch64_sframe_cfa_fp_reg
>   
> -/* The return address register number for CFA stack trace info.  */
> +/* The return address register number for SFrame stack trace info.  */
>   extern unsigned int aarch64_sframe_cfa_ra_reg;
>   #define SFRAME_CFA_RA_REG aarch64_sframe_cfa_ra_reg
>   

Re: [PATCH v2 3/9] sframe: Enhance comments for SFRAME_CFA_*_REG macros

[Indu Bhagat]

On 2/23/24 09:07, Jens Remus wrote:
> Specify what the SFRAME_CFA_*_REG register numbers are used for:
> - SP (stack pointer): CFA tracking
> - FP (frame pointer): CFA and FP tracking
> - RA (return address): RA tracking
> 
> gas/
> 	* config/tc-aarch64.h: Enhance comments for SFRAME_CFA_*_REG
> 	  macros.
> 	* config/tc-i386.h: Likewise.
> 
> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
>   gas/config/tc-aarch64.h | 6 +++---
>   gas/config/tc-i386.h    | 4 ++--
>   2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/gas/config/tc-aarch64.h b/gas/config/tc-aarch64.h
> index 599d78db7908..59c9b5a09ec0 100644
> --- a/gas/config/tc-aarch64.h
> +++ b/gas/config/tc-aarch64.h
> @@ -267,15 +267,15 @@ extern void aarch64_after_parse_args (void);
>   extern bool aarch64_support_sframe_p (void);
>   #define support_sframe_p aarch64_support_sframe_p
>   
> -/* The stack-pointer register number for SFrame stack trace info.  */
> +/* The stack-pointer register number for CFA tracking.  */

What do you think about including "SFrame" in all the touched comments 
in this patch.  So something like:

/* The stack-pointer register number for SFrame CFA tracking.  */

above ...

>   extern unsigned int aarch64_sframe_cfa_sp_reg;
>   #define SFRAME_CFA_SP_REG aarch64_sframe_cfa_sp_reg
>   
> -/* The frame-pointer register number for SFrame stack trace info.  */
> +/* The frame-pointer register number for CFA and FP tracking.  */

... and here

>   extern unsigned int aarch64_sframe_cfa_fp_reg;
>   #define SFRAME_CFA_FP_REG aarch64_sframe_cfa_fp_reg
>   
> -/* The return address register number for SFrame stack trace info.  */
> +/* The return address register number for RA tracking.  */

and here. And others below :)

Thanks

>   extern unsigned int aarch64_sframe_cfa_ra_reg;
>   #define SFRAME_CFA_RA_REG aarch64_sframe_cfa_ra_reg
>   
> diff --git a/gas/config/tc-i386.h b/gas/config/tc-i386.h
> index b93799a3b48c..0d6fb002166f 100644
> --- a/gas/config/tc-i386.h
> +++ b/gas/config/tc-i386.h
> @@ -440,11 +440,11 @@ extern bool x86_scfi_callee_saved_p (uint32_t dw2reg_num);
>   extern bool x86_support_sframe_p (void);
>   #define support_sframe_p x86_support_sframe_p
>   
> -/* The stack-pointer register number for SFrame stack trace info.  */
> +/* The stack-pointer register number for CFA tracking.  */
>   extern unsigned int x86_sframe_cfa_sp_reg;
>   #define SFRAME_CFA_SP_REG x86_sframe_cfa_sp_reg
>   
> -/* The frame-pointer register number for SFrame stack trace info.  */
> +/* The frame-pointer register number for CFA and FP tracking.  */
>   extern unsigned int x86_sframe_cfa_fp_reg;
>   #define SFRAME_CFA_FP_REG x86_sframe_cfa_fp_reg
>   

Re: [PATCH v2 4/9] readelf/objdump: Dump SFrame CFA fixed FP and RA offsets

[Indu Bhagat]

On 2/23/24 09:07, Jens Remus wrote:
> The SFrame format allows architectures to specify fixed offsets from the
> CFA, if any, from which the frame pointer (FP) and/or return address
> (RA) may be recovered. These offsets are stored in the SFrame header.
> 
> For instance the SFrame generation in the assembler for x86 AMD64
> specifies a fixed offset from the CFA, from which the return address
> (RA) may be recovered.
> 
> When dumping the SFrame header, for instance in readelf/objdump with
> option --sframe, do also dump the specified fixed offsets from the CFA,
> if any, from which the frame pointer (FP) and return address (RA) may
> be recovered.
> 
> Update the common SFrame test case verification patterns to allow for
> the optional dumping of the CFA fixed FP/RA offsets. Update the x86-
> specific SFrame and SCFI test case verification patterns to require a
> CFA fixed RA offset of -8.
> 
> libsframe:

Nit - libsframe: --> libsframe/

LGTM.
Thanks

> 	* sframe-dump.c: Dump CFA fixed FP and RA offsets.
> 
> gas/testsuite/
> 	* gas/cfi-sframe/cfi-sframe-common-1.d: Test for optional fixed
> 	  FP and RA offsets.
> 	* gas/cfi-sframe/cfi-sframe-common-2.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-common-3.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-common-4.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-common-5.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-common-6.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-common-7.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-common-8.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-x86_64-1.d: Test for fixed RA
> 	  offset.
> 	* gas/cfi-sframe/common-empty-1.d: Test for optional fixed FP
> 	  and RA offsets.
> 	* gas/cfi-sframe/common-empty-2.d: Likewise.
> 	* gas/cfi-sframe/common-empty-3.d: Likewise.
> 	* gas/scfi/x86_64/scfi-cfi-sections-1.d: Test for SFrame fixed
> 	  RA offset.
> 	* gas/scfi/x86_64/scfi-dyn-stack-1.d: Likewise.
> 
> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
>   gas/testsuite/gas/cfi-sframe/cfi-sframe-common-1.d  |  2 ++
>   gas/testsuite/gas/cfi-sframe/cfi-sframe-common-2.d  |  2 ++
>   gas/testsuite/gas/cfi-sframe/cfi-sframe-common-3.d  |  2 ++
>   gas/testsuite/gas/cfi-sframe/cfi-sframe-common-4.d  |  2 ++
>   gas/testsuite/gas/cfi-sframe/cfi-sframe-common-5.d  |  2 ++
>   gas/testsuite/gas/cfi-sframe/cfi-sframe-common-6.d  |  2 ++
>   gas/testsuite/gas/cfi-sframe/cfi-sframe-common-7.d  |  2 ++
>   gas/testsuite/gas/cfi-sframe/cfi-sframe-common-8.d  |  2 ++
>   gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-1.d  |  1 +
>   gas/testsuite/gas/cfi-sframe/common-empty-1.d       |  2 ++
>   gas/testsuite/gas/cfi-sframe/common-empty-2.d       |  2 ++
>   gas/testsuite/gas/cfi-sframe/common-empty-3.d       |  2 ++
>   gas/testsuite/gas/scfi/x86_64/scfi-cfi-sections-1.d |  2 ++
>   gas/testsuite/gas/scfi/x86_64/scfi-dyn-stack-1.d    |  2 ++
>   libsframe/sframe-dump.c                             | 10 ++++++++++
>   15 files changed, 37 insertions(+)
> 
> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-1.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-1.d
> index 32577f31860e..5f4ae00747de 100644
> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-1.d
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-1.d
> @@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
>   
>       Version: SFRAME_VERSION_2
>       Flags: NONE
> +#?    CFA fixed FP offset: \-?\d+
> +#?    CFA fixed RA offset: \-?\d+
>       Num FDEs: 1
>       Num FREs: 1
>   
> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-2.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-2.d
> index 3e3f74dbe424..ded8c450a942 100644
> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-2.d
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-2.d
> @@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
>   
>       Version: SFRAME_VERSION_2
>       Flags: NONE
> +#?    CFA fixed FP offset: \-?\d+
> +#?    CFA fixed RA offset: \-?\d+
>       Num FDEs: 1
>       Num FREs: 1
>   
> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-3.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-3.d
> index 6430d463a891..d23fd9790f63 100644
> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-3.d
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-3.d
> @@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
>   
>       Version: SFRAME_VERSION_2
>       Flags: NONE
> +#?    CFA fixed FP offset: \-?\d+
> +#?    CFA fixed RA offset: \-?\d+
>       Num FDEs: 1
>       Num FREs: 1
>   
> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-4.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-4.d
> index 319ff96cce2a..ca559bd0a029 100644
> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-4.d
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-4.d
> @@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
>   
>       Version: SFRAME_VERSION_2
>       Flags: NONE
> +#?    CFA fixed FP offset: \-?\d+
> +#?    CFA fixed RA offset: \-?\d+
>       Num FDEs: 1
>       Num FREs: 3
>   
> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-5.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-5.d
> index 82d34973ddde..ee82053e13db 100644
> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-5.d
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-5.d
> @@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
>   
>       Version: SFRAME_VERSION_2
>       Flags: NONE
> +#?    CFA fixed FP offset: \-?\d+
> +#?    CFA fixed RA offset: \-?\d+
>       Num FDEs: 1
>       Num FREs: 3
>   
> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-6.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-6.d
> index fe6917c70800..9d54b98552bf 100644
> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-6.d
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-6.d
> @@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
>   
>       Version: SFRAME_VERSION_2
>       Flags: NONE
> +#?    CFA fixed FP offset: \-?\d+
> +#?    CFA fixed RA offset: \-?\d+
>       Num FDEs: 1
>       Num FREs: 3
>   
> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-7.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-7.d
> index 39724d9cdf19..2b7fe3aec8f4 100644
> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-7.d
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-7.d
> @@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
>   
>       Version: SFRAME_VERSION_2
>       Flags: NONE
> +#?    CFA fixed FP offset: \-?\d+
> +#?    CFA fixed RA offset: \-?\d+
>       Num FDEs: 1
>       Num FREs: 3
>   
> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-8.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-8.d
> index c0a0e627fad9..d654e1d0bcd4 100644
> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-8.d
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-8.d
> @@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
>   
>       Version: SFRAME_VERSION_2
>       Flags: NONE
> +#?    CFA fixed FP offset: \-?\d+
> +#?    CFA fixed RA offset: \-?\d+
>       Num FDEs: 1
>       Num FREs: 2
>   
> diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-1.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-1.d
> index ae36c21b3b7c..c8b5e6adfea0 100644
> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-1.d
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-1.d
> @@ -8,6 +8,7 @@ Contents of the SFrame section .sframe:
>   
>       Version: SFRAME_VERSION_2
>       Flags: NONE
> +    CFA fixed RA offset: \-8
>       Num FDEs: 1
>       Num FREs: 4
>   
> diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-1.d b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
> index b133b15b051d..125612ff841f 100644
> --- a/gas/testsuite/gas/cfi-sframe/common-empty-1.d
> +++ b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
> @@ -9,6 +9,8 @@ Contents of the SFrame section .sframe:
>   
>       Version: SFRAME_VERSION_2
>       Flags: NONE
> +#?    CFA fixed FP offset: \-?\d+
> +#?    CFA fixed RA offset: \-?\d+
>       Num FDEs: 0
>       Num FREs: 0
>   
> diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-2.d b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
> index c5bc8594f1b7..59328fc1033f 100644
> --- a/gas/testsuite/gas/cfi-sframe/common-empty-2.d
> +++ b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
> @@ -9,6 +9,8 @@ Contents of the SFrame section .sframe:
>   
>       Version: SFRAME_VERSION_2
>       Flags: NONE
> +#?    CFA fixed FP offset: \-?\d+
> +#?    CFA fixed RA offset: \-?\d+
>       Num FDEs: 0
>       Num FREs: 0
>   
> diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-3.d b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
> index df0b19ee1bd1..5914c620760d 100644
> --- a/gas/testsuite/gas/cfi-sframe/common-empty-3.d
> +++ b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
> @@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
>   
>       Version: SFRAME_VERSION_2
>       Flags: NONE
> +#?    CFA fixed FP offset: \-?\d+
> +#?    CFA fixed RA offset: \-?\d+
>       Num FDEs: 0
>       Num FREs: 0
>   
> diff --git a/gas/testsuite/gas/scfi/x86_64/scfi-cfi-sections-1.d b/gas/testsuite/gas/scfi/x86_64/scfi-cfi-sections-1.d
> index 5962980256c1..b0cf8f4f5212 100644
> --- a/gas/testsuite/gas/scfi/x86_64/scfi-cfi-sections-1.d
> +++ b/gas/testsuite/gas/scfi/x86_64/scfi-cfi-sections-1.d
> @@ -8,6 +8,8 @@ Contents of the SFrame section .sframe:
>   
>       Version: SFRAME_VERSION_2
>       Flags: NONE
> +#?    CFA fixed FP offset: \-?\d+
> +#?    CFA fixed RA offset: \-?\d+
>       Num FDEs: 1
>       Num FREs: 5
>   
> diff --git a/gas/testsuite/gas/scfi/x86_64/scfi-dyn-stack-1.d b/gas/testsuite/gas/scfi/x86_64/scfi-dyn-stack-1.d
> index b51546af1494..4a449957187b 100644
> --- a/gas/testsuite/gas/scfi/x86_64/scfi-dyn-stack-1.d
> +++ b/gas/testsuite/gas/scfi/x86_64/scfi-dyn-stack-1.d
> @@ -9,6 +9,8 @@ Contents of the SFrame section .sframe:
>   
>       Version: SFRAME_VERSION_2
>       Flags: NONE
> +#?    CFA fixed FP offset: \-?\d+
> +#?    CFA fixed RA offset: \-?\d+
>       Num FDEs: 1
>       Num FREs: 4
>   
> diff --git a/libsframe/sframe-dump.c b/libsframe/sframe-dump.c
> index 42a086a5691f..493d052ce91f 100644
> --- a/libsframe/sframe-dump.c
> +++ b/libsframe/sframe-dump.c
> @@ -47,6 +47,8 @@ dump_sframe_header (sframe_decoder_ctx *sfd_ctx)
>     uint8_t flags;
>     char *flags_str;
>     const char *ver_str = NULL;
> +  int8_t cfa_fixed_fp_offset;
> +  int8_t cfa_fixed_ra_offset;
>     const sframe_header *header = &(sfd_ctx->sfd_header);
>   
>     /* Prepare SFrame section version string.  */
> @@ -82,12 +84,20 @@ dump_sframe_header (sframe_decoder_ctx *sfd_ctx)
>     else
>       strcpy (flags_str, "NONE");
>   
> +  /* CFA fixed FP and RA offsets.  */
> +  cfa_fixed_fp_offset = header->sfh_cfa_fixed_fp_offset;
> +  cfa_fixed_ra_offset = header->sfh_cfa_fixed_ra_offset;
> +
>     const char* subsec_name = "Header";
>     printf ("\n");
>     printf ("  %s :\n", subsec_name);
>     printf ("\n");
>     printf ("    Version: %s\n", ver_str);
>     printf ("    Flags: %s\n", flags_str);
> +  if (cfa_fixed_fp_offset != SFRAME_CFA_FIXED_FP_INVALID)
> +    printf ("    CFA fixed FP offset: %d\n", cfa_fixed_fp_offset);
> +  if (cfa_fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID)
> +    printf ("    CFA fixed RA offset: %d\n", cfa_fixed_ra_offset);
>     printf ("    Num FDEs: %d\n", sframe_decoder_get_num_fidx (sfd_ctx));
>     printf ("    Num FREs: %d\n", header->sfh_num_fres);
>   

Re: [PATCH v2 5/9] gas: Print DWARF call frame insn name in SFrame warning message

[Indu Bhagat]

On 2/23/24 09:07, Jens Remus wrote:
> SFrame generation prints the DWARF call frame instruction opcode in
> hexadecimal. Leverage get_DW_CFA_name to additionally print the
> DWARF call frame instruction name in human readable form, while also
> respecting fake CFI types. Use "(unknown)", if the DWARF call frame
> instruction name is not known.
> 
> This changes the following assembler SFrame generation warning message
> as follows:
> 
> Old:
> Warning: skipping SFrame FDE due to DWARF CFI op 0x<hexnum>
> 
> New:
> Warning: skipping SFrame FDE due to DWARF CFI op <name> (0x<hexnum>)
> 
> gas/
> 	* gen-sframe.c (sframe_get_cfi_name): New function to get the
> 	  DWARF call frame instruction name for a DWARF call frame
> 	  instruction opcode.
> 	  (sframe_do_cfi_insn): Use sframe_get_cfi_name to print the
> 	  DWARF call frame instruction name for the DWARF call frame
> 	  instruction opcode in the warning message.
> 
> gas/testsuite/
> 	* gas/cfi-sframe/common-empty-1.d: Update expected SFrame
> 	  warning message text for DWARF call frame insn name.
> 	* gas/cfi-sframe/common-empty-2.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-s390-err-1.l: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-s390-err-2.l: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-s390-err-3.l: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-s390-err-4.l: Likewise.
> 

Stale entries in ChangeLog for cfi-sframe-s390* files.  These are not 
included in this patch.

Otherwise, LGTM.

Thanks

> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
>   gas/gen-sframe.c                              | 49 ++++++++++++++++++-
>   gas/testsuite/gas/cfi-sframe/common-empty-1.d |  2 +-
>   gas/testsuite/gas/cfi-sframe/common-empty-2.d |  2 +-
>   3 files changed, 50 insertions(+), 3 deletions(-)
> 
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index 75781fc8ccbd..d35baaac54b2 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -1197,6 +1197,46 @@ sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
>     return SFRAME_XLATE_OK;
>   }
>   
> +/* Returns the DWARF call frame instruction name or fake CFI name for the
> +   specified CFI opcode, or NULL if the value is not recognized.  */
> +
> +static const char *
> +sframe_get_cfi_name (int cfi_opc)
> +{
> +  const char *cfi_name;
> +
> +  switch (cfi_opc)
> +    {
> +      /* Fake CFI type; outside the byte range of any real CFI insn.  */
> +      /* See gas/dw2gencfi.h.  */
> +      case CFI_adjust_cfa_offset:
> +	cfi_name = "CFI_adjust_cfa_offset";
> +	break;
> +      case CFI_return_column:
> +	cfi_name = "CFI_return_column";
> +	break;
> +      case CFI_rel_offset:
> +	cfi_name = "CFI_rel_offset";
> +	break;
> +      case CFI_escape:
> +	cfi_name = "CFI_escape";
> +	break;
> +      case CFI_signal_frame:
> +	cfi_name = "CFI_signal_frame";
> +	break;
> +      case CFI_val_encoded_addr:
> +	cfi_name = "CFI_val_encoded_addr";
> +	break;
> +      case CFI_label:
> +	cfi_name = "CFI_label";
> +	break;
> +      default:
> +	cfi_name = get_DW_CFA_name (cfi_opc);
> +    }
> +
> +  return cfi_name;
> +}
> +
>   /* Process CFI_INSN and update the translation context with the FRE
>      information.
>   
> @@ -1272,7 +1312,14 @@ sframe_do_cfi_insn (struct sframe_xlate_ctx *xlate_ctx,
>     /* An error here will cause no SFrame FDE later.  Warn the user because this
>        will affect the overall coverage and hence, asynchronicity.  */
>     if (err)
> -    as_warn (_("skipping SFrame FDE due to DWARF CFI op %#x"), op);
> +    {
> +      const char *cfi_name = sframe_get_cfi_name (op);
> +
> +      if (!cfi_name)
> +	cfi_name = _("(unknown)");
> +      as_warn (_("skipping SFrame FDE due to DWARF CFI op %s (%#x)"),
> +	       cfi_name, op);
> +    }
>   
>     return err;
>   }
> diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-1.d b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
> index 125612ff841f..d7756302b559 100644
> --- a/gas/testsuite/gas/cfi-sframe/common-empty-1.d
> +++ b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
> @@ -1,5 +1,5 @@
>   #as: --gsframe
> -#warning: skipping SFrame FDE due to DWARF CFI op 0xa
> +#warning: skipping SFrame FDE due to DWARF CFI op DW_CFA_remember_state \(0xa\)
>   #objdump: --sframe=.sframe
>   #name: Uninteresting cfi directives generate an empty SFrame section
>   #...
> diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-2.d b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
> index 59328fc1033f..20282c7854e8 100644
> --- a/gas/testsuite/gas/cfi-sframe/common-empty-2.d
> +++ b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
> @@ -1,5 +1,5 @@
>   #as: --gsframe
> -#warning: skipping SFrame FDE due to DWARF CFI op 0xe
> +#warning: skipping SFrame FDE due to DWARF CFI op DW_CFA_def_cfa_offset \(0xe\)
>   #objdump: --sframe=.sframe
>   #name: SFrame supports only FP/SP based CFA
>   #...

Re: [PATCH v2 6/9] gas: Skip SFrame FDE if CFI specifies non-FP/SP base register

[Indu Bhagat]

On 2/23/24 09:07, Jens Remus wrote:
> Do not generate SFrame FDE if DWARF CFI directives .cfi_def_cfa or
> .cfi_def_cfa_register specify a CFA base register number other than
> the architecture-specific stack-pointer (SP) or frame-pointer (FP)
> register numbers.
> 
> This also causes the assembler to print a warning message, so that
> skipping of the SFrame FDE does not occur silently.
> 
> Update the generic ld SFrame test case to be architecture independent.
> Do not use CFI directive .cfi_def_cfa, as the specified CFA base
> register number is not a valid SP/FP register number on all
> architectures. An invalid SP/FP register number will now cause the
> assembler to print a warning message and skip SFrame FDE generation.
> Remove the offending CFI directive, that cannot be coded architecture-
> independent, as the test case requires SFrame information to be
> generated. This was reported by the Linaro-TCWG-CI for AArch64.
> 
> gas/
> 	* gen-sframe.c: Skip SFrame generation if CFI specifies
> 	  non-FP/SP base register.
> 
> ld/testsuite/
> 	* ld-sframe/discard.s: Update generic SFrame test case to be
> 	  architecture independent.
> 

LGTM.

Thanks

> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
> 
> Notes (jremus):
>      v1 -> v2:
>      - Update generic SFrame test case to be architecture independent to
>        resolve generic ld SFrame test case failure reported by
>        Linaro-TCWG-CI for AArch64. It would fail similar on s390x.
>      
>      Without this patch the assembler would erroneously generate bad SFrame
>      information for the s390-specific SFrame error test cases 1 and 2, that
>      get introduced by patch "s390: Initial support to generate .sframe from
>      CFI directives in assembler".
> 
>   gas/gen-sframe.c                 | 13 +++++++++++--
>   ld/testsuite/ld-sframe/discard.s |  1 -
>   2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index d35baaac54b2..1269b2b77c54 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -986,7 +986,11 @@ sframe_xlate_do_def_cfa (struct sframe_xlate_ctx *xlate_ctx,
>   			       get_dw_fde_start_addrS (xlate_ctx->dw_fde));
>     }
>     /* Define the current CFA rule to use the provided register and
> -     offset.  */
> +     offset.  However, if the register is not FP/SP, skip creating
> +     SFrame stack trace info for the function.  */
> +  if (cfi_insn->u.r != SFRAME_CFA_SP_REG
> +      && cfi_insn->u.r != SFRAME_CFA_FP_REG)
> +    return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
>     sframe_fre_set_cfa_base_reg (cur_fre, cfi_insn->u.ri.reg);
>     sframe_fre_set_cfa_offset (cur_fre, cfi_insn->u.ri.offset);
>     cur_fre->merge_candidate = false;
> @@ -1004,9 +1008,14 @@ sframe_xlate_do_def_cfa_register (struct sframe_xlate_ctx *xlate_ctx,
>     struct sframe_row_entry *last_fre = xlate_ctx->last_fre;
>     /* Get the scratchpad FRE.  This FRE will eventually get linked in.  */
>     struct sframe_row_entry *cur_fre = xlate_ctx->cur_fre;
> +
>     gas_assert (cur_fre);
>     /* Define the current CFA rule to use the provided register (but to
> -     keep the old offset).  */
> +     keep the old offset).  However, if the register is not FP/SP,
> +     skip creating SFrame stack trace info for the function.  */
> +  if (cfi_insn->u.r != SFRAME_CFA_SP_REG
> +      && cfi_insn->u.r != SFRAME_CFA_FP_REG)
> +    return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
>     sframe_fre_set_cfa_base_reg (cur_fre, cfi_insn->u.ri.reg);
>     sframe_fre_set_cfa_offset (cur_fre, last_fre->cfa_offset);
>     cur_fre->merge_candidate = false;
> diff --git a/ld/testsuite/ld-sframe/discard.s b/ld/testsuite/ld-sframe/discard.s
> index a438b42bffa1..5591a50d486a 100644
> --- a/ld/testsuite/ld-sframe/discard.s
> +++ b/ld/testsuite/ld-sframe/discard.s
> @@ -5,7 +5,6 @@
>   foo:
>   	.cfi_startproc
>   	.cfi_def_cfa_offset 16
> -	.cfi_def_cfa 7, 8
>   	.cfi_endproc
>   
>   	.globl _start

Re: [PATCH v2 7/9] gas: Warn if SFrame FDE is skipped due to non-default return column

[Indu Bhagat]

On 2/23/24 09:07, Jens Remus wrote:
> Print a warning message if SFrame FDE is skipped due to a non-default
> DWARF return column (i.e. return address (RA) register number). This
> may be caused by the use of CFI directive .cfi_return_column with a
> non-default return address (RA) register number in the processed
> assembler source code.
> 
>    Warning: skipping SFrame FDE due to non-default DWARF return column
> 
> gas/
> 	* gen-sframe.c: Warn if SFrame FDE is skipped due to non-default
> 	  DWARF return column.
> 
> gas/testsuite/
> 	* gas/cfi-sframe/common-empty-3.d: Update test case to expect
> 	  for new warning message when SFrame FDE is skipped due to
> 	  a non-default DWARF return column.
> 

One request in my comments below.

LGTM, Thanks!

PS: I will take a look at the next 2 patches (Patch 8/9 and Patch 9/9) 
in the series soon.

> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
> 
> Notes (jremus):
>      Without this patch the assembler would erroneously generate bad SFrame
>      information for the s390-specific SFrame error test case 4, that gets
>      introduced by patch "s390: Initial support to generate .sframe from CFI
>      directives in assembler".
> 

This patch is only adding a warning.  The absence of it shouldn't have 
caused any bad SFrame info.

>   gas/gen-sframe.c                              | 8 ++++++--
>   gas/testsuite/gas/cfi-sframe/common-empty-3.d | 1 +
>   2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index 1269b2b77c54..28b49a2a8425 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -1345,7 +1345,10 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
>   
>     /* If the return column is not RIP, SFrame format cannot represent it.  */

May I ask you to also include an update to this incorrect comment in 
your patch to something like:

/* SFrame format cannot represent a non-default DWARF return column reg.  */
>     if (xlate_ctx->dw_fde->return_column != DWARF2_DEFAULT_RETURN_COLUMN)
> -    return SFRAME_XLATE_ERR_NOTREPRESENTED;
> +    {
> +      as_warn (_("skipping SFrame FDE due to non-default DWARF return column"));
> +      return SFRAME_XLATE_ERR_NOTREPRESENTED;
> +    }
>   
>     /* Iterate over the CFIs and create SFrame FREs.  */
>     for (cfi_insn = dw_fde->data; cfi_insn; cfi_insn = cfi_insn->next)
> @@ -1355,7 +1358,8 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
>         if (err != SFRAME_XLATE_OK)
>   	{
>   	  /* Skip generating SFrame stack trace info for the function if any
> -	     offending CFI is encountered by sframe_do_cfi_insn ().  */
> +	     offending CFI is encountered by sframe_do_cfi_insn ().  Warning
> +	     message already printed by sframe_do_cfi_insn ().  */
>   	  return err; /* Return the error code.  */
>   	}
>       }
> diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-3.d b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
> index 5914c620760d..d17521dd88ea 100644
> --- a/gas/testsuite/gas/cfi-sframe/common-empty-3.d
> +++ b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
> @@ -1,4 +1,5 @@
>   #as: --gsframe
> +#warning: skipping SFrame FDE due to non-default DWARF return column
>   #objdump: --sframe=.sframe
>   #name: SFrame supports only default return column
>   #...

Re: [PATCH v2 1/9] x86: Remove unused SFrame CFI RA register variable

[Jan Beulich]

On 24.02.2024 08:38, Indu Bhagat wrote:
> On 2/23/24 09:07, Jens Remus wrote:
>> gas/
>> 	* config/tc-i386.c: Remove unused SFrame CFI RA register
>> 	  variable.
>>
> 
> The ChangeLog formatting is a bit off here and in the rest of the 
> patches in this series.
> 
> The indentation of each line should be such that all content lines up 
> below the *, like so:
> 
> <TAB>* xyz.c: first line content
> <TAB>next line content
> 
> Other than that, LGTM.

Just to clarify: It's not actually a missing SFRAME_CFA_RA_REG define
(and initialization of the variable)?

Jan

Re: [PATCH v2 3/9] sframe: Enhance comments for SFRAME_CFA_*_REG macros

[Jan Beulich]

On 24.02.2024 08:46, Indu Bhagat wrote:
> On 2/23/24 09:07, Jens Remus wrote:
>> --- a/gas/config/tc-aarch64.h
>> +++ b/gas/config/tc-aarch64.h
>> @@ -267,15 +267,15 @@ extern void aarch64_after_parse_args (void);
>>   extern bool aarch64_support_sframe_p (void);
>>   #define support_sframe_p aarch64_support_sframe_p
>>   
>> -/* The stack-pointer register number for SFrame stack trace info.  */
>> +/* The stack-pointer register number for CFA tracking.  */
> 
> What do you think about including "SFrame" in all the touched comments 
> in this patch.  So something like:
> 
> /* The stack-pointer register number for SFrame CFA tracking.  */
> 
> above ...
> 
>>   extern unsigned int aarch64_sframe_cfa_sp_reg;
>>   #define SFRAME_CFA_SP_REG aarch64_sframe_cfa_sp_reg
>>   
>> -/* The frame-pointer register number for SFrame stack trace info.  */
>> +/* The frame-pointer register number for CFA and FP tracking.  */
> 
> ... and here
> 
>>   extern unsigned int aarch64_sframe_cfa_fp_reg;
>>   #define SFRAME_CFA_FP_REG aarch64_sframe_cfa_fp_reg
>>   
>> -/* The return address register number for SFrame stack trace info.  */
>> +/* The return address register number for RA tracking.  */
> 
> and here. And others below :)

Question is: In how far are these variables sframe-specific? On x86 at
least they exactly match the Dwarf2 register numbers, and hence are
in principle pretty generic as to potential future usage. In which
case rather than adding SFrame to the comments, I'd wonder in how far
"sframe" may want purging from their names instead.

In fact on x86 I think these could be #define-s instead of variables,
at least as long as only 64-bit code is supported for anything using
these values.

Jan

Re: [PATCH v2 1/9] x86: Remove unused SFrame CFI RA register variable

[Jens Remus]

Am 26.02.2024 um 14:01 schrieb Jan Beulich:
> On 24.02.2024 08:38, Indu Bhagat wrote:
>> On 2/23/24 09:07, Jens Remus wrote:
>>> gas/
>>> 	* config/tc-i386.c: Remove unused SFrame CFI RA register
>>> 	  variable.
>>>
>>
>> The ChangeLog formatting is a bit off here and in the rest of the
>> patches in this series.
>>
>> The indentation of each line should be such that all content lines up
>> below the *, like so:
>>
>> <TAB>* xyz.c: first line content
>> <TAB>next line content
>>
>> Other than that, LGTM.
> 
> Just to clarify: It's not actually a missing SFRAME_CFA_RA_REG define
> (and initialization of the variable)?

My understanding is that on x86 AMD64 SFrame assumes the return address 
to be always passed on the stack at fixed offset -8 from CFA (see 
x86_sframe_cfa_ra_offset). That is why SFrame does not use 
SFRAME_CFA_RA_REG (= DWARF2_DEFAULT_RETURN_COLUMN) on that architecture 
to track the return address being saved on the stack (see 
x86_sframe_ra_tracking_p). SFrame only supports either FP/RA tracking or 
fixed offset to CFA.

Regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303) and z/VSE Support
+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/

Re: [PATCH v2 1/9] x86: Remove unused SFrame CFI RA register variable

[Jens Remus]

Am 24.02.2024 um 08:38 schrieb Indu Bhagat:
> On 2/23/24 09:07, Jens Remus wrote:
>> gas/
>>     * config/tc-i386.c: Remove unused SFrame CFI RA register
>>       variable.
>>
> 
> The ChangeLog formatting is a bit off here and in the rest of the 
> patches in this series.
> 
> The indentation of each line should be such that all content lines up 
> below the *, like so:
> 
> <TAB>* xyz.c: first line content
> <TAB>next line content

Thanks for letting me know! Have corrected that in the whole series.

> Other than that, LGTM.

Thank you for the review! Will wait for your response on Jan's question 
before pushing it to master.

> 
> Thanks
> Indu
> 
>> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
>> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
>> ---
>>   gas/config/tc-i386.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
>> index c56ca4a2b4b8..1a6fd52f229a 100644
>> --- a/gas/config/tc-i386.c
>> +++ b/gas/config/tc-i386.c
>> @@ -627,7 +627,6 @@ static int shared = 0;
>>   unsigned int x86_sframe_cfa_sp_reg;
>>   /* The other CFA base register for SFrame stack trace info.  */
>>   unsigned int x86_sframe_cfa_fp_reg;
>> -unsigned int x86_sframe_cfa_ra_reg;
>>   #endif

Regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303) and z/VSE Support
+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/

Re: [PATCH v2 5/9] gas: Print DWARF call frame insn name in SFrame warning message

[Jens Remus]

Am 24.02.2024 um 08:56 schrieb Indu Bhagat:
> On 2/23/24 09:07, Jens Remus wrote:
>> SFrame generation prints the DWARF call frame instruction opcode in
>> hexadecimal. Leverage get_DW_CFA_name to additionally print the
>> DWARF call frame instruction name in human readable form, while also
>> respecting fake CFI types. Use "(unknown)", if the DWARF call frame
>> instruction name is not known.
>>
>> This changes the following assembler SFrame generation warning message
>> as follows:
>>
>> Old:
>> Warning: skipping SFrame FDE due to DWARF CFI op 0x<hexnum>
>>
>> New:
>> Warning: skipping SFrame FDE due to DWARF CFI op <name> (0x<hexnum>)
>>
>> gas/
>>     * gen-sframe.c (sframe_get_cfi_name): New function to get the
>>       DWARF call frame instruction name for a DWARF call frame
>>       instruction opcode.
>>       (sframe_do_cfi_insn): Use sframe_get_cfi_name to print the
>>       DWARF call frame instruction name for the DWARF call frame
>>       instruction opcode in the warning message.
>>
>> gas/testsuite/
>>     * gas/cfi-sframe/common-empty-1.d: Update expected SFrame
>>       warning message text for DWARF call frame insn name.
>>     * gas/cfi-sframe/common-empty-2.d: Likewise.
>>     * gas/cfi-sframe/cfi-sframe-s390-err-1.l: Likewise.
>>     * gas/cfi-sframe/cfi-sframe-s390-err-2.l: Likewise.
>>     * gas/cfi-sframe/cfi-sframe-s390-err-3.l: Likewise.
>>     * gas/cfi-sframe/cfi-sframe-s390-err-4.l: Likewise.
>>
> 
> Stale entries in ChangeLog for cfi-sframe-s390* files.  These are not 
> included in this patch.

Good catch! Is there a (git pre-commit) script I could use to validate 
those? Googling for "GNU changelog git commit message validate" did not 
return anything useful.

> Otherwise, LGTM.

Thank you for the review!

> Thanks
> 
>> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
>> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
>> ---
>>   gas/gen-sframe.c                              | 49 ++++++++++++++++++-
>>   gas/testsuite/gas/cfi-sframe/common-empty-1.d |  2 +-
>>   gas/testsuite/gas/cfi-sframe/common-empty-2.d |  2 +-
>>   3 files changed, 50 insertions(+), 3 deletions(-)
>>
>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>> index 75781fc8ccbd..d35baaac54b2 100644
>> --- a/gas/gen-sframe.c
>> +++ b/gas/gen-sframe.c
>> @@ -1197,6 +1197,46 @@ sframe_xlate_do_gnu_window_save (struct 
>> sframe_xlate_ctx *xlate_ctx,
>>     return SFRAME_XLATE_OK;
>>   }
>> +/* Returns the DWARF call frame instruction name or fake CFI name for 
>> the
>> +   specified CFI opcode, or NULL if the value is not recognized.  */
>> +
>> +static const char *
>> +sframe_get_cfi_name (int cfi_opc)
>> +{
>> +  const char *cfi_name;
>> +
>> +  switch (cfi_opc)
>> +    {
>> +      /* Fake CFI type; outside the byte range of any real CFI insn.  */
>> +      /* See gas/dw2gencfi.h.  */
>> +      case CFI_adjust_cfa_offset:
>> +    cfi_name = "CFI_adjust_cfa_offset";
>> +    break;
>> +      case CFI_return_column:
>> +    cfi_name = "CFI_return_column";
>> +    break;
>> +      case CFI_rel_offset:
>> +    cfi_name = "CFI_rel_offset";
>> +    break;
>> +      case CFI_escape:
>> +    cfi_name = "CFI_escape";
>> +    break;
>> +      case CFI_signal_frame:
>> +    cfi_name = "CFI_signal_frame";
>> +    break;
>> +      case CFI_val_encoded_addr:
>> +    cfi_name = "CFI_val_encoded_addr";
>> +    break;
>> +      case CFI_label:
>> +    cfi_name = "CFI_label";
>> +    break;
>> +      default:
>> +    cfi_name = get_DW_CFA_name (cfi_opc);
>> +    }
>> +
>> +  return cfi_name;
>> +}
>> +
>>   /* Process CFI_INSN and update the translation context with the FRE
>>      information.
>> @@ -1272,7 +1312,14 @@ sframe_do_cfi_insn (struct sframe_xlate_ctx 
>> *xlate_ctx,
>>     /* An error here will cause no SFrame FDE later.  Warn the user 
>> because this
>>        will affect the overall coverage and hence, asynchronicity.  */
>>     if (err)
>> -    as_warn (_("skipping SFrame FDE due to DWARF CFI op %#x"), op);
>> +    {
>> +      const char *cfi_name = sframe_get_cfi_name (op);
>> +
>> +      if (!cfi_name)
>> +    cfi_name = _("(unknown)");
>> +      as_warn (_("skipping SFrame FDE due to DWARF CFI op %s (%#x)"),
>> +           cfi_name, op);
>> +    }
>>     return err;
>>   }
>> diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-1.d 
>> b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
>> index 125612ff841f..d7756302b559 100644
>> --- a/gas/testsuite/gas/cfi-sframe/common-empty-1.d
>> +++ b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
>> @@ -1,5 +1,5 @@
>>   #as: --gsframe
>> -#warning: skipping SFrame FDE due to DWARF CFI op 0xa
>> +#warning: skipping SFrame FDE due to DWARF CFI op 
>> DW_CFA_remember_state \(0xa\)
>>   #objdump: --sframe=.sframe
>>   #name: Uninteresting cfi directives generate an empty SFrame section
>>   #...
>> diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-2.d 
>> b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
>> index 59328fc1033f..20282c7854e8 100644
>> --- a/gas/testsuite/gas/cfi-sframe/common-empty-2.d
>> +++ b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
>> @@ -1,5 +1,5 @@
>>   #as: --gsframe
>> -#warning: skipping SFrame FDE due to DWARF CFI op 0xe
>> +#warning: skipping SFrame FDE due to DWARF CFI op 
>> DW_CFA_def_cfa_offset \(0xe\)
>>   #objdump: --sframe=.sframe
>>   #name: SFrame supports only FP/SP based CFA
>>   #...

Regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303) and z/VSE Support
+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/

Re: [PATCH v2 7/9] gas: Warn if SFrame FDE is skipped due to non-default return column

[Jens Remus]

Am 24.02.2024 um 09:43 schrieb Indu Bhagat:
> On 2/23/24 09:07, Jens Remus wrote:
>> Print a warning message if SFrame FDE is skipped due to a non-default
>> DWARF return column (i.e. return address (RA) register number). This
>> may be caused by the use of CFI directive .cfi_return_column with a
>> non-default return address (RA) register number in the processed
>> assembler source code.
>>
>>    Warning: skipping SFrame FDE due to non-default DWARF return column
>>
>> gas/
>>     * gen-sframe.c: Warn if SFrame FDE is skipped due to non-default
>>       DWARF return column.
>>
>> gas/testsuite/
>>     * gas/cfi-sframe/common-empty-3.d: Update test case to expect
>>       for new warning message when SFrame FDE is skipped due to
>>       a non-default DWARF return column.
>>
> 
> One request in my comments below.
> 
> LGTM, Thanks!

Thank you for the review!

> PS: I will take a look at the next 2 patches (Patch 8/9 and Patch 9/9) 
> in the series soon.
> 
>> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
>> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
>> ---
>>
>> Notes (jremus):
>>      Without this patch the assembler would erroneously generate bad 
>> SFrame
>>      information for the s390-specific SFrame error test case 4, that 
>> gets
>>      introduced by patch "s390: Initial support to generate .sframe 
>> from CFI
>>      directives in assembler".
>>
> 
> This patch is only adding a warning.  The absence of it shouldn't have 
> caused any bad SFrame info.

You are correct! It is not bad SFrame info, but unexpected absent SFrame 
info. Without the warning the user does not know that the SFrame info is 
incomplete, which may be an issue during unwinding.

>>   gas/gen-sframe.c                              | 8 ++++++--
>>   gas/testsuite/gas/cfi-sframe/common-empty-3.d | 1 +
>>   2 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>> index 1269b2b77c54..28b49a2a8425 100644
>> --- a/gas/gen-sframe.c
>> +++ b/gas/gen-sframe.c
>> @@ -1345,7 +1345,10 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
>>     /* If the return column is not RIP, SFrame format cannot represent 
>> it.  */
> 
> May I ask you to also include an update to this incorrect comment in 
> your patch to something like:
> 
> /* SFrame format cannot represent a non-default DWARF return column 
> reg.  */

Sure.

>>     if (xlate_ctx->dw_fde->return_column != DWARF2_DEFAULT_RETURN_COLUMN)
>> -    return SFRAME_XLATE_ERR_NOTREPRESENTED;
>> +    {
>> +      as_warn (_("skipping SFrame FDE due to non-default DWARF return 
>> column"));
>> +      return SFRAME_XLATE_ERR_NOTREPRESENTED;
>> +    }

Actually I wonder whether the test should better use SFRAME_CFA_RA_REG
instead of DWARF2_DEFAULT_RETURN_COLUMN? Of course that would require 
SFRAME_CFA_RA_REG and x86_sframe_cfa_ra_reg to be defined and assigned 
for x86 AMD64, instead of removing the currently unused variable in 
patch 1 of this series.

Thinking more about this I wonder why doesn't SFrame by default #define 
SFRAME_CFA_RA_REG to be DWARF2_DEFAULT_RETURN_COLUMN?

>>     /* Iterate over the CFIs and create SFrame FREs.  */
>>     for (cfi_insn = dw_fde->data; cfi_insn; cfi_insn = cfi_insn->next)
>> @@ -1355,7 +1358,8 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
>>         if (err != SFRAME_XLATE_OK)
>>       {
>>         /* Skip generating SFrame stack trace info for the function if 
>> any
>> -         offending CFI is encountered by sframe_do_cfi_insn ().  */
>> +         offending CFI is encountered by sframe_do_cfi_insn ().  Warning
>> +         message already printed by sframe_do_cfi_insn ().  */
>>         return err; /* Return the error code.  */
>>       }
>>       }
>> diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-3.d 
>> b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
>> index 5914c620760d..d17521dd88ea 100644
>> --- a/gas/testsuite/gas/cfi-sframe/common-empty-3.d
>> +++ b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
>> @@ -1,4 +1,5 @@
>>   #as: --gsframe
>> +#warning: skipping SFrame FDE due to non-default DWARF return column
>>   #objdump: --sframe=.sframe
>>   #name: SFrame supports only default return column
>>   #...

Thanks and regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303) and z/VSE Support
+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/

Re: [PATCH v2 3/9] sframe: Enhance comments for SFRAME_CFA_*_REG macros

[Indu Bhagat]

On 2/26/24 05:51, Jan Beulich wrote:
> On 24.02.2024 08:46, Indu Bhagat wrote:
>> On 2/23/24 09:07, Jens Remus wrote:
>>> --- a/gas/config/tc-aarch64.h
>>> +++ b/gas/config/tc-aarch64.h
>>> @@ -267,15 +267,15 @@ extern void aarch64_after_parse_args (void);
>>>    extern bool aarch64_support_sframe_p (void);
>>>    #define support_sframe_p aarch64_support_sframe_p
>>>    
>>> -/* The stack-pointer register number for SFrame stack trace info.  */
>>> +/* The stack-pointer register number for CFA tracking.  */
>>
>> What do you think about including "SFrame" in all the touched comments
>> in this patch.  So something like:
>>
>> /* The stack-pointer register number for SFrame CFA tracking.  */
>>
>> above ...
>>
>>>    extern unsigned int aarch64_sframe_cfa_sp_reg;
>>>    #define SFRAME_CFA_SP_REG aarch64_sframe_cfa_sp_reg
>>>    
>>> -/* The frame-pointer register number for SFrame stack trace info.  */
>>> +/* The frame-pointer register number for CFA and FP tracking.  */
>>
>> ... and here
>>
>>>    extern unsigned int aarch64_sframe_cfa_fp_reg;
>>>    #define SFRAME_CFA_FP_REG aarch64_sframe_cfa_fp_reg
>>>    
>>> -/* The return address register number for SFrame stack trace info.  */
>>> +/* The return address register number for RA tracking.  */
>>
>> and here. And others below :)
> 
> Question is: In how far are these variables sframe-specific? On x86 at
> least they exactly match the Dwarf2 register numbers, and hence are
> in principle pretty generic as to potential future usage. In which
> case rather than adding SFrame to the comments, I'd wonder in how far
> "sframe" may want purging from their names instead.
> 

So far, the names contained "sframe" solely because that was the only 
user of these definitions.

> In fact on x86 I think these could be #define-s instead of variables,
> at least as long as only 64-bit code is supported for anything using
> these values.
> 

Makes sense.  Now that we have some code in SCFI and elsewhere in x86 
using these DWARF numbers for SP/FP, we can revisit the names etc once 
Jan's patch that defines them for x86 goes in.

Indu

Re: [PATCH v2 3/9] sframe: Enhance comments for SFRAME_CFA_*_REG macros

[Jan Beulich]

On 27.02.2024 09:53, Indu Bhagat wrote:
> On 2/26/24 05:51, Jan Beulich wrote:
>> On 24.02.2024 08:46, Indu Bhagat wrote:
>>> On 2/23/24 09:07, Jens Remus wrote:
>>>> --- a/gas/config/tc-aarch64.h
>>>> +++ b/gas/config/tc-aarch64.h
>>>> @@ -267,15 +267,15 @@ extern void aarch64_after_parse_args (void);
>>>>    extern bool aarch64_support_sframe_p (void);
>>>>    #define support_sframe_p aarch64_support_sframe_p
>>>>    
>>>> -/* The stack-pointer register number for SFrame stack trace info.  */
>>>> +/* The stack-pointer register number for CFA tracking.  */
>>>
>>> What do you think about including "SFrame" in all the touched comments
>>> in this patch.  So something like:
>>>
>>> /* The stack-pointer register number for SFrame CFA tracking.  */
>>>
>>> above ...
>>>
>>>>    extern unsigned int aarch64_sframe_cfa_sp_reg;
>>>>    #define SFRAME_CFA_SP_REG aarch64_sframe_cfa_sp_reg
>>>>    
>>>> -/* The frame-pointer register number for SFrame stack trace info.  */
>>>> +/* The frame-pointer register number for CFA and FP tracking.  */
>>>
>>> ... and here
>>>
>>>>    extern unsigned int aarch64_sframe_cfa_fp_reg;
>>>>    #define SFRAME_CFA_FP_REG aarch64_sframe_cfa_fp_reg
>>>>    
>>>> -/* The return address register number for SFrame stack trace info.  */
>>>> +/* The return address register number for RA tracking.  */
>>>
>>> and here. And others below :)
>>
>> Question is: In how far are these variables sframe-specific? On x86 at
>> least they exactly match the Dwarf2 register numbers, and hence are
>> in principle pretty generic as to potential future usage. In which
>> case rather than adding SFrame to the comments, I'd wonder in how far
>> "sframe" may want purging from their names instead.
>>
> 
> So far, the names contained "sframe" solely because that was the only 
> user of these definitions.
> 
>> In fact on x86 I think these could be #define-s instead of variables,
>> at least as long as only 64-bit code is supported for anything using
>> these values.
>>
> 
> Makes sense.  Now that we have some code in SCFI and elsewhere in x86 
> using these DWARF numbers for SP/FP, we can revisit the names etc once 
> Jan's patch that defines them for x86 goes in.

I'm confused: Which my patch?

Jan

Re: [PATCH v2 3/9] sframe: Enhance comments for SFRAME_CFA_*_REG macros

[Indu Bhagat]

On 2/27/24 00:57, Jan Beulich wrote:
> On 27.02.2024 09:53, Indu Bhagat wrote:
>> On 2/26/24 05:51, Jan Beulich wrote:
>>> On 24.02.2024 08:46, Indu Bhagat wrote:
>>>> On 2/23/24 09:07, Jens Remus wrote:
>>>>> --- a/gas/config/tc-aarch64.h
>>>>> +++ b/gas/config/tc-aarch64.h
>>>>> @@ -267,15 +267,15 @@ extern void aarch64_after_parse_args (void);
>>>>>     extern bool aarch64_support_sframe_p (void);
>>>>>     #define support_sframe_p aarch64_support_sframe_p
>>>>>     
>>>>> -/* The stack-pointer register number for SFrame stack trace info.  */
>>>>> +/* The stack-pointer register number for CFA tracking.  */
>>>>
>>>> What do you think about including "SFrame" in all the touched comments
>>>> in this patch.  So something like:
>>>>
>>>> /* The stack-pointer register number for SFrame CFA tracking.  */
>>>>
>>>> above ...
>>>>
>>>>>     extern unsigned int aarch64_sframe_cfa_sp_reg;
>>>>>     #define SFRAME_CFA_SP_REG aarch64_sframe_cfa_sp_reg
>>>>>     
>>>>> -/* The frame-pointer register number for SFrame stack trace info.  */
>>>>> +/* The frame-pointer register number for CFA and FP tracking.  */
>>>>
>>>> ... and here
>>>>
>>>>>     extern unsigned int aarch64_sframe_cfa_fp_reg;
>>>>>     #define SFRAME_CFA_FP_REG aarch64_sframe_cfa_fp_reg
>>>>>     
>>>>> -/* The return address register number for SFrame stack trace info.  */
>>>>> +/* The return address register number for RA tracking.  */
>>>>
>>>> and here. And others below :)
>>>
>>> Question is: In how far are these variables sframe-specific? On x86 at
>>> least they exactly match the Dwarf2 register numbers, and hence are
>>> in principle pretty generic as to potential future usage. In which
>>> case rather than adding SFrame to the comments, I'd wonder in how far
>>> "sframe" may want purging from their names instead.
>>>
>>
>> So far, the names contained "sframe" solely because that was the only
>> user of these definitions.
>>
>>> In fact on x86 I think these could be #define-s instead of variables,
>>> at least as long as only 64-bit code is supported for anything using
>>> these values.
>>>
>>
>> Makes sense.  Now that we have some code in SCFI and elsewhere in x86
>> using these DWARF numbers for SP/FP, we can revisit the names etc once
>> Jan's patch that defines them for x86 goes in.
> 
> I'm confused: Which my patch?
> 

Sorry, I was thinking about "[PATCH v3] x86: adjust which Dwarf2 
register numbers to use" 
https://sourceware.org/pipermail/binutils/2024-February/132623.html, and 
mistakenly thought your patch defines the REG_FP/REG_SP afresh.

Re: [PATCH v2 1/9] x86: Remove unused SFrame CFI RA register variable

[Indu Bhagat]

On 2/26/24 05:01, Jan Beulich wrote:
> On 24.02.2024 08:38, Indu Bhagat wrote:
>> On 2/23/24 09:07, Jens Remus wrote:
>>> gas/
>>> 	* config/tc-i386.c: Remove unused SFrame CFI RA register
>>> 	  variable.
>>>
>>
>> The ChangeLog formatting is a bit off here and in the rest of the
>> patches in this series.
>>
>> The indentation of each line should be such that all content lines up
>> below the *, like so:
>>
>> <TAB>* xyz.c: first line content
>> <TAB>next line content
>>
>> Other than that, LGTM.
> 
> Just to clarify: It's not actually a missing SFRAME_CFA_RA_REG define
> (and initialization of the variable)?
> 

For architectures where return address is at a fixed offset from CFA, 
like x86, it made sense to not define and initialize SFRAME_CFA_RA_REG.

So, the variable x86_sframe_cfa_ra_reg was unused and unnecessary.

Re: [PATCH v2 1/9] x86: Remove unused SFrame CFI RA register variable

[Jan Beulich]

On 27.02.2024 10:08, Indu Bhagat wrote:
> On 2/26/24 05:01, Jan Beulich wrote:
>> On 24.02.2024 08:38, Indu Bhagat wrote:
>>> On 2/23/24 09:07, Jens Remus wrote:
>>>> gas/
>>>> 	* config/tc-i386.c: Remove unused SFrame CFI RA register
>>>> 	  variable.
>>>>
>>>
>>> The ChangeLog formatting is a bit off here and in the rest of the
>>> patches in this series.
>>>
>>> The indentation of each line should be such that all content lines up
>>> below the *, like so:
>>>
>>> <TAB>* xyz.c: first line content
>>> <TAB>next line content
>>>
>>> Other than that, LGTM.
>>
>> Just to clarify: It's not actually a missing SFRAME_CFA_RA_REG define
>> (and initialization of the variable)?
>>
> 
> For architectures where return address is at a fixed offset from CFA, 
> like x86, it made sense to not define and initialize SFRAME_CFA_RA_REG.
> 
> So, the variable x86_sframe_cfa_ra_reg was unused and unnecessary.

Okay. So Jens - feel free to put in.

Jan

Re: [RFC PATCH 9/9] s390: Initial support to generate .sframe from CFI directives in assembler

[Indu Bhagat]

On 2/23/24 09:08, Jens Remus wrote:
> This introduces initial support to generate .sframe from CFI directives
> in assembler on s390x. Due to the following SFrame limitations it is
> incomplete and does not work for most real-world applications (i.e.
> generation of SFrame FDE is skipped):
> 

Hi Jens,

I am curious to know more about the use-case that triggered the interest 
in exploring SFrame for s390x.  It will be helpful if you can elaborate 
a bit.

My comments are scattered inline, but let me state some high-level 
comments here:

- As I previously mentioned, we cannot go down the route of tracking all 
callee-saved registers in SFrame.  This will significantly bloat up the 
information.  SFrame (like ORC) is designed to provide the exact offsets 
for stacktracing, which is what enables it to support fast stacktracing.

- In general, SFrame format relies heavily on ABI and is meant to cater 
to needs of ABI compliant code.  If some information is recoverable 
unambiguously from the ABI, SFrame does not encode it.  This manifests 
itself in what you already observed:

    + SFrame assumes RA is either on stack or a fixed/single designated 
register.
    + SFrame assumes FP is either on stack or a fixed/single designated 
register.
    + SFrame assumes CFA tracking is SP or FP based.

  Since the s390x ABI is flexible on each one of the above aspects, 
SFrame cannot be used easily.

> - SFrame FP/RA tracking assumes the register contents to be 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
>    other registers (e.g. floating-point registers).
> 

Is it possible to limit the choice of registers which the compiler uses 
to save the FP ?  If there is one more register to track (apart from 
CFA, RA, FP), we can see if an additional column can be accommodated in 
the format representation.

Is it possible to maintain the role "return address" for the register 
r14 at all times ?

What I am driving to is: If there is some degree of freedom here, is it 
possible to create a compiler option of, say -msframe, so that the 
generated code is more amenable to easier backtracing using SFrame format.

> - SFrame assumes a static RA register number. While the s390x ELF ABI
>    [1] specifies register 14 to contain the return address on entry,
>    GCC can be observed to copy the return address to another register
>    and may even use that to return. Additionally glibc on s390x has
>    two functions (mcount and fentry) that specify register 0 to hold
>    the return address. This would either require a change to glibc (if
>    possible) or SFrame support to track the RA register number specified
>    in CFI directive .cfi_return_column.
> 
> - SFrame assumes a static FP register number. The s390x ELF ABI [1]
>    does not specify any FP register number. GCC and clang on s390x
>    usually use register 11 as frame pointer. GCC on s390x can also be
>    observed to use register 14 (e.g. binutils and glibc builds). glibc
>    on s390x contains code that uses register 12 for the frame pointer.
>    This would require SFrame support to track the SP/FP register number
>    specified in CFI directives .cfi_def_cfa and .cfi_def_cfa_register.
> 
> - SFrame does not support CFI directive .cfi_val_offset. glibc on s390x
>    has two functions (mcount and fentry), that make use of that CFI
>    directive. This would either require a change in glibc (if possible)
>    or SFrame to support CFI directive .cfi_val_offset.
> 
> This s390x support is largely based on the AArch64 support from commit
> b52c4ee46657 ("gas: generate .sframe from CFI directives").
> 
> SFrame ABI/arch identifier SFRAME_ABI_S390_ENDIAN_BIG is introduced
> for s390 and added to the SFrame specification.
> 
> According to the s390x ELF ABI [1] the following calling conventions
> are used on s390x architecture:
> - Register 15 is used as stack pointer (SP). The CFA is initially
>    located at offset +160 from the SP.
> - Register 14 is used as return address (RA).
> - There is no dedicated frame pointer. GCC and LLVM currently use
>    register 11 as frame pointer.

Can FP be pinned to r11 in the hypothetical option of -msframe ?

> 
> The s390x ELF ABI [1] standard stack frame layout has the following
> conventions:
> - The return address (RA, r15) may be saved at offset -40 from the
>    CFA. Unlike x86 AMD64 architecture it is not necessarily saved on
>    the stack. Also compilers may use a non-standard stack frame layout,
>    such as but not limited to GCC with option -mpacked-stack.
>    Therefore SFrame RA tracking is used.

If SFrame RA tracking is enabled, this allows:
   - the return address to be in register (RA, r14), or
   - at an offset from CFA

Does the non-standard stack layout affect the identification of RA as 
CFA+offset ? If not, the remaining thing to worry about here (wrt SFrame 
usage) is if RA is saved in another register.

I see that the s390x does define r14 as return register.  But adds 
"Except at function entry, no special role
is assigned to r14.", which bring the complication for SFrame.

But at least for the packages that you have tested, we do not see 
occurrences of "skipping SFrame FDE due to .cfi_register specifying RA 
register (r14)", which is good to see.

> - The potential frame pointer (FP, r11) may be saved at offset -72
>    from the CFA. It is not necessarily saved on the stack and compilers
>    may use a non-standard stack frame layout (see above).
>    Therefore SFrame FP tracking is used.
> 
> Support for SFrame is only enabled for z/Architecture (s390x) with
> 64-bit architecture mode. It is disabled for 32-bit architecture mode
> and ESA/390 (s390).
> 
> Add s390-specific SFrame test cases. As for the error test cases add
> ones that use a non-default frame pointer (FP) register number and ones
> that save the return address (RA) in a non-default RA register number.
> 
> [1] ELF ABI s390x Supplement:
>      https://github.com/IBM/s390x-abi/releases
> [2] ELF ABI s390 Supplement:
>      https://refspecs.linuxfoundation.org/ELF/zSeries/lzsabi0_s390.html
>      https://refspecs.linuxfoundation.org/ELF/zSeries/lzsabi0_s390.pdf
> 
> include/
> 	* sframe.h (SFRAME_ABI_S390_ENDIAN_BIG): Define SFrame ABI/arch
> 	  identifier for s390x. Reference s390x architecture in comments.
> 
> libsframe/
> 	* doc/sframe-spec.texi: Document SFrame ABI/arch identifier for
> 	  s390x and reference s390x architecture.
> 
> gas/
> 	* config/tc-s390.h: s390x support to generate .sframe from CFI
> 	  directives in assembler.
> 	* config/tc-s390.c: Likewise.
> 	* gen-sframe.c: Reference s390x architecture in comments.
> 
> gas/testsuite/
> 	* gas/cfi-sframe/cfi-sframe.exp: Enable common SFrame test cases
> 	  for s390x. Add s390-specific SFrame test cases.
> 	* gas/cfi-sframe/cfi-sframe-s390-1.s: New s390-specific SFrame
> 	  test case.
> 	* gas/cfi-sframe/cfi-sframe-s390-1.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-s390-2.s: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-s390-2.d: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-s390-err-1.s: New s390-specific
> 	  SFrame error test case that uses a non-default register number
> 	  for the frame pointer.
> 	* gas/cfi-sframe/cfi-sframe-s390-err-1.l: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-s390-err-2.s: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-s390-err-2.l: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-s390-err-3.s: New s390-specific
> 	  SFrame error test case that uses a non-default register number
> 	  to save the return address.
> 	* gas/cfi-sframe/cfi-sframe-s390-err-3.l: Likewise.
> 	* gas/cfi-sframe/cfi-sframe-s390-err-4.s: New s390-specific
> 	  SFrame error test case that used a non-default return column
> 	  (return address) register number.
> 	* gas/cfi-sframe/cfi-sframe-s390-err-4.l: Likewise.
> 
> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
> 
> Notes (jremus):
>      The SFrame support for s390x provided by this patch still has some open
>      issues, which need to be addressed. Any ideas or assistance to overcome
>      the SFrame limitations listed in the commit description are very
>      welcome.
>      
>      Note that unlike the AArch64 and x68 AMD64 implementation this s390x
>      implementation does statically initialize the architecture-dependent
>      variables s390_sframe_cfa_{sp|fp|ra}_reg, which are referenced by the
>      SFrame interface macros SFRAME_CFA_{SP|FP|RA}_REG. The other
>      implementations do initialize them in md_begin. I verified that all
>      occurrences of the macros SFRAME_CFA_{SP|FP|RA}_REG are in context of
>      test/read and never write.
>      Is there a reason to define the SFrame interface macros
>      SFRAME_CFA_{SP|FP|RA}_REG to variables? For instance should the SFrame
>      common code be able to modify these? Currently I do not see how it
>      would work well, if the architecture-specific code would change these
>      at run time. Similar for the predicate sframe_ra_tracking_p.
>      

No, there is no scenario so far where these variables will be modified. 
So the code can be simplified, yes.

IOW, you are right about that : SFrame format, in its current V2 
representation, does not support the flexibility of changing 
SFRAME_CFA_{SP|FP|RA}_REG dynamically across FDEs.

>      Example #1: Warnings when compiling binutils with SFrame
>      
>      $ ../configure CC="gcc -B$HOME/temp/binutils-gcc/bin -Wa,--gsframe" \
>        CXX="g++ -B$HOME/temp/binutils-gcc/bin -Wa,--gsframe" \
>        --prefix=$HOME/temp/binutils-sframe
>      $ make -j $(nprocs) 2>&1 | tee make.log
>      $ grep --only-matching "Warning:.*" make.log | sort | uniq -c
>            1 Warning: skipping SFrame FDE due to .cfi_def_cfa specifying CFA
>              base register other than SP or FP (14 instead of 15 or 11)
>          205 Warning: skipping SFrame FDE due to .cfi_register specifying FP
>              register (11)
>           56 Warning: skipping SFrame FDE due to .cfi_register specifying SP
>              register (15)
>      
>      Example #2: Warnings when compiling glibc with SFrame
>      
>      $ ../configure CC="gcc -B$HOME/temp/binutils-sframe/bin -Wa,--gsframe" \
>        CXX="g++ -B$HOME/temp/binutils-sframe/bin -Wa,--gsframe" \
>        --prefix=$HOME/temp/glibc-sframe
>      $ make -j $(nprocs) 2>&1 | tee make.log
>      $ grep --only-matching "Warning:.*" make.log | sort | uniq -c
>            2 Warning: skipping SFrame FDE due to .cfi_def_cfa_register
>              specifying CFA base register other than SP or FP (12 instead of
>              15 or 11)
>            7 Warning: skipping SFrame FDE due to .cfi_def_cfa specifying CFA
>              base register other than SP or FP (14 instead of 15 or 11)
>          225 Warning: skipping SFrame FDE due to .cfi_register specifying FP
>              register (11)
>          187 Warning: skipping SFrame FDE due to .cfi_register specifying SP
>              register (15)
>            2 Warning: skipping SFrame FDE due to DWARF CFI op CFI_escape
>              (0x103)
>            2 Warning: skipping SFrame FDE due to non-default DWARF return
>              column (0 instead of 14)
>      
>      .cfi_def_cfa 14, ... originates from GCC generated code, which uses
>      register %r14 as frame pointer (FP).
>      
>      .cfi_def_cfa_register 12 originates from hand-written assembler code
>      sysdeps/s390/s390-64/dl-trampoline.S, which uses register %r12 as frame
>      pointer (FP).
>      
>      .cfi_return_column 0 and .cfi_escape both originate from hand-written
>      assembler code sysdeps/s390/s390-64/s390x-mcount.S, which is compiled
>      twice (with and without -DSHARED).
>      
>      - .cfi_escape 0x14, 0x15, 0x14: Is used to code
>        ".cfi_val_offset r15, -160", which would require binutils 2.28+ (glibc
>        currently supports a minimum binutils 2.25). This CFI directive states
>        that the contents of register %r15 are CFA-160 (not to be confused
>        with saved at CFA-160).
>      

This indirection property is not representable in SFrame format 
currently. BTW, x86, AArch64 also suffer with the negative consequences 
of this limitation, but in practice I have run into very limited 
occurrences of this.

>      - .cfi_return_column 0: The following comment explains why register %r0
>        contains the return address:
>      
>        "The _mcount implementation now has to call __mcount_internal with the
>        address of .LP0 as first parameter and the return address as second
>        parameter. &.LP0 was loaded to %r1 and the return address is in %r14.
>        _mcount may not modify any register.
>      
>        Alternatively, at the start of each function __fentry__ is called
>        using a single
>      
>                brasl  0,__fentry__
>      
>        instruction.  In this case %r0 points to the callee, and %r14 points
>        to the caller.  These values need to be passed to __mcount_internal
>        using the same sequence as for _mcount, so the code below is shared
>        between both functions.
>        The only major difference is that __fentry__ cannot return through
>        %r0, in which the return address is located, because br instruction
>        is a no-op with this register.  Therefore %r1, which is clobbered by
>        the PLT anyway, is used."
>      
>        Excerpt of the relevant glibc source code:
>      
>                .globl C_SYMBOL_NAME(MCOUNT_SYMBOL)
>                .type C_SYMBOL_NAME(MCOUNT_SYMBOL), @function
>                cfi_startproc
>                .align ALIGNARG(4)
>        C_LABEL(MCOUNT_SYMBOL)
>                cfi_return_column (glue(r, MCOUNT_CALLEE_REG))
>                /* Save the caller-clobbered registers.  */
>                aghi  %r15,-224
>                cfi_adjust_cfa_offset (224)
>                /* binutils 2.28+: .cfi_val_offset r15, -160 */
>                .cfi_escape \
>                        /* DW_CFA_val_offset */ 0x14, \
>                        /* r15 */               0x0f, \
>                        /* scaled offset */     0x14
>                stmg  %r14,%r5,160(%r15)
>      
>                cfi_offset (r14, -224)
>                cfi_offset (r0, -224+16)
>        ...
> 

Thanks for your clear notes. They were very helpful.

Indu

Re: [PATCH v2 8/9] gas: User readable warnings if SFrame FDE is not generated

[Indu Bhagat]

On 2/23/24 09:07, Jens Remus wrote:
> The following generic warning message, which is printed whenever the
> assembler skips generation of SFrame FDE, is not very helpful for the
> user:
> 
>    skipping SFrame FDE due to DWARF CFI op <name> (0x<hexval>)
> 
> Whenever possible print meaningful warning messages, when the assembler
> skips generation of SFrame FDE:
> 
> - skipping SFrame FDE due to .cfi_def_cfa specifying CFA base register
>    other than SP or FP (<regno> instead of <SP-regno> or <FP-regno>)

I find that stating "<SP-regno> or <FP-regno>" is not necessary in the 
warning.  This information is identified unambiguously, given an ABI.

How about:
"skipping SFrame FDE due to .cfi_def_cfa defining CFA base reg to 
<regno>" or similar ?

> - skipping SFrame FDE due to .cfi_def_cfa_register specifying CFA base
>    register other than SP or FP (<regno> instead of <SP-regno> or
>    <FP-regno>)

Same thought as above.

> - skipping SFrame FDE due to .cfi_def_cfa_offset without CFA base
>    register in effect
> - skipping SFrame FDE due to .cfi_def_cfa_offset while CFA base register
>    other than SP or FP in effect (<regno> instead of <SP-regno> or
>    <FP-regno>)

Ditto.

> - skipping SFrame FDE due to .cfi_val_offset specifying {FP|RA} register
>    (<regno>)
> - skipping SFrame FDE due to .cfi_remember_state without SFrame FRE
>    state

I dont expect this warning to occur at all out in the field 
(uhand-written asm with errors is the only case that comes to mind). 
The testcase which triggers it simply happens to be just a testcase for 
checking corner case handling.

WDYT about skipping this warning altogether ?

> - skipping SFrame FDE due to .cfi_register specifying {SP|FP|RA}
>    register (<regno>)
> - skipping SFrame FDE due to non-default DWARF return column (<regno>
>    instead of <DWARF-def-ret-col-regno>)

I think this can be simply "skipping SFrame FDE due to non-default 
return addr reg <regno>".  IMO, stating the default DWARF return address 
register information in each warning msg does not add value.

Thanks

> 
> gas/
> 	* gen-sframe.h (SFRAME_FRE_BASE_REG_INVAL): New macro for
> 	  invalid SFrame FRE CFA base register value of -1.
> 	* gen-sframe.c: User readable warnings if SFrame FDE is not
> 	  generated.
> 
> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
>   gas/gen-sframe.c                              | 96 ++++++++++++++-----
>   gas/gen-sframe.h                              |  2 +
>   gas/testsuite/gas/cfi-sframe/common-empty-1.d |  2 +-
>   gas/testsuite/gas/cfi-sframe/common-empty-2.d |  2 +-
>   gas/testsuite/gas/cfi-sframe/common-empty-3.d |  2 +-
>   5 files changed, 79 insertions(+), 25 deletions(-)
> 
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index 28b49a2a8425..339f4412ca05 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -867,7 +867,7 @@ sframe_row_entry_new (void)
>     struct sframe_row_entry *fre = XCNEW (struct sframe_row_entry);
>     /* Reset cfa_base_reg to -1.  A value of 0 will imply some valid register
>        for the supported arches.  */
> -  fre->cfa_base_reg = -1;
> +  fre->cfa_base_reg = SFRAME_FRE_BASE_REG_INVAL;
>     fre->merge_candidate = true;
>     /* Reset the mangled RA status bit to zero by default.  We will initialize it in
>        sframe_row_entry_initialize () with the sticky bit if set.  */
> @@ -922,6 +922,24 @@ sframe_row_entry_initialize (struct sframe_row_entry *cur_fre,
>     cur_fre->mangled_ra_p = prev_fre->mangled_ra_p;
>   }
>   
> +/* Return SFrame register name for SP, FP, and RA, or NULL if other.  */
> +
> +static const char *
> +sframe_register_name (unsigned int reg)
> +{
> +  if (reg == SFRAME_CFA_SP_REG)
> +    return "SP";
> +  else if (reg == SFRAME_CFA_FP_REG)
> +    return "FP";
> +#ifdef SFRAME_FRE_RA_TRACKING
> +  else if (sframe_ra_tracking_p ()
> +	   && (reg == SFRAME_CFA_RA_REG))
> +    return "RA";
> +#endif
> +  else
> +    return NULL;
> +}
> +
>   /* Translate DW_CFA_advance_loc into SFrame context.
>      Return SFRAME_XLATE_OK if success.  */
>   
> @@ -990,7 +1008,12 @@ sframe_xlate_do_def_cfa (struct sframe_xlate_ctx *xlate_ctx,
>        SFrame stack trace info for the function.  */
>     if (cfi_insn->u.r != SFRAME_CFA_SP_REG
>         && cfi_insn->u.r != SFRAME_CFA_FP_REG)
> -    return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
> +    {
> +      as_warn (_("skipping SFrame FDE due to .cfi_def_cfa specifying CFA base "
> +		 "register other than SP or FP (%u instead of %u or %u)"),
> +	       cfi_insn->u.r, SFRAME_CFA_SP_REG, SFRAME_CFA_FP_REG);
> +      return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
> +    }
>     sframe_fre_set_cfa_base_reg (cur_fre, cfi_insn->u.ri.reg);
>     sframe_fre_set_cfa_offset (cur_fre, cfi_insn->u.ri.offset);
>     cur_fre->merge_candidate = false;
> @@ -1015,7 +1038,12 @@ sframe_xlate_do_def_cfa_register (struct sframe_xlate_ctx *xlate_ctx,
>        skip creating SFrame stack trace info for the function.  */
>     if (cfi_insn->u.r != SFRAME_CFA_SP_REG
>         && cfi_insn->u.r != SFRAME_CFA_FP_REG)
> -    return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
> +    {
> +      as_warn (_("skipping SFrame FDE due to .cfi_def_cfa_register specifying "
> +		 "CFA base register other than SP or FP (%u instead of %u or %u)"),
> +	       cfi_insn->u.r, SFRAME_CFA_SP_REG, SFRAME_CFA_FP_REG);
> +      return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
> +    }
>     sframe_fre_set_cfa_base_reg (cur_fre, cfi_insn->u.ri.reg);
>     sframe_fre_set_cfa_offset (cur_fre, last_fre->cfa_offset);
>     cur_fre->merge_candidate = false;
> @@ -1046,7 +1074,16 @@ sframe_xlate_do_def_cfa_offset (struct sframe_xlate_ctx *xlate_ctx,
>         cur_fre->merge_candidate = false;
>       }
>     else
> -    return SFRAME_XLATE_ERR_NOTREPRESENTED;
> +    {
> +      if (cur_fre->cfa_base_reg == SFRAME_FRE_BASE_REG_INVAL)
> +	as_warn (_("skipping SFrame FDE due to .cfi_def_cfa_offset without CFA "
> +		   "base register in effect"));
> +      else
> +	as_warn (_("skipping SFrame FDE due to .cfi_def_cfa_offset while CFA "
> +		   "base register other than SP or FP in effect (%u instead of %u or %u)"),
> +		 cur_fre->cfa_base_reg, SFRAME_CFA_SP_REG, SFRAME_CFA_FP_REG);
> +      return SFRAME_XLATE_ERR_NOTREPRESENTED;
> +    }
>   
>     return SFRAME_XLATE_OK;
>   }
> @@ -1097,11 +1134,19 @@ sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>        instruction can be safely skipped without sacrificing the asynchronicity of
>        stack trace information.  */
>     if (cfi_insn->u.r == SFRAME_CFA_FP_REG)
> -    return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
> +    {
> +      as_warn (_("skipping SFrame FDE due to .cfi_val_offset specifying FP register (%u)"),
> +	       SFRAME_CFA_FP_REG);
> +      return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
> +    }
>   #ifdef SFRAME_FRE_RA_TRACKING
>     else if (sframe_ra_tracking_p ()
>   	   && cfi_insn->u.r == SFRAME_CFA_RA_REG)
> -    return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
> +    {
> +      as_warn (_("skipping SFrame FDE due to .cfi_val_offset specifying RA register (%u)"),
> +	       SFRAME_CFA_RA_REG);
> +      return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
> +    }
>   #endif
>   
>     /* Safe to skip.  */
> @@ -1120,7 +1165,10 @@ sframe_xlate_do_remember_state (struct sframe_xlate_ctx *xlate_ctx)
>        early with non-zero error code, this will cause no SFrame stack trace
>        info for the function involved.  */
>     if (!last_fre)
> -    return SFRAME_XLATE_ERR_INVAL;
> +    {
> +      as_warn (_("skipping SFrame FDE due to .cfi_remember_state without SFrame FRE state"));
> +      return SFRAME_XLATE_ERR_INVAL;
> +    }
>   
>     if (!xlate_ctx->remember_fre)
>       xlate_ctx->remember_fre = sframe_row_entry_new ();
> @@ -1307,7 +1355,12 @@ sframe_do_cfi_insn (struct sframe_xlate_ctx *xlate_ctx,
>   	  || cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG
>   #endif
>   	  || cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
> -	err = SFRAME_XLATE_ERR_NOTREPRESENTED;
> +	{
> +	  as_warn (_("skipping SFrame FDE due to .cfi_register specifying %s register (%u)"),
> +		   sframe_register_name (cfi_insn->u.rr.reg1),
> +		   cfi_insn->u.rr.reg1);
> +	  err = SFRAME_XLATE_ERR_NOTREPRESENTED;
> +	}
>         break;
>       case DW_CFA_undefined:
>       case DW_CFA_same_value:
> @@ -1315,21 +1368,19 @@ sframe_do_cfi_insn (struct sframe_xlate_ctx *xlate_ctx,
>       default:
>         /* Following skipped operations do, however, impact the asynchronicity:
>   	  - CFI_escape.  */
> -      err = SFRAME_XLATE_ERR_NOTREPRESENTED;
> -    }
> -
> -  /* An error here will cause no SFrame FDE later.  Warn the user because this
> -     will affect the overall coverage and hence, asynchronicity.  */
> -  if (err)
> -    {
> -      const char *cfi_name = sframe_get_cfi_name (op);
> -
> -      if (!cfi_name)
> -	cfi_name = _("(unknown)");
> -      as_warn (_("skipping SFrame FDE due to DWARF CFI op %s (%#x)"),
> -	       cfi_name, op);
> +      {
> +	const char *cfi_name = sframe_get_cfi_name (op);
> +
> +	if (!cfi_name)
> +	  cfi_name = _("(unknown)");
> +	as_warn (_("skipping SFrame FDE due to DWARF CFI op %s (%#x)"),
> +		 cfi_name, op);
> +        err = SFRAME_XLATE_ERR_NOTREPRESENTED;
> +      }
>       }
>   
> +  /* Any error will cause no SFrame FDE later.  The user has already been
> +     warned.  */
>     return err;
>   }
>   
> @@ -1346,7 +1397,8 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
>     /* If the return column is not RIP, SFrame format cannot represent it.  */
>     if (xlate_ctx->dw_fde->return_column != DWARF2_DEFAULT_RETURN_COLUMN)
>       {
> -      as_warn (_("skipping SFrame FDE due to non-default DWARF return column"));
> +      as_warn (_("skipping SFrame FDE due to non-default DWARF return column (%u instead of %u)"),
> +	       xlate_ctx->dw_fde->return_column, DWARF2_DEFAULT_RETURN_COLUMN);
>         return SFRAME_XLATE_ERR_NOTREPRESENTED;
>       }
>   
> diff --git a/gas/gen-sframe.h b/gas/gen-sframe.h
> index fbe2fd5d9368..8ed46dbb087b 100644
> --- a/gas/gen-sframe.h
> +++ b/gas/gen-sframe.h
> @@ -24,6 +24,8 @@
>   #define SFRAME_FRE_ELEM_LOC_REG		0
>   #define SFRAME_FRE_ELEM_LOC_STACK	1
>   
> +#define SFRAME_FRE_BASE_REG_INVAL	((unsigned int)-1)
> +
>   /* SFrame Frame Row Entry (FRE).
>   
>      A frame row entry is a slice of the frame and can be valid for a set of
> diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-1.d b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
> index d7756302b559..08731b069229 100644
> --- a/gas/testsuite/gas/cfi-sframe/common-empty-1.d
> +++ b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
> @@ -1,5 +1,5 @@
>   #as: --gsframe
> -#warning: skipping SFrame FDE due to DWARF CFI op DW_CFA_remember_state \(0xa\)
> +#warning: skipping SFrame FDE due to \.cfi_remember_state without SFrame FRE state
>   #objdump: --sframe=.sframe
>   #name: Uninteresting cfi directives generate an empty SFrame section
>   #...
> diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-2.d b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
> index 20282c7854e8..e759cddfcc20 100644
> --- a/gas/testsuite/gas/cfi-sframe/common-empty-2.d
> +++ b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
> @@ -1,5 +1,5 @@
>   #as: --gsframe
> -#warning: skipping SFrame FDE due to DWARF CFI op DW_CFA_def_cfa_offset \(0xe\)
> +#warning: skipping SFrame FDE due to \.cfi_def_cfa_offset without CFA base register in effect
>   #objdump: --sframe=.sframe
>   #name: SFrame supports only FP/SP based CFA
>   #...
> diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-3.d b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
> index d17521dd88ea..9a46b016a9ba 100644
> --- a/gas/testsuite/gas/cfi-sframe/common-empty-3.d
> +++ b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
> @@ -1,5 +1,5 @@
>   #as: --gsframe
> -#warning: skipping SFrame FDE due to non-default DWARF return column
> +#warning: skipping SFrame FDE due to non-default DWARF return column \(0 instead of \d+\)
>   #objdump: --sframe=.sframe
>   #name: SFrame supports only default return column
>   #...

Re: [RFC PATCH 0/9] s390: Initial support to generate SFrame info from CFI directives in assembler

[Indu Bhagat]

On 2/23/24 13:16, Indu Bhagat wrote:
> Hi Jens,
> 
> On 2/23/24 09:07, Jens Remus wrote:
>> This patch series adds initial support to the assembler on s390x to
>> generate SFrame stack trace information from CFI directives. It is
>> largely based on the respective AArch64 support.
>>
>> Please be aware that the SFrame support for s390x provided by patch 9
>> of this series still has some open issues, which need to be addressed.
>> Any ideas or assistance to overcome the current SFrame limitations
>> listed below and in the patch description are very welcome.
>>
>> Patches 1-8 are generic cleanups and enhancements to the generation
>> of SFrame information in the assembler. Patch 9 adds the initial s390x
>> support and is purely RFC.
>>
>>
>> Patches 1-3 are minor cleanups/enhancements to the existing SFrame
>> support on AArch64 and x86 AMD64.
>>
>> Patch 4 enables readelf/objdump to dump the SFrame fixed offsets from
>> CFA to the frame pointer (FP) and return address (RA).
>>
>> Patch 5 enhances an SFrame warning message to print the human readable
>> DWARF call frame instruction name.
>>
>> Patch 6 and 7 resolve issues that cause the assembler to either generate
>> bad SFrame FDE or to silently skip it. Both issues would be triggered by
>> s390-specific SFrame error test cases introduced by patch 9.
>>
>> Patch 8 adds verbose assembler warning messages when generation of
>> SFrame FDE is skipped.
>>
> 
> Thanks for fixing issues and enhancing SFrame handling in Binutils.
> 

Hi Jens,

I think it will be great to split this series and send a V2 for the 
Patches 1-8 from your series. What do you think ?

Thanks
Indu

Re: [PATCH v2 8/9] gas: User readable warnings if SFrame FDE is not generated

[Jens Remus]

Am 29.02.2024 um 08:39 schrieb Indu Bhagat:
> On 2/23/24 09:07, Jens Remus wrote:
>> The following generic warning message, which is printed whenever the
>> assembler skips generation of SFrame FDE, is not very helpful for the
>> user:
>>
>>    skipping SFrame FDE due to DWARF CFI op <name> (0x<hexval>)
>>
>> Whenever possible print meaningful warning messages, when the assembler
>> skips generation of SFrame FDE:
>>
>> - skipping SFrame FDE due to .cfi_def_cfa specifying CFA base register
>>    other than SP or FP (<regno> instead of <SP-regno> or <FP-regno>)
> 
> I find that stating "<SP-regno> or <FP-regno>" is not necessary in the 
> warning.  This information is identified unambiguously, given an ABI.
> 
> How about:
> "skipping SFrame FDE due to .cfi_def_cfa defining CFA base reg to 
> <regno>" or similar ?

Sure. What about:

"skipping SFrame FDE due to .cfi_def_cfa defining non-SP/FP register 
<regno> as CFA base register"

I think it would be helpful for the user to know that the issue is 
caused by a non-SP/FP register. But maybe that is because I am new to 
CFI and those that would actually debug the cause for these warnings 
would not need this extra information?

If you prefer your suggestions I would go with yours. I understand that 
the warning text is otherwise pretty long.

>> - skipping SFrame FDE due to .cfi_def_cfa_register specifying CFA base
>>    register other than SP or FP (<regno> instead of <SP-regno> or
>>    <FP-regno>)
> 
> Same thought as above.

Agreed.

>> - skipping SFrame FDE due to .cfi_def_cfa_offset without CFA base
>>    register in effect
>> - skipping SFrame FDE due to .cfi_def_cfa_offset while CFA base register
>>    other than SP or FP in effect (<regno> instead of <SP-regno> or
>>    <FP-regno>)
> 
> Ditto.

Agreed.

>> - skipping SFrame FDE due to .cfi_val_offset specifying {FP|RA} register
>>    (<regno>)
>> - skipping SFrame FDE due to .cfi_remember_state without SFrame FRE
>>    state
> 
> I dont expect this warning to occur at all out in the field 
> (uhand-written asm with errors is the only case that comes to mind). The 
> testcase which triggers it simply happens to be just a testcase for 
> checking corner case handling.
> 
> WDYT about skipping this warning altogether ?

I understand your reasoning. But I would prefer generation of SFrame FDE 
not to be silently skipped. Even or especially if it is an obscure 
special case.

With my changes sframe_do_cfi_insn() assumes any of its called 
sframe_do_*() CFI instruction processing functions to have issued a 
warning, if they return with an error such as 
SFRAME_XLATE_ERR_NOTREPRESENTED. Thus skipping this warning would cause 
generation of SFrame FDE to be silently skipped.

>> - skipping SFrame FDE due to .cfi_register specifying {SP|FP|RA}
>>    register (<regno>)
>> - skipping SFrame FDE due to non-default DWARF return column (<regno>
>>    instead of <DWARF-def-ret-col-regno>)
> 
> I think this can be simply "skipping SFrame FDE due to non-default 
> return addr reg <regno>".  IMO, stating the default DWARF return address 
> register information in each warning msg does not add value.

Agreed.

> Thanks
> 
>>
>> gas/
>>     * gen-sframe.h (SFRAME_FRE_BASE_REG_INVAL): New macro for
>>       invalid SFrame FRE CFA base register value of -1.
>>     * gen-sframe.c: User readable warnings if SFrame FDE is not
>>       generated.
>>
>> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
>> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
>> ---
>>   gas/gen-sframe.c                              | 96 ++++++++++++++-----
>>   gas/gen-sframe.h                              |  2 +
>>   gas/testsuite/gas/cfi-sframe/common-empty-1.d |  2 +-
>>   gas/testsuite/gas/cfi-sframe/common-empty-2.d |  2 +-
>>   gas/testsuite/gas/cfi-sframe/common-empty-3.d |  2 +-
>>   5 files changed, 79 insertions(+), 25 deletions(-)
>>
>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>> index 28b49a2a8425..339f4412ca05 100644
>> --- a/gas/gen-sframe.c
>> +++ b/gas/gen-sframe.c
>> @@ -867,7 +867,7 @@ sframe_row_entry_new (void)
>>     struct sframe_row_entry *fre = XCNEW (struct sframe_row_entry);
>>     /* Reset cfa_base_reg to -1.  A value of 0 will imply some valid 
>> register
>>        for the supported arches.  */
>> -  fre->cfa_base_reg = -1;
>> +  fre->cfa_base_reg = SFRAME_FRE_BASE_REG_INVAL;
>>     fre->merge_candidate = true;
>>     /* Reset the mangled RA status bit to zero by default.  We will 
>> initialize it in
>>        sframe_row_entry_initialize () with the sticky bit if set.  */
>> @@ -922,6 +922,24 @@ sframe_row_entry_initialize (struct 
>> sframe_row_entry *cur_fre,
>>     cur_fre->mangled_ra_p = prev_fre->mangled_ra_p;
>>   }
>> +/* Return SFrame register name for SP, FP, and RA, or NULL if other.  */
>> +
>> +static const char *
>> +sframe_register_name (unsigned int reg)
>> +{
>> +  if (reg == SFRAME_CFA_SP_REG)
>> +    return "SP";
>> +  else if (reg == SFRAME_CFA_FP_REG)
>> +    return "FP";
>> +#ifdef SFRAME_FRE_RA_TRACKING
>> +  else if (sframe_ra_tracking_p ()
>> +       && (reg == SFRAME_CFA_RA_REG))
>> +    return "RA";
>> +#endif
>> +  else
>> +    return NULL;
>> +}
>> +
>>   /* Translate DW_CFA_advance_loc into SFrame context.
>>      Return SFRAME_XLATE_OK if success.  */
>> @@ -990,7 +1008,12 @@ sframe_xlate_do_def_cfa (struct sframe_xlate_ctx 
>> *xlate_ctx,
>>        SFrame stack trace info for the function.  */
>>     if (cfi_insn->u.r != SFRAME_CFA_SP_REG
>>         && cfi_insn->u.r != SFRAME_CFA_FP_REG)
>> -    return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
>> +    {
>> +      as_warn (_("skipping SFrame FDE due to .cfi_def_cfa specifying 
>> CFA base "
>> +         "register other than SP or FP (%u instead of %u or %u)"),
>> +           cfi_insn->u.r, SFRAME_CFA_SP_REG, SFRAME_CFA_FP_REG);
>> +      return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
>> +    }
>>     sframe_fre_set_cfa_base_reg (cur_fre, cfi_insn->u.ri.reg);
>>     sframe_fre_set_cfa_offset (cur_fre, cfi_insn->u.ri.offset);
>>     cur_fre->merge_candidate = false;
>> @@ -1015,7 +1038,12 @@ sframe_xlate_do_def_cfa_register (struct 
>> sframe_xlate_ctx *xlate_ctx,
>>        skip creating SFrame stack trace info for the function.  */
>>     if (cfi_insn->u.r != SFRAME_CFA_SP_REG
>>         && cfi_insn->u.r != SFRAME_CFA_FP_REG)
>> -    return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
>> +    {
>> +      as_warn (_("skipping SFrame FDE due to .cfi_def_cfa_register 
>> specifying "
>> +         "CFA base register other than SP or FP (%u instead of %u or 
>> %u)"),
>> +           cfi_insn->u.r, SFRAME_CFA_SP_REG, SFRAME_CFA_FP_REG);
>> +      return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
>> +    }
>>     sframe_fre_set_cfa_base_reg (cur_fre, cfi_insn->u.ri.reg);
>>     sframe_fre_set_cfa_offset (cur_fre, last_fre->cfa_offset);
>>     cur_fre->merge_candidate = false;
>> @@ -1046,7 +1074,16 @@ sframe_xlate_do_def_cfa_offset (struct 
>> sframe_xlate_ctx *xlate_ctx,
>>         cur_fre->merge_candidate = false;
>>       }
>>     else
>> -    return SFRAME_XLATE_ERR_NOTREPRESENTED;
>> +    {
>> +      if (cur_fre->cfa_base_reg == SFRAME_FRE_BASE_REG_INVAL)
>> +    as_warn (_("skipping SFrame FDE due to .cfi_def_cfa_offset 
>> without CFA "
>> +           "base register in effect"));
>> +      else
>> +    as_warn (_("skipping SFrame FDE due to .cfi_def_cfa_offset while 
>> CFA "
>> +           "base register other than SP or FP in effect (%u instead 
>> of %u or %u)"),
>> +         cur_fre->cfa_base_reg, SFRAME_CFA_SP_REG, SFRAME_CFA_FP_REG);
>> +      return SFRAME_XLATE_ERR_NOTREPRESENTED;
>> +    }
>>     return SFRAME_XLATE_OK;
>>   }
>> @@ -1097,11 +1134,19 @@ sframe_xlate_do_val_offset (struct 
>> sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>>        instruction can be safely skipped without sacrificing the 
>> asynchronicity of
>>        stack trace information.  */
>>     if (cfi_insn->u.r == SFRAME_CFA_FP_REG)
>> -    return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
>> +    {
>> +      as_warn (_("skipping SFrame FDE due to .cfi_val_offset 
>> specifying FP register (%u)"),
>> +           SFRAME_CFA_FP_REG);
>> +      return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
>> +    }
>>   #ifdef SFRAME_FRE_RA_TRACKING
>>     else if (sframe_ra_tracking_p ()
>>          && cfi_insn->u.r == SFRAME_CFA_RA_REG)
>> -    return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
>> +    {
>> +      as_warn (_("skipping SFrame FDE due to .cfi_val_offset 
>> specifying RA register (%u)"),
>> +           SFRAME_CFA_RA_REG);
>> +      return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
>> +    }
>>   #endif
>>     /* Safe to skip.  */
>> @@ -1120,7 +1165,10 @@ sframe_xlate_do_remember_state (struct 
>> sframe_xlate_ctx *xlate_ctx)
>>        early with non-zero error code, this will cause no SFrame stack 
>> trace
>>        info for the function involved.  */
>>     if (!last_fre)
>> -    return SFRAME_XLATE_ERR_INVAL;
>> +    {
>> +      as_warn (_("skipping SFrame FDE due to .cfi_remember_state 
>> without SFrame FRE state"));
>> +      return SFRAME_XLATE_ERR_INVAL;
>> +    }
>>     if (!xlate_ctx->remember_fre)
>>       xlate_ctx->remember_fre = sframe_row_entry_new ();
>> @@ -1307,7 +1355,12 @@ sframe_do_cfi_insn (struct sframe_xlate_ctx 
>> *xlate_ctx,
>>         || cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG
>>   #endif
>>         || cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
>> -    err = SFRAME_XLATE_ERR_NOTREPRESENTED;
>> +    {
>> +      as_warn (_("skipping SFrame FDE due to .cfi_register specifying 
>> %s register (%u)"),
>> +           sframe_register_name (cfi_insn->u.rr.reg1),
>> +           cfi_insn->u.rr.reg1);
>> +      err = SFRAME_XLATE_ERR_NOTREPRESENTED;
>> +    }
>>         break;
>>       case DW_CFA_undefined:
>>       case DW_CFA_same_value:
>> @@ -1315,21 +1368,19 @@ sframe_do_cfi_insn (struct sframe_xlate_ctx 
>> *xlate_ctx,
>>       default:
>>         /* Following skipped operations do, however, impact the 
>> asynchronicity:
>>         - CFI_escape.  */
>> -      err = SFRAME_XLATE_ERR_NOTREPRESENTED;
>> -    }
>> -
>> -  /* An error here will cause no SFrame FDE later.  Warn the user 
>> because this
>> -     will affect the overall coverage and hence, asynchronicity.  */
>> -  if (err)
>> -    {
>> -      const char *cfi_name = sframe_get_cfi_name (op);
>> -
>> -      if (!cfi_name)
>> -    cfi_name = _("(unknown)");
>> -      as_warn (_("skipping SFrame FDE due to DWARF CFI op %s (%#x)"),
>> -           cfi_name, op);
>> +      {
>> +    const char *cfi_name = sframe_get_cfi_name (op);
>> +
>> +    if (!cfi_name)
>> +      cfi_name = _("(unknown)");
>> +    as_warn (_("skipping SFrame FDE due to DWARF CFI op %s (%#x)"),
>> +         cfi_name, op);
>> +        err = SFRAME_XLATE_ERR_NOTREPRESENTED;
>> +      }
>>       }
>> +  /* Any error will cause no SFrame FDE later.  The user has already 
>> been
>> +     warned.  */
>>     return err;
>>   }
>> @@ -1346,7 +1397,8 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
>>     /* If the return column is not RIP, SFrame format cannot represent 
>> it.  */
>>     if (xlate_ctx->dw_fde->return_column != DWARF2_DEFAULT_RETURN_COLUMN)
>>       {
>> -      as_warn (_("skipping SFrame FDE due to non-default DWARF return 
>> column"));
>> +      as_warn (_("skipping SFrame FDE due to non-default DWARF return 
>> column (%u instead of %u)"),
>> +           xlate_ctx->dw_fde->return_column, 
>> DWARF2_DEFAULT_RETURN_COLUMN);
>>         return SFRAME_XLATE_ERR_NOTREPRESENTED;
>>       }
>> diff --git a/gas/gen-sframe.h b/gas/gen-sframe.h
>> index fbe2fd5d9368..8ed46dbb087b 100644
>> --- a/gas/gen-sframe.h
>> +++ b/gas/gen-sframe.h
>> @@ -24,6 +24,8 @@
>>   #define SFRAME_FRE_ELEM_LOC_REG        0
>>   #define SFRAME_FRE_ELEM_LOC_STACK    1
>> +#define SFRAME_FRE_BASE_REG_INVAL    ((unsigned int)-1)
>> +
>>   /* SFrame Frame Row Entry (FRE).
>>      A frame row entry is a slice of the frame and can be valid for a 
>> set of
>> diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-1.d 
>> b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
>> index d7756302b559..08731b069229 100644
>> --- a/gas/testsuite/gas/cfi-sframe/common-empty-1.d
>> +++ b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
>> @@ -1,5 +1,5 @@
>>   #as: --gsframe
>> -#warning: skipping SFrame FDE due to DWARF CFI op 
>> DW_CFA_remember_state \(0xa\)
>> +#warning: skipping SFrame FDE due to \.cfi_remember_state without 
>> SFrame FRE state
>>   #objdump: --sframe=.sframe
>>   #name: Uninteresting cfi directives generate an empty SFrame section
>>   #...
>> diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-2.d 
>> b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
>> index 20282c7854e8..e759cddfcc20 100644
>> --- a/gas/testsuite/gas/cfi-sframe/common-empty-2.d
>> +++ b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
>> @@ -1,5 +1,5 @@
>>   #as: --gsframe
>> -#warning: skipping SFrame FDE due to DWARF CFI op 
>> DW_CFA_def_cfa_offset \(0xe\)
>> +#warning: skipping SFrame FDE due to \.cfi_def_cfa_offset without CFA 
>> base register in effect
>>   #objdump: --sframe=.sframe
>>   #name: SFrame supports only FP/SP based CFA
>>   #...
>> diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-3.d 
>> b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
>> index d17521dd88ea..9a46b016a9ba 100644
>> --- a/gas/testsuite/gas/cfi-sframe/common-empty-3.d
>> +++ b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
>> @@ -1,5 +1,5 @@
>>   #as: --gsframe
>> -#warning: skipping SFrame FDE due to non-default DWARF return column
>> +#warning: skipping SFrame FDE due to non-default DWARF return column 
>> \(0 instead of \d+\)
>>   #objdump: --sframe=.sframe
>>   #name: SFrame supports only default return column
>>   #...
> 

Thanks and regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303) and z/VSE Support
+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/

Re: [RFC PATCH 0/9] s390: Initial support to generate SFrame info from CFI directives in assembler

[Jens Remus]

Am 05.04.2024 um 20:26 schrieb Indu Bhagat:
> On 2/23/24 13:16, Indu Bhagat wrote:
>> On 2/23/24 09:07, Jens Remus wrote:
>>> This patch series adds initial support to the assembler on s390x to
>>> generate SFrame stack trace information from CFI directives. It is
>>> largely based on the respective AArch64 support.
>>>
>>> Please be aware that the SFrame support for s390x provided by patch 9
>>> of this series still has some open issues, which need to be addressed.
>>> Any ideas or assistance to overcome the current SFrame limitations
>>> listed below and in the patch description are very welcome.
>>>
>>> Patches 1-8 are generic cleanups and enhancements to the generation
>>> of SFrame information in the assembler. Patch 9 adds the initial s390x
>>> support and is purely RFC.
>>>
>>>
>>> Patches 1-3 are minor cleanups/enhancements to the existing SFrame
>>> support on AArch64 and x86 AMD64.
>>>
>>> Patch 4 enables readelf/objdump to dump the SFrame fixed offsets from
>>> CFA to the frame pointer (FP) and return address (RA).
>>>
>>> Patch 5 enhances an SFrame warning message to print the human readable
>>> DWARF call frame instruction name.
>>>
>>> Patch 6 and 7 resolve issues that cause the assembler to either generate
>>> bad SFrame FDE or to silently skip it. Both issues would be triggered by
>>> s390-specific SFrame error test cases introduced by patch 9.
>>>
>>> Patch 8 adds verbose assembler warning messages when generation of
>>> SFrame FDE is skipped.
>>>
>>
>> Thanks for fixing issues and enhancing SFrame handling in Binutils.
>>
> 
> Hi Jens,
> 
> I think it will be great to split this series and send a V2 for the 
> Patches 1-8 from your series. What do you think ?

Hi Indu,

thank you for your valuable feedback to my patch series! Sorry for the 
long delay in getting back to you.

That is an excellent suggestion to separate the non-s390 specific 
patches from the experimental s390 ones. I will do so shortly for both. 
Patches 1-8 have meanwhile grown to 1-15.

Regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303) and z/VSE Support
+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/

Re: [RFC PATCH 9/9] s390: Initial support to generate .sframe from CFI directives in assembler

[Jens Remus]

Hello Indu,

thank you for reviewing my series and for your valuable feedback! Sorry 
for the long delay in getting back to you.

I'll separate the non-s390 and s390-specific patches as you suggested 
and will send a V3 shortly.

Am 29.02.2024 um 08:01 schrieb Indu Bhagat:
> On 2/23/24 09:08, Jens Remus wrote:
>> This introduces initial support to generate .sframe from CFI directives
>> in assembler on s390x. Due to the following SFrame limitations it is
>> incomplete and does not work for most real-world applications (i.e.
>> generation of SFrame FDE is skipped):
>>
> 
> Hi Jens,
> 
> I am curious to know more about the use-case that triggered the interest 
> in exploring SFrame for s390x.  It will be helpful if you can elaborate 
> a bit.

We are evaluating whether SFrame could be used on s390x for Linux Kernel 
perf unwinding, similar as it is being worked on for x86-64.

> My comments are scattered inline, but let me state some high-level 
> comments here:
> 
> - As I previously mentioned, we cannot go down the route of tracking all 
> callee-saved registers in SFrame.  This will significantly bloat up the 
> information.  SFrame (like ORC) is designed to provide the exact offsets 
> for stacktracing, which is what enables it to support fast stacktracing.
> 
> - In general, SFrame format relies heavily on ABI and is meant to cater 
> to needs of ABI compliant code.  If some information is recoverable 
> unambiguously from the ABI, SFrame does not encode it.  This manifests 
> itself in what you already observed:
> 
>     + SFrame assumes RA is either on stack or a fixed/single designated 
> register.
>     + SFrame assumes FP is either on stack or a fixed/single designated 
> register.
>     + SFrame assumes CFA tracking is SP or FP based.
> 
>   Since the s390x ABI is flexible on each one of the above aspects, 
> SFrame cannot be used easily.
> 
>> - SFrame FP/RA tracking assumes the register contents to be 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
>>    other registers (e.g. floating-point registers).
>>
> 
> Is it possible to limit the choice of registers which the compiler uses 
> to save the FP ?  If there is one more register to track (apart from 
> CFA, RA, FP), we can see if an additional column can be accommodated in 
> the format representation.

The s390x ABI does not specify any set of register number(s) to be used 
to save the FP/RA. Therefore this could not be reduced down to a single 
additional register.

Meanwhile we have determined that GCC on s390x does only do so for 
leaf-functions. That are functions, which do not call any other 
functions. Therefore they can only ever occur as topmost frame during 
unwinding.

Therefore it would be sufficient if SFrame could represent FP/RA in 
another register on s390x, as it would not need to be able to track 
those other registers at all. An unwinder would have access to all of 
the registers of the topmost frame.

I will send an experimantal patch for that in my split-off s390x patch 
series.

> Is it possible to maintain the role "return address" for the register 
> r14 at all times ?

Similar if GCC on s390x would restore the RA from the stack into a 
register other than the RA-register r14 for the purpose of returning to 
the caller it would not perform any further function calls. That is this 
also only ever can occur in the topmost frame, so that an unwinder would 
have all registers available.

> What I am driving to is: If there is some degree of freedom here, is it 
> possible to create a compiler option of, say -msframe, so that the 
> generated code is more amenable to easier backtracing using SFrame format.

We need to review the performance impact of any optimization 
restrictions such a SFrame-specific compiler option would have and how 
that would compare to the alternative to use -mbackchain for unwinding 
in Linux Kernel perf.

In general it would be preferable not to impose additional restrictions 
wherever possible.

>> - SFrame assumes a static RA register number. While the s390x ELF ABI
>>    [1] specifies register 14 to contain the return address on entry,
>>    GCC can be observed to copy the return address to another register
>>    and may even use that to return. Additionally glibc on s390x has
>>    two functions (mcount and fentry) that specify register 0 to hold
>>    the return address. This would either require a change to glibc (if
>>    possible) or SFrame support to track the RA register number specified
>>    in CFI directive .cfi_return_column.
>>
>> - SFrame assumes a static FP register number. The s390x ELF ABI [1]
>>    does not specify any FP register number. GCC and clang on s390x
>>    usually use register 11 as frame pointer. GCC on s390x can also be
>>    observed to use register 14 (e.g. binutils and glibc builds). glibc
>>    on s390x contains code that uses register 12 for the frame pointer.
>>    This would require SFrame support to track the SP/FP register number
>>    specified in CFI directives .cfi_def_cfa and .cfi_def_cfa_register.
>>
>> - SFrame does not support CFI directive .cfi_val_offset. glibc on s390x
>>    has two functions (mcount and fentry), that make use of that CFI
>>    directive. This would either require a change in glibc (if possible)
>>    or SFrame to support CFI directive .cfi_val_offset.
>>
>> This s390x support is largely based on the AArch64 support from commit
>> b52c4ee46657 ("gas: generate .sframe from CFI directives").
>>
>> SFrame ABI/arch identifier SFRAME_ABI_S390_ENDIAN_BIG is introduced
>> for s390 and added to the SFrame specification.
>>
>> According to the s390x ELF ABI [1] the following calling conventions
>> are used on s390x architecture:
>> - Register 15 is used as stack pointer (SP). The CFA is initially
>>    located at offset +160 from the SP.
>> - Register 14 is used as return address (RA).
>> - There is no dedicated frame pointer. GCC and LLVM currently use
>>    register 11 as frame pointer.
> 
> Can FP be pinned to r11 in the hypothetical option of -msframe ?

We determined that GCC only chooses another register than r11 (e.g. 
RA-register r14) as temporary frame pointer in the stack protector in 
the function prologue. Again a case that only can occur when in the 
topmost stack frame.

It should be possible to eliminate this and use the FP-register r11 instead.

Another option would be to extend SFrame to track the CFA base register 
number on s390x without tracking that register contents. Since this 
special case would only occur on the topmost frame an unwinder would 
have access to all registers.

>> The s390x ELF ABI [1] standard stack frame layout has the following
>> conventions:
>> - The return address (RA, r15) may be saved at offset -40 from the
>>    CFA. Unlike x86 AMD64 architecture it is not necessarily saved on
>>    the stack. Also compilers may use a non-standard stack frame layout,
>>    such as but not limited to GCC with option -mpacked-stack.
>>    Therefore SFrame RA tracking is used.
> 
> If SFrame RA tracking is enabled, this allows:
>    - the return address to be in register (RA, r14), or
>    - at an offset from CFA
> 
> Does the non-standard stack layout affect the identification of RA as 
> CFA+offset ? If not, the remaining thing to worry about here (wrt SFrame 
> usage) is if RA is saved in another register.

No, that is not affected. I just wanted to explain that we cannot assume 
a fixed offset on the stack, if it is saved on the stack. But that is 
already taken care by SFrame.

> I see that the s390x does define r14 as return register.  But adds 
> "Except at function entry, no special role
> is assigned to r14.", which bring the complication for SFrame.

The RA is restored from the stack into another register than r14 only to 
perform the return.

Therefore we are wondering whether SFrame could be extended on s390x to 
track the FP/RA being saved in another register in leaf functions and 
the RA being saved in another register when being used to return to the 
caller. Both cases only occur when in the topmost stack frame.

An idea would be to use one unused bits of the FP/RA offset from CFA to 
identify whether it is actually an offset or a DWARF register number. On 
s390x the CFA offsets are always a multiple of -8. Unless SFrame would 
make use of the DWARF data alignment factor to encode the offsets in the 
future, this would enable to use the lowest-significant bit (LSB) for 
that purpose. DWARF register numbers could then be encoded as ((regno << 
1) | 1) and offsets as (offset).

One hopefully minor issue with that approach would be that it would 
probably be impossible to distinguish between our compiler generated 
cases and any handwritten assembler code and CFI cases, which could be 
impossible to represent with SFrame. That is generation of SFrame 
information might not be able to warn that the resulting information may 
be insufficient during unwinding. Only during unwinding the unwinder 
would be able to detect this and throw an error.

> But at least for the packages that you have tested, we do not see 
> occurrences of "skipping SFrame FDE due to .cfi_register specifying RA 
> register (r14)", which is good to see.

According to my colleagues GCC can be observed using another register 
than r14 to restore the return address from the stack and return to the 
caller. Unless there is a .cfi_register that should not bother us, as 
the RA would then still be on the stack. I still do not have an example 
for this to experiment with.

>> - The potential frame pointer (FP, r11) may be saved at offset -72
>>    from the CFA. It is not necessarily saved on the stack and compilers
>>    may use a non-standard stack frame layout (see above).
>>    Therefore SFrame FP tracking is used.
>>
>> Support for SFrame is only enabled for z/Architecture (s390x) with
>> 64-bit architecture mode. It is disabled for 32-bit architecture mode
>> and ESA/390 (s390).
>>
>> Add s390-specific SFrame test cases. As for the error test cases add
>> ones that use a non-default frame pointer (FP) register number and ones
>> that save the return address (RA) in a non-default RA register number.
>>
>> [1] ELF ABI s390x Supplement:
>>      https://github.com/IBM/s390x-abi/releases
>> [2] ELF ABI s390 Supplement:
>>      https://refspecs.linuxfoundation.org/ELF/zSeries/lzsabi0_s390.html
>>      https://refspecs.linuxfoundation.org/ELF/zSeries/lzsabi0_s390.pdf
>>
>> include/
>>     * sframe.h (SFRAME_ABI_S390_ENDIAN_BIG): Define SFrame ABI/arch
>>       identifier for s390x. Reference s390x architecture in comments.
>>
>> libsframe/
>>     * doc/sframe-spec.texi: Document SFrame ABI/arch identifier for
>>       s390x and reference s390x architecture.
>>
>> gas/
>>     * config/tc-s390.h: s390x support to generate .sframe from CFI
>>       directives in assembler.
>>     * config/tc-s390.c: Likewise.
>>     * gen-sframe.c: Reference s390x architecture in comments.
>>
>> gas/testsuite/
>>     * gas/cfi-sframe/cfi-sframe.exp: Enable common SFrame test cases
>>       for s390x. Add s390-specific SFrame test cases.
>>     * gas/cfi-sframe/cfi-sframe-s390-1.s: New s390-specific SFrame
>>       test case.
>>     * gas/cfi-sframe/cfi-sframe-s390-1.d: Likewise.
>>     * gas/cfi-sframe/cfi-sframe-s390-2.s: Likewise.
>>     * gas/cfi-sframe/cfi-sframe-s390-2.d: Likewise.
>>     * gas/cfi-sframe/cfi-sframe-s390-err-1.s: New s390-specific
>>       SFrame error test case that uses a non-default register number
>>       for the frame pointer.
>>     * gas/cfi-sframe/cfi-sframe-s390-err-1.l: Likewise.
>>     * gas/cfi-sframe/cfi-sframe-s390-err-2.s: Likewise.
>>     * gas/cfi-sframe/cfi-sframe-s390-err-2.l: Likewise.
>>     * gas/cfi-sframe/cfi-sframe-s390-err-3.s: New s390-specific
>>       SFrame error test case that uses a non-default register number
>>       to save the return address.
>>     * gas/cfi-sframe/cfi-sframe-s390-err-3.l: Likewise.
>>     * gas/cfi-sframe/cfi-sframe-s390-err-4.s: New s390-specific
>>       SFrame error test case that used a non-default return column
>>       (return address) register number.
>>     * gas/cfi-sframe/cfi-sframe-s390-err-4.l: Likewise.
>>
>> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
>> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
>> ---
>>
>> Notes (jremus):
>>      The SFrame support for s390x provided by this patch still has 
>> some open
>>      issues, which need to be addressed. Any ideas or assistance to 
>> overcome
>>      the SFrame limitations listed in the commit description are very
>>      welcome.
>>      Note that unlike the AArch64 and x68 AMD64 implementation this s390x
>>      implementation does statically initialize the architecture-dependent
>>      variables s390_sframe_cfa_{sp|fp|ra}_reg, which are referenced by 
>> the
>>      SFrame interface macros SFRAME_CFA_{SP|FP|RA}_REG. The other
>>      implementations do initialize them in md_begin. I verified that all
>>      occurrences of the macros SFRAME_CFA_{SP|FP|RA}_REG are in 
>> context of
>>      test/read and never write.
>>      Is there a reason to define the SFrame interface macros
>>      SFRAME_CFA_{SP|FP|RA}_REG to variables? For instance should the 
>> SFrame
>>      common code be able to modify these? Currently I do not see how it
>>      would work well, if the architecture-specific code would change 
>> these
>>      at run time. Similar for the predicate sframe_ra_tracking_p.
> 
> No, there is no scenario so far where these variables will be modified. 
> So the code can be simplified, yes.

Great!

> IOW, you are right about that : SFrame format, in its current V2 
> representation, does not support the flexibility of changing 
> SFRAME_CFA_{SP|FP|RA}_REG dynamically across FDEs.
> 
>>      Example #1: Warnings when compiling binutils with SFrame
>>      $ ../configure CC="gcc -B$HOME/temp/binutils-gcc/bin 
>> -Wa,--gsframe" \
>>        CXX="g++ -B$HOME/temp/binutils-gcc/bin -Wa,--gsframe" \
>>        --prefix=$HOME/temp/binutils-sframe
>>      $ make -j $(nprocs) 2>&1 | tee make.log
>>      $ grep --only-matching "Warning:.*" make.log | sort | uniq -c
>>            1 Warning: skipping SFrame FDE due to .cfi_def_cfa 
>> specifying CFA
>>              base register other than SP or FP (14 instead of 15 or 11)
>>          205 Warning: skipping SFrame FDE due to .cfi_register 
>> specifying FP
>>              register (11)
>>           56 Warning: skipping SFrame FDE due to .cfi_register 
>> specifying SP
>>              register (15)
>>      Example #2: Warnings when compiling glibc with SFrame
>>      $ ../configure CC="gcc -B$HOME/temp/binutils-sframe/bin 
>> -Wa,--gsframe" \
>>        CXX="g++ -B$HOME/temp/binutils-sframe/bin -Wa,--gsframe" \
>>        --prefix=$HOME/temp/glibc-sframe
>>      $ make -j $(nprocs) 2>&1 | tee make.log
>>      $ grep --only-matching "Warning:.*" make.log | sort | uniq -c
>>            2 Warning: skipping SFrame FDE due to .cfi_def_cfa_register
>>              specifying CFA base register other than SP or FP (12 
>> instead of
>>              15 or 11)
>>            7 Warning: skipping SFrame FDE due to .cfi_def_cfa 
>> specifying CFA
>>              base register other than SP or FP (14 instead of 15 or 11)
>>          225 Warning: skipping SFrame FDE due to .cfi_register 
>> specifying FP
>>              register (11)
>>          187 Warning: skipping SFrame FDE due to .cfi_register 
>> specifying SP
>>              register (15)
>>            2 Warning: skipping SFrame FDE due to DWARF CFI op CFI_escape
>>              (0x103)
>>            2 Warning: skipping SFrame FDE due to non-default DWARF return
>>              column (0 instead of 14)
>>      .cfi_def_cfa 14, ... originates from GCC generated code, which uses
>>      register %r14 as frame pointer (FP).
>>      .cfi_def_cfa_register 12 originates from hand-written assembler code
>>      sysdeps/s390/s390-64/dl-trampoline.S, which uses register %r12 as 
>> frame
>>      pointer (FP).

We can possibly change the handwritten assembler code to use the 
FP-register r11 as frame pointer instead of r12.

>>      .cfi_return_column 0 and .cfi_escape both originate from 
>> hand-written
>>      assembler code sysdeps/s390/s390-64/s390x-mcount.S, which is 
>> compiled
>>      twice (with and without -DSHARED).

It does not look like we can remove the use of r0 as return column in 
this special case. But since this only affects mcount/fentry, which are 
used for profiling we can potentially live with that, as long as SFrame 
would only be used for Linux Kernel perf unwinding.

>>      - .cfi_escape 0x14, 0x15, 0x14: Is used to code
>>        ".cfi_val_offset r15, -160", which would require binutils 2.28+ 
>> (glibc
>>        currently supports a minimum binutils 2.25). This CFI directive 
>> states
>>        that the contents of register %r15 are CFA-160 (not to be confused
>>        with saved at CFA-160).
> 
> This indirection property is not representable in SFrame format 
> currently. BTW, x86, AArch64 also suffer with the negative consequences 
> of this limitation, but in practice I have run into very limited 
> occurrences of this.

I meanwhile believe SFrame does not need to care about .cfi_register, 
.cfi_offset, and .cfi_val_offset definitions involving the SP register.

The reason is that SFrame does not track the SP register contents. It 
only tracks the CFA offset from SP/FP register. As a result the SP 
contents are restored using the CFA offset from SP/FP of the current FRE 
and the first FRE of a FDE, if I am not mistaken.

>>      - .cfi_return_column 0: The following comment explains why 
>> register %r0
>>        contains the return address:
>>        "The _mcount implementation now has to call __mcount_internal 
>> with the
>>        address of .LP0 as first parameter and the return address as 
>> second
>>        parameter. &.LP0 was loaded to %r1 and the return address is in 
>> %r14.
>>        _mcount may not modify any register.
>>        Alternatively, at the start of each function __fentry__ is called
>>        using a single
>>                brasl  0,__fentry__
>>        instruction.  In this case %r0 points to the callee, and %r14 
>> points
>>        to the caller.  These values need to be passed to 
>> __mcount_internal
>>        using the same sequence as for _mcount, so the code below is 
>> shared
>>        between both functions.
>>        The only major difference is that __fentry__ cannot return through
>>        %r0, in which the return address is located, because br 
>> instruction
>>        is a no-op with this register.  Therefore %r1, which is 
>> clobbered by
>>        the PLT anyway, is used."
>>        Excerpt of the relevant glibc source code:
>>                .globl C_SYMBOL_NAME(MCOUNT_SYMBOL)
>>                .type C_SYMBOL_NAME(MCOUNT_SYMBOL), @function
>>                cfi_startproc
>>                .align ALIGNARG(4)
>>        C_LABEL(MCOUNT_SYMBOL)
>>                cfi_return_column (glue(r, MCOUNT_CALLEE_REG))
>>                /* Save the caller-clobbered registers.  */
>>                aghi  %r15,-224
>>                cfi_adjust_cfa_offset (224)
>>                /* binutils 2.28+: .cfi_val_offset r15, -160 */
>>                .cfi_escape \
>>                        /* DW_CFA_val_offset */ 0x14, \
>>                        /* r15 */               0x0f, \
>>                        /* scaled offset */     0x14
>>                stmg  %r14,%r5,160(%r15)
>>                cfi_offset (r14, -224)
>>                cfi_offset (r0, -224+16)
>>        ...
>>
> 
> Thanks for your clear notes. They were very helpful.
> 
> Indu

Thanks and regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303) and z/VSE Support
+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/