[PATCH v3 00/15] sframe: Enhancements to SFrame info generation

[PATCH v3 00/15] sframe: Enhancements to SFrame info generation

[Jens Remus]

As suggested by Indu I have split my RFC V2 patch series to add initial
support to generate SFrame information on s390x into:

1. This series consisting of all the generic preparatory cleanups,
   enhancements, and fixes to the generation of SFrame information
   in the assembler.

2. A separate RFC series on top with the actual "s390: Initial support
   to generate SFrame info from CFI directives in assembler", which I
   am going to send soon.

Since the original patch series was at V2 I am sending both as V3.


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

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

Patch 4 (new in v3) changes readelf/objdump to display 'f' in the SFrame
RA tracking column, if the architecture is using a fixed RA offset.
Additionally it corrects the logic to display 'u' in the SFrame
FP tracking column.

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

Patches 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 the
separate patch series.

Patch 8 refactors SFrame CFI opcode DW_CFA_register processing into a
separate function. This harmonizes the CFI opcode processing.

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

Patch 10 (new in v3) resolves an issue that causes the assembler to
generate bad SFrame FDE in case the FP without RA was saved on the
stack, which the SFrame format cannot represent. I will send two
alternative solution proposals as RFC.

Patch 11 (new in v3) skips SFrame FDE for .cfi_window_save on all
architectures except AArch64, which multiplexed it with
.cfi_negate_ra_state.

Patches 12 and 13 (both new in v3) resolve issues where generation of
SFrame FDE was unnecessarily skipped.

Path 14 (new in v3) adds tests for the SFrame RA tracking predicate to
places where it was missing to align the logic.

Patch 14 (new in v3) is a minor enhancement to add checks that the
architecture-dependent RA tracking is correctly configured.


Changes v2 -> v3:
- Additional patches as noted in cover letter and patch notes.
- Reword further SFrame macro, variable, and function descriptions
  to align with those previously touched.
- Align description of definition in source with declaration in header.
- Updated gas synthesized CFI test cases for x86 AMD64 to test for
  architecture-specific fixed RA offset instead of using a pattern.
- Updated ld SFrame test cases for x86 AMD64 to test for architecture-
  specific fixed RA offset.
- Updated SFrame warning message texts as suggested by Indu, except for
  the .cfi_def_cfa[_register] ones.
- Do not test sframe_ra_tracking_p when determining the SFrame register
  name in sframe_register_name. The RA register name does not depend on
  whether RA tracking is used or not.
- Corrected formatting of ChangeLog in commit message.

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 (15):
  x86: Remove unused SFrame CFI RA register variable
  gas: Enhance arch-specific SFrame configuration descriptions
  readelf/objdump: Dump SFrame CFA fixed FP and RA offsets
  readelf/objdump: Display SFrame fixed RA offset as 'f' in dump
  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: Refactor SFrame CFI opcode DW_CFA_register processing
  gas: User readable warnings if SFrame FDE is not generated
  gas: Skip SFrame FDE if FP without RA on stack
  gas: Skip SFrame FDE if .cfi_window_save
  gas: Don't skip SFrame FDE if .cfi_register specifies RA w/o tracking
  gas: Don't skip SFrame FDE if .cfi_register specifies SP register
  gas: Test predicate whether SFrame RA tracking is used
  gas: Validate SFrame RA tracking and fixed RA offset

 gas/config/tc-aarch64.c                       |   6 +-
 gas/config/tc-aarch64.h                       |  12 +-
 gas/config/tc-i386.c                          |   6 +-
 gas/config/tc-i386.h                          |  10 +-
 gas/gen-sframe.c                              | 244 +++++++++++++++---
 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      |   6 +-
 .../gas/cfi-sframe/cfi-sframe-common-5.d      |   6 +-
 .../gas/cfi-sframe/cfi-sframe-common-6.d      |   6 +-
 .../gas/cfi-sframe/cfi-sframe-common-7.d      |   6 +-
 .../gas/cfi-sframe/cfi-sframe-common-8.d      |   4 +-
 .../gas/cfi-sframe/cfi-sframe-x86_64-1.d      |   9 +-
 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     |  11 +-
 .../gas/scfi/x86_64/scfi-dyn-stack-1.d        |  11 +-
 ld/testsuite/ld-sframe/discard.s              |   1 -
 ld/testsuite/ld-x86-64/sframe-plt-1.d         |   9 +-
 ld/testsuite/ld-x86-64/sframe-simple-1.d      |  17 +-
 libsframe/sframe-dump.c                       |  18 +-
 24 files changed, 310 insertions(+), 91 deletions(-)

-- 
2.40.1

[PATCH v3 01/15] 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>
Reviewed-by: Indu Bhagat <indu.bhagat@oracle.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---

Notes (jremus):
    Changes v2 -> v3:
    - Corrected formatting of ChangeLog in commit message.

 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 8f6337b34af4..cd20e0c9415e 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -629,7 +629,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 v3 02/15] gas: Enhance arch-specific SFrame configuration descriptions

[Jens Remus]

Explicitly mention "SFrame" in the descriptions for the architecture-
specific SFrame configuration macros, variables, and functions.

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.

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

Align the descriptions for definitions in the source files to the
declarations in the header files.

gas/
	* config/tc-aarch64.h: Enhance architecture-specific SFrame
	configuration descriptions.
	* config/tc-aarch64.c: Likewise.
	* config/tc-i386.h: Likewise.
	* config/tc-i386.c: Likewise.

Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---

Notes (jremus):
    Changes v2 -> v3:
    - Add "SFrame" to architecture-specific SFrame macro, variable, and
      function descriptions as suggested by Indu. Do so for all and not
      only those previously touched.
    - Reword further SFrame macro, variable, and function descriptions
      to align with those previously touched.
    - Align description of definition in source with declaration in header.
    - Corrected formatting of ChangeLog in commit message.
    - Changed commit subject prefix from "sframe" to "gas".

 gas/config/tc-aarch64.c |  6 +++---
 gas/config/tc-aarch64.h | 12 ++++++------
 gas/config/tc-i386.c    |  5 +++++
 gas/config/tc-i386.h    | 10 +++++-----
 4 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/gas/config/tc-aarch64.c b/gas/config/tc-aarch64.c
index 6ad4fae8b0ec..077cbd485979 100644
--- a/gas/config/tc-aarch64.c
+++ b/gas/config/tc-aarch64.c
@@ -8870,7 +8870,7 @@ aarch64_support_sframe_p (void)
   return (aarch64_abi == AARCH64_ABI_LP64);
 }
 
-/* Specify if RA tracking is needed.  */
+/* Whether SFrame return-address tracking is needed.  */
 
 bool
 aarch64_sframe_ra_tracking_p (void)
@@ -8878,8 +8878,8 @@ aarch64_sframe_ra_tracking_p (void)
   return true;
 }
 
-/* Specify the fixed offset to recover RA from CFA.
-   (useful only when RA tracking is not needed).  */
+/* The fixed offset from CFA for SFrame to recover the return address.
+   (useful only when SFrame RA tracking is not needed).  */
 
 offsetT
 aarch64_sframe_cfa_ra_offset (void)
diff --git a/gas/config/tc-aarch64.h b/gas/config/tc-aarch64.h
index 1b8badad9fdc..63acbd46a379 100644
--- a/gas/config/tc-aarch64.h
+++ b/gas/config/tc-aarch64.h
@@ -267,24 +267,24 @@ 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 DWARF register number for SFrame CFA tracking.  */
 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 DWARF register number for SFrame 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 CFA stack trace info.  */
+/* The return-address DWARF register number for SFrame RA tracking.  */
 extern unsigned int aarch64_sframe_cfa_ra_reg;
 #define SFRAME_CFA_RA_REG aarch64_sframe_cfa_ra_reg
 
-/* Specify if RA tracking is needed.  */
+/* Whether SFrame return-address tracking is needed.  */
 extern bool aarch64_sframe_ra_tracking_p (void);
 #define sframe_ra_tracking_p aarch64_sframe_ra_tracking_p
 
-/* Specify the fixed offset to recover RA from CFA.
-   (useful only when RA tracking is not needed).  */
+/* The fixed offset from CFA for SFrame to recover the return address.
+   (useful only when SFrame RA tracking is not needed).  */
 extern offsetT aarch64_sframe_cfa_ra_offset (void);
 #define sframe_cfa_ra_offset aarch64_sframe_cfa_ra_offset
 
diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index cd20e0c9415e..08a8eb2aeb69 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -11181,6 +11181,7 @@ x86_cleanup (void)
     subseg_set (seg, subseg);
 }
 
+/* Whether SFrame stack trace info is supported.  */
 bool
 x86_support_sframe_p (void)
 {
@@ -11188,6 +11189,7 @@ x86_support_sframe_p (void)
   return (x86_elf_abi == X86_64_ABI);
 }
 
+/* Whether SFrame return-address tracking is needed.  */
 bool
 x86_sframe_ra_tracking_p (void)
 {
@@ -11197,6 +11199,8 @@ x86_sframe_ra_tracking_p (void)
   return false;
 }
 
+/* The fixed offset from CFA for SFrame to recover the return address.
+   (useful only when SFrame RA tracking is not needed).  */
 offsetT
 x86_sframe_cfa_ra_offset (void)
 {
@@ -11204,6 +11208,7 @@ x86_sframe_cfa_ra_offset (void)
   return (offsetT) -8;
 }
 
+/* The abi/arch indentifier for SFrame.  */
 unsigned char
 x86_sframe_get_abi_arch (void)
 {
diff --git a/gas/config/tc-i386.h b/gas/config/tc-i386.h
index 7aae7a33dc14..e98362111698 100644
--- a/gas/config/tc-i386.h
+++ b/gas/config/tc-i386.h
@@ -441,20 +441,20 @@ 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 DWARF register number for SFrame 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 DWARF register number for SFrame CFA and FP tracking.  */
 extern unsigned int x86_sframe_cfa_fp_reg;
 #define SFRAME_CFA_FP_REG x86_sframe_cfa_fp_reg
 
-/* Specify if RA tracking is needed.  */
+/* Whether SFrame return-address tracking is needed.  */
 extern bool x86_sframe_ra_tracking_p (void);
 #define sframe_ra_tracking_p x86_sframe_ra_tracking_p
 
-/* Specify the fixed offset to recover RA from CFA.
-   (useful only when RA tracking is not needed).  */
+/* The fixed offset from CFA for SFrame to recover the return address.
+   (useful only when SFrame RA tracking is not needed).  */
 extern offsetT x86_sframe_cfa_ra_offset (void);
 #define sframe_cfa_ra_offset x86_sframe_cfa_ra_offset
 
-- 
2.40.1

[PATCH v3 03/15] 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.

ld/testsuite/
	* ld-x86-64/sframe-plt-1.d: Test for SFrame fixed RA offset.
	* ld-x86-64/sframe-simple-1.d: Likewise.

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

Notes (jremus):
    Changes v2 -> v3:
    - Updated gas synthesized CFI test cases for x86 AMD64 to test for
      architecture-specific fixed RA offset instead of using a pattern.
    - Updated ld SFrame test cases for x86 AMD64 to test for architecture-
      specific fixed RA offset.
    - Corrected formatting of ChangeLog in commit message.

 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 |  1 +
 gas/testsuite/gas/scfi/x86_64/scfi-dyn-stack-1.d    |  1 +
 ld/testsuite/ld-x86-64/sframe-plt-1.d               |  1 +
 ld/testsuite/ld-x86-64/sframe-simple-1.d            |  1 +
 libsframe/sframe-dump.c                             | 10 ++++++++++
 17 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..c45933b72edc 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,7 @@ Contents of the SFrame section .sframe:
 
     Version: SFRAME_VERSION_2
     Flags: NONE
+    CFA fixed RA offset: \-8
     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..6cd0484d5793 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,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/ld/testsuite/ld-x86-64/sframe-plt-1.d b/ld/testsuite/ld-x86-64/sframe-plt-1.d
index 5e734610b970..9d123a73826d 100644
--- a/ld/testsuite/ld-x86-64/sframe-plt-1.d
+++ b/ld/testsuite/ld-x86-64/sframe-plt-1.d
@@ -12,6 +12,7 @@ Contents of the SFrame section .sframe:
 
     Version: SFRAME_VERSION_2
     Flags: SFRAME_F_FDE_SORTED
+    CFA fixed RA offset: \-8
 #...
 
   Function Index :
diff --git a/ld/testsuite/ld-x86-64/sframe-simple-1.d b/ld/testsuite/ld-x86-64/sframe-simple-1.d
index 7f4db31fe1b7..ce5f94386ac2 100644
--- a/ld/testsuite/ld-x86-64/sframe-simple-1.d
+++ b/ld/testsuite/ld-x86-64/sframe-simple-1.d
@@ -12,6 +12,7 @@ Contents of the SFrame section .sframe:
 
     Version: SFRAME_VERSION_2
     Flags: SFRAME_F_FDE_SORTED
+    CFA fixed RA offset: \-8
 #...
 
   Function Index :
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 v3 04/15] readelf/objdump: Display SFrame fixed RA offset as 'f' in dump

[Jens Remus]

For the SFrame FRE frame-pointer (FP) offset from CFA a 'u' is displayed
if it is unavailable.

For the SFrame FRE return-address (RA) offset from CFA a 'u' was
displayed if the ABI uses a fixed RA offset from CFA. By chance a
'u' was also displayed if the RA offset is unavailable, as the string
buffer was not initialized after formatting the FP offset. Note that it
could not occur that the FP offset was erroneously displayed as RA
offset, as the SFrame format cannot have a FRE with FP offset without
RA offset.

For the FRE RA offset display 'f' if the ABI uses a fixed RA offset
from CFA. Display a 'u' if it is unavailable.

libsframe/
	* sframe-dump.c: Display SFrame fixed RA offset as 'f' in dump.

gas/testsuite/
	* gas/cfi-sframe/cfi-sframe-common-4.d: Test for RA displayed
	either as 'u' (if RA tracking) or as 'f' (fixed RA offset if no
	RA tracking).
	* 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 RA displayed
	as 'f' (fixed RA offset), as x86-64 does not use RA tracking.
	* gas/scfi/x86_64/scfi-cfi-sections-1.d: Likewise.
	* gas/scfi/x86_64/scfi-dyn-stack-1.d: Likewise.

ld/testsuite/
	* ld-x86-64/sframe-plt-1.d: Test for RA displayed as 'f' (fixed
	RA offset), as x86-64 does not use RA tracking.
	* ld-x86-64/sframe-simple-1.d: Likewise.

Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---

Notes (jremus):
    Changes v2 -> v3:
    - New patch. If the change in display of fixed RA offset as 'f' is
      undesired the patch can be reduced to fixing the logic to display the
      FP offset.

 .../gas/cfi-sframe/cfi-sframe-common-4.d         |  4 ++--
 .../gas/cfi-sframe/cfi-sframe-common-5.d         |  4 ++--
 .../gas/cfi-sframe/cfi-sframe-common-6.d         |  4 ++--
 .../gas/cfi-sframe/cfi-sframe-common-7.d         |  4 ++--
 .../gas/cfi-sframe/cfi-sframe-common-8.d         |  2 +-
 .../gas/cfi-sframe/cfi-sframe-x86_64-1.d         |  8 ++++----
 .../gas/scfi/x86_64/scfi-cfi-sections-1.d        | 10 +++++-----
 gas/testsuite/gas/scfi/x86_64/scfi-dyn-stack-1.d | 10 +++++-----
 ld/testsuite/ld-x86-64/sframe-plt-1.d            |  8 ++++----
 ld/testsuite/ld-x86-64/sframe-simple-1.d         | 16 ++++++++--------
 libsframe/sframe-dump.c                          |  8 +++++---
 11 files changed, 40 insertions(+), 38 deletions(-)

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 ca559bd0a029..8632613f532f 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-4.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-4.d
@@ -17,7 +17,7 @@ Contents of the SFrame section .sframe:
     func idx \[0\]: pc = 0x0, size = 12 bytes
     STARTPC + CFA + FP + RA +
 #...
-    0+0004 +sp\+16 +u +u +
-    0+0008 +sp\+32 +u +u +
+    0+0004 +sp\+16 +u +[uf] +
+    0+0008 +sp\+32 +u +[uf] +
 
 #pass
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 ee82053e13db..dd2c32d3d9fc 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-5.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-5.d
@@ -17,7 +17,7 @@ Contents of the SFrame section .sframe:
     func idx \[0\]: pc = 0x0, size = 12 bytes
     STARTPC + CFA + FP + RA +
 #...
-    0+0004 +sp\+16 +u +u +
-    0+0008 +sp\+24 +u +u +
+    0+0004 +sp\+16 +u +[uf] +
+    0+0008 +sp\+24 +u +[uf] +
 
 #pass
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 9d54b98552bf..34390c46a074 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-6.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-6.d
@@ -17,7 +17,7 @@ Contents of the SFrame section .sframe:
     func idx \[0\]: pc = 0x0, size = 12 bytes
     STARTPC + CFA + FP + RA +
 #...
-    0+0004 +sp\+8 +u +u +
-    0+0008 +sp\+8 +u +u +
+    0+0004 +sp\+8 +u +[uf] +
+    0+0008 +sp\+8 +u +[uf] +
 
 #pass
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 2b7fe3aec8f4..61efb9c4ed12 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-7.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-7.d
@@ -17,7 +17,7 @@ Contents of the SFrame section .sframe:
     func idx \[0\]: pc = 0x0, size = 12 bytes
     STARTPC + CFA + FP + RA +
 #...
-    0+0004 +sp\+8 +u +u +
-    0+0008 +sp\+8 +u +u +
+    0+0004 +sp\+8 +u +[uf] +
+    0+0008 +sp\+8 +u +[uf] +
 
 #pass
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 d654e1d0bcd4..d77645636b36 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-8.d
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-8.d
@@ -17,6 +17,6 @@ Contents of the SFrame section .sframe:
     func idx \[0\]: pc = 0x0, size = 8 bytes
     STARTPC + CFA + FP + RA +
 #...
-    0+0004 +sp\+16 +u +u +
+    0+0004 +sp\+16 +u +[uf] +
 
 #pass
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 c8b5e6adfea0..88b4cc63dbaa 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
@@ -16,8 +16,8 @@ Contents of the SFrame section .sframe:
 
     func idx \[0\]: pc = 0x0, size = 25 bytes
     STARTPC +CFA +FP +RA +
-    0+0000 +sp\+8 +u +u +
-    0+0001 +sp\+16 +c\-16 +u +
-    0+0004 +fp\+16 +c\-16 +u +
-    0+0018 +sp\+8 +c\-16 +u +
+    0+0000 +sp\+8 +u +f +
+    0+0001 +sp\+16 +c\-16 +f +
+    0+0004 +fp\+16 +c\-16 +f +
+    0+0018 +sp\+8 +c\-16 +f +
 #pass
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 c45933b72edc..7c247e33a6e8 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
@@ -16,10 +16,10 @@ Contents of the SFrame section .sframe:
 
     func idx \[0\]: pc = 0x0, size = 12 bytes
     STARTPC + CFA + FP + RA +
-    0+0000 +sp\+8 +u +u +
-    0+0001 +sp\+16 +c\-16 +u +
-    0+0004 +fp\+16 +c-16 +u +
-    0+000a +sp\+16 +c\-16 +u +
-    0+000b +sp\+8 +u +u +
+    0+0000 +sp\+8 +u +f +
+    0+0001 +sp\+16 +c\-16 +f +
+    0+0004 +fp\+16 +c-16 +f +
+    0+000a +sp\+16 +c\-16 +f +
+    0+000b +sp\+8 +u +f +
 
 #pass
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 6cd0484d5793..c6a9b53f4e09 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
@@ -16,10 +16,10 @@ Contents of the SFrame section .sframe:
   Function Index :
 
     func idx \[0\]: pc = 0x0, size = 87 bytes
-    STARTPC + CFA + FP + RA           
-    0+0000 + sp\+8 + u + u            
-    0+0001 + sp\+16 + c-16 + u            
-    0+0004 + fp\+16 + c-16 + u            
-    0+0056 + sp\+8 + u + u            
+    STARTPC + CFA + FP + RA +
+    0+0000 + sp\+8 + u + f +
+    0+0001 + sp\+16 + c-16 + f +
+    0+0004 + fp\+16 + c-16 + f +
+    0+0056 + sp\+8 + u + f +
 
 #pass
diff --git a/ld/testsuite/ld-x86-64/sframe-plt-1.d b/ld/testsuite/ld-x86-64/sframe-plt-1.d
index 9d123a73826d..617fb9950acc 100644
--- a/ld/testsuite/ld-x86-64/sframe-plt-1.d
+++ b/ld/testsuite/ld-x86-64/sframe-plt-1.d
@@ -19,12 +19,12 @@ Contents of the SFrame section .sframe:
 
     func idx \[0\]: pc = 0x1000, size = 16 bytes
     STARTPC +CFA +FP +RA +
-    0+1000 +sp\+16 +u +u +
-    0+1006 +sp\+24 +u +u +
+    0+1000 +sp\+16 +u +f +
+    0+1006 +sp\+24 +u +f +
 
     func idx \[1\]: pc = 0x1010, size = 16 bytes
     STARTPC\[m\] +CFA +FP +RA +
-    0+0000 +sp\+8 +u +u +
-    0+000b +sp\+16 +u +u +
+    0+0000 +sp\+8 +u +f +
+    0+000b +sp\+16 +u +f +
 
 #...
diff --git a/ld/testsuite/ld-x86-64/sframe-simple-1.d b/ld/testsuite/ld-x86-64/sframe-simple-1.d
index ce5f94386ac2..18c863938d39 100644
--- a/ld/testsuite/ld-x86-64/sframe-simple-1.d
+++ b/ld/testsuite/ld-x86-64/sframe-simple-1.d
@@ -23,14 +23,14 @@ Contents of the SFrame section .sframe:
 
     func idx \[2\]: pc = 0x1020, size = 53 bytes
     STARTPC +CFA +FP +RA +
-    0+1020 +sp\+8 +u +u +
-    0+1021 +sp\+16 +c-16 +u +
-    0+1024 +fp\+16 +c-16 +u +
-    0+1054 +sp\+8 +c-16 +u +
+    0+1020 +sp\+8 +u +f +
+    0+1021 +sp\+16 +c-16 +f +
+    0+1024 +fp\+16 +c-16 +f +
+    0+1054 +sp\+8 +c-16 +f +
 
     func idx \[3\]: pc = 0x1055, size = 37 bytes
     STARTPC +CFA +FP +RA +
-    0+1055 +sp\+8 +u +u +
-    0+1056 +sp\+16 +c-16 +u +
-    0+1059 +fp\+16 +c-16 +u +
-    0+1079 +sp\+8 +c-16 +u +
+    0+1055 +sp\+8 +u +f +
+    0+1056 +sp\+16 +c-16 +f +
+    0+1059 +fp\+16 +c-16 +f +
+    0+1079 +sp\+8 +c-16 +f +
diff --git a/libsframe/sframe-dump.c b/libsframe/sframe-dump.c
index 493d052ce91f..69633d53a33a 100644
--- a/libsframe/sframe-dump.c
+++ b/libsframe/sframe-dump.c
@@ -181,13 +181,15 @@ dump_sframe_func_with_fres (sframe_decoder_ctx *sfd_ctx,
       printf ("%-10s", temp);
 
       /* Dump RA info.
-	 If an ABI does not track RA offset, e.g., AMD64, display a 'u',
+	 If an ABI does not track RA offset, e.g., AMD64, display 'f',
 	 else display the offset d as 'c+-d'.  */
-      if (sframe_decoder_get_fixed_ra_offset(sfd_ctx)
+      if (sframe_decoder_get_fixed_ra_offset (sfd_ctx)
 	  != SFRAME_CFA_FIXED_RA_INVALID)
-	strcpy (temp, "u");
+	strcpy (temp, "f");
       else if (err[2] == 0)
 	sprintf (temp, "c%+d", ra_offset);
+      else
+	strcpy (temp, "u");
 
       /* Mark SFrame FRE's RA information with "[s]" if the RA is mangled
 	 with signature bits.  */
-- 
2.40.1

[PATCH v3 05/15] 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.

Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
Reviewed-by: Indu Bhagat <indu.bhagat@oracle.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---

Notes (jremus):
    Changes v2 -> v3:
    - Removed stale ChangeLog entries from commit message.
    - Corrected formatting of ChangeLog in commit message.

 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 v3 06/15] 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>
Reviewed-by: Indu Bhagat <indu.bhagat@oracle.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---

Notes (jremus):
    Changes v2 -> v3:
    - Corrected formatting of ChangeLog in commit message.
    
    Changes 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 v3 07/15] 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>
Reviewed-by: Indu Bhagat <indu.bhagat@oracle.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---

Notes (jremus):
    Changes v2 -> v3:
    - Enhance comment in code as suggested by Indu.
    - Corrected formatting of ChangeLog in commit message.
    
    Without this patch the assembler would generate incomplete SFrame
    information without warning 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                              | 10 +++++++---
 gas/testsuite/gas/cfi-sframe/common-empty-3.d |  1 +
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index 1269b2b77c54..fe8b24906c1b 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -1343,9 +1343,12 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
 
   xlate_ctx->dw_fde = dw_fde;
 
-  /* If the return column is not RIP, SFrame format cannot represent it.  */
+  /* 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
 #...
-- 
2.40.1

[PATCH v3 08/15] gas: Refactor SFrame CFI opcode DW_CFA_register processing

[Jens Remus]

Refactor SFrame processing of CFI opcode DW_CFA_register into a separate
function. This harmonizes the CFI opcode processing.

While at it reword the comment on CFI opcodes that are not processed.

This is a purely mechanical change.

gas/
	* gen-sframe.c (sframe_do_cfi_insn, sframe_xlate_do_register):
	Refactor SFrame CFI opcode DW_CFA_register processing.

Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---

Notes (jremus):
    Changes v2 -> v3:
    - New patch.

 gas/gen-sframe.c | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index fe8b24906c1b..f47f51c7a354 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -1108,6 +1108,28 @@ sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
   return SFRAME_XLATE_OK;
 }
 
+/* Translate DW_CFA_register into SFrame context.
+   Return SFRAME_XLATE_OK if success.  */
+
+static int
+sframe_xlate_do_register (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
+			  struct cfi_insn_data *cfi_insn)
+{
+  /* Previous value of register1 is register2.  However, if the specified
+     register1 is not interesting (SP, FP, or RA reg), the current DW_CFA_register
+     instruction can be safely skipped without sacrificing the asynchronicity of
+     stack trace information.  */
+  if (cfi_insn->u.rr.reg1 == SFRAME_CFA_SP_REG
+#ifdef SFRAME_FRE_RA_TRACKING
+      || (cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG)
+#endif
+      || cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
+    return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
+
+  /* Safe to skip.  */
+  return SFRAME_XLATE_OK;
+}
+
 /* Translate DW_CFA_remember_state into SFrame context.
    Return SFRAME_XLATE_OK if success.  */
 
@@ -1296,19 +1318,12 @@ sframe_do_cfi_insn (struct sframe_xlate_ctx *xlate_ctx,
     case DW_CFA_GNU_window_save:
       err = sframe_xlate_do_gnu_window_save (xlate_ctx, cfi_insn);
       break;
-    /* Other CFI opcodes are not processed at this time.
-       These do not impact the coverage of the basic stack tracing
-       information as conveyed in the SFrame format.
-	- DW_CFA_register,
-	- etc.  */
     case DW_CFA_register:
-      if (cfi_insn->u.rr.reg1 == SFRAME_CFA_SP_REG
-#ifdef SFRAME_FRE_RA_TRACKING
-	  || cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG
-#endif
-	  || cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
-	err = SFRAME_XLATE_ERR_NOTREPRESENTED;
+      err = sframe_xlate_do_register (xlate_ctx, cfi_insn);
       break;
+    /* Following CFI opcodes are not processed at this time.
+       These do not impact the coverage of the basic stack tracing
+       information as conveyed in the SFrame format.  */
     case DW_CFA_undefined:
     case DW_CFA_same_value:
       break;
-- 
2.40.1

[PATCH v3 09/15] 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 defining non-SP/FP register
  <regno> as CFA base register
- skipping SFrame FDE due to .cfi_def_cfa_register defining non-SP/FP
  register <regno> as CFA base register
- skipping SFrame FDE due to .cfi_def_cfa_offset without CFA base
  register in effect
- skipping SFrame FDE due to .cfi_def_cfa_offset with non-SP/FP register
  <regno> as CFA base register in effect
- skipping SFrame FDE due to .cfi_val_offset specifying {FP|RA} register
- skipping SFrame FDE due to .cfi_remember_state without SFrame FRE
  state
- skipping SFrame FDE due to .cfi_register specifying {SP|FP|RA}
  register
- skipping SFrame FDE due to non-default return-address register <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.

gas/testsuite/
	* gas/cfi-sframe/common-empty-1.d: Update generic SFrame test
	case to updated warning message texts.
	* gas/cfi-sframe/common-empty-2.d: Likewise.
	* gas/cfi-sframe/common-empty-3.d: Likewise.

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

Notes (jremus):
    Changes v2 -> v3:
    - Updated SFrame warning message texts as suggested by Indu, except for
      the .cfi_def_cfa[_register] ones. 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? Anyhow, Indu, if you still prefer your suggestions I
      would go with yours.
      The ".cfi_remember_state without SFrame FRE state" warning needs to
      remain, as there would otherwise not be any warning at all. The reason
      is that sframe_do_cfi_insn now expects any of its called CFI
      processing functions to emit a warning, if they return with an error,
      such as SFRAME_XLATE_ERR_NOTREPRESENTED. Silently skipping SFrame FDE
      does not seem an acceptable approach to me.
    - Do not test sframe_ra_tracking_p when determining the SFrame register
      name in sframe_register_name. The RA register name does not depend on
      whether RA tracking is used or not.
    - Corrected formatting of ChangeLog in commit message.

 gas/gen-sframe.c                              | 95 ++++++++++++++-----
 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, 75 insertions(+), 28 deletions(-)

diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index f47f51c7a354..a3b6f75cfe85 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,23 @@ 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 (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 +1007,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 defining "
+		 "non-SP/FP register %u as CFA base register"),
+	       cfi_insn->u.r);
+      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 +1037,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 defining "
+		 "non-SP/FP register %u as CFA base register"),
+	       cfi_insn->u.r);
+      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 +1073,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 with "
+		   "non-SP/FP register %u as CFA base register in effect"),
+		 cur_fre->cfa_base_reg);
+      return SFRAME_XLATE_ERR_NOTREPRESENTED;
+    }
 
   return SFRAME_XLATE_OK;
 }
@@ -1096,13 +1132,16 @@ sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
      register is not interesting (FP or RA reg), the current DW_CFA_val_offset
      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.  */
+  if (cfi_insn->u.r == SFRAME_CFA_FP_REG
 #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.  */
+      || (sframe_ra_tracking_p () && cfi_insn->u.r == SFRAME_CFA_RA_REG)
 #endif
+      )
+    {
+      as_warn (_("skipping SFrame FDE due to .cfi_val_offset specifying %s register"),
+	       sframe_register_name (cfi_insn->u.r));
+      return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
+    }
 
   /* Safe to skip.  */
   return SFRAME_XLATE_OK;
@@ -1124,7 +1163,11 @@ sframe_xlate_do_register (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
       || (cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG)
 #endif
       || cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
-    return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
+    {
+      as_warn (_("skipping SFrame FDE due to .cfi_register specifying %s register"),
+	       sframe_register_name (cfi_insn->u.rr.reg1));
+      return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
+    }
 
   /* Safe to skip.  */
   return SFRAME_XLATE_OK;
@@ -1142,7 +1185,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 ();
@@ -1330,21 +1376,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;
 }
 
@@ -1361,7 +1405,8 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
   /* SFrame format cannot represent a non-default DWARF return column reg.  */
   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 return-address register %u"),
+	       xlate_ctx->dw_fde->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..5cc37d5ff440 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 return-address register 0
 #objdump: --sframe=.sframe
 #name: SFrame supports only default return column
 #...
-- 
2.40.1

[PATCH v3 10/15] gas: Skip SFrame FDE if FP without RA on stack

[Jens Remus]

The SFrame format cannot represent the frame pointer (FP) being saved
on the stack without the return address (RA) also being saved on the
stack, if RA tracking is used.

A SFrame FDE is followed by 1-3 offsets with the following information:

Without RA tracking:
1. Offset from base pointer (SP or FP) to locate the CFA
2. Optional: Offset to CFA to restore the frame pointer (FP)

With RA tracking:
1. Offset from base pointer (SP or FP) to locate the CFA
2. Optional: Offset to CFA to restore the return address (RA)
3. Optional: Offset to CFA to restore the frame pointer (FP)

When RA tracking is used and a FDE is followed by two offsets the
SFrame format does not provide any information to distinguish whether
the second offset is the RA or FP offset. SFrame assumes the offset to
be the RA offset, which may be wrong.

Therefore skip generation of SFrame FDE information and print the
following warning, if RA tracking is used and the FP is saved on the
stack without the RA being saved as well:

  skipping SFrame FDE due to FP without RA on stack

gas/
	* gen-sframe.c (sframe_do_fde): Skip SFrame FDE if FP without RA
	on stack, as the SFrame format cannot represent this case.

Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---

Notes (jremus):
    Changes v2 -> v3:
    - New patch.
    
    Without this patch the assembler would generate incorrect SFrame
    information without warning for the s390-specific SFrame error test
    case 5, that gets introduced by patch "s390: Initial support to
    generate .sframe from CFI directives in assembler". The FRE would
    be followed by two offsets for the CFA and FP. SFrame would
    erroneously interpret them as CFA and RA offsets, as it cannot
    represent FP without RA on stack.

 gas/gen-sframe.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index a3b6f75cfe85..87be3eb05ad2 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -1439,6 +1439,25 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
 	= get_dw_fde_end_addrS (xlate_ctx->dw_fde);
     }
 
+#ifdef SFRAME_FRE_RA_TRACKING
+  if (sframe_ra_tracking_p ())
+    {
+      struct sframe_row_entry *fre;
+
+      /* Iterate over the scratchpad FREs and validate them.  */
+      for (fre = xlate_ctx->first_fre; fre; fre = fre->next)
+	{
+	  /* SFrame format cannot represent FP on stack without RA on stack.  */
+	  if (fre->ra_loc != SFRAME_FRE_ELEM_LOC_STACK
+	      && fre->bp_loc == SFRAME_FRE_ELEM_LOC_STACK)
+	    {
+	      as_warn (_("skipping SFrame FDE due to FP without RA on stack"));
+	      return SFRAME_XLATE_ERR_NOTREPRESENTED;
+	    }
+	}
+    }
+#endif /* SFRAME_FRE_RA_TRACKING  */
+
   return SFRAME_XLATE_OK;
 }
 
-- 
2.40.1

[PATCH v3 11/15] gas: Skip SFrame FDE if .cfi_window_save

[Jens Remus]

CFI opcode DW_CFA_AARCH64_negate_ra_state is multiplexed with
DW_CFA_GNU_window_save. Process DW_CFA_AARCH64_negate_ra_state on
AArch64. Skip generation of SFrame FDE otherwise.

gas/
	* gen-sframe.c: Skip SFrame FDE if .cfi_window_save.

Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---

Notes (jremus):
    Changes v2 -> v3:
    - New patch. The intention is to skip all "unknown" CFI opcodes,
      which SFrame does not explicitly handle. DW_CFA_GNU_window_save
      seems to be handled only for the AArch64-specific multiplexed
      DW_CFA_AARCH64_negate_ra_state. The logic could be changed to be
      dependent on TC_AARCH64 at build-time instead of
      sframe_get_abi_arch() at run-time.

 gas/gen-sframe.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index 87be3eb05ad2..98166de8cc01 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -1257,12 +1257,12 @@ sframe_xlate_do_restore (struct sframe_xlate_ctx *xlate_ctx,
   return SFRAME_XLATE_OK;
 }
 
-/* Translate DW_CFA_GNU_window_save into SFrame context.
+/* Translate DW_CFA_AARCH64_negate_ra_state into SFrame context.
    Return SFRAME_XLATE_OK if success.  */
 
 static int
-sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
-				 struct cfi_insn_data *cfi_insn ATTRIBUTE_UNUSED)
+sframe_xlate_do_aarch64_negate_ra_state (struct sframe_xlate_ctx *xlate_ctx,
+					 struct cfi_insn_data *cfi_insn ATTRIBUTE_UNUSED)
 {
   struct sframe_row_entry *cur_fre = xlate_ctx->cur_fre;
 
@@ -1274,6 +1274,25 @@ sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
   return SFRAME_XLATE_OK;
 }
 
+/* Translate DW_CFA_GNU_window_save into SFrame context.
+   DW_CFA_AARCH64_negate_ra_state is multiplexed with DW_CFA_GNU_window_save.
+   Return SFRAME_XLATE_OK if success.  */
+
+static int
+sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
+				 struct cfi_insn_data *cfi_insn)
+{
+  unsigned char abi_arch = sframe_get_abi_arch ();
+
+  /* Translate DW_CFA_AARCH64_negate_ra_state into SFrame context.  */
+  if (abi_arch == SFRAME_ABI_AARCH64_ENDIAN_BIG
+      || abi_arch == SFRAME_ABI_AARCH64_ENDIAN_LITTLE)
+    return sframe_xlate_do_aarch64_negate_ra_state (xlate_ctx, cfi_insn);
+
+  as_warn (_("skipping SFrame FDE due to .cfi_window_save"));
+  return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
+}
+
 /* Returns the DWARF call frame instruction name or fake CFI name for the
    specified CFI opcode, or NULL if the value is not recognized.  */
 
-- 
2.40.1

[PATCH v3 12/15] gas: Don't skip SFrame FDE if .cfi_register specifies RA w/o tracking

[Jens Remus]

Do not skip SFrame FDE if .cfi_register specifies RA register without
RA tracking being actually used. Without RA tracking the register
contents can always be restored from the stack using the fixed
RA offset from CFA.

gas/
	* gen-sframe.c (sframe_xlate_do_register): Do not skip SFrame
	FDE if .cfi_register specifies RA register without RA tracking
	being used.

Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---

Notes (jremus):
    Changes v2 -> v3:
    - New patch.

 gas/gen-sframe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index 98166de8cc01..61c846f214ee 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -1160,7 +1160,7 @@ sframe_xlate_do_register (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
      stack trace information.  */
   if (cfi_insn->u.rr.reg1 == SFRAME_CFA_SP_REG
 #ifdef SFRAME_FRE_RA_TRACKING
-      || (cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG)
+      || (sframe_ra_tracking_p () && cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG)
 #endif
       || cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
     {
-- 
2.40.1

[PATCH v3 13/15] gas: Don't skip SFrame FDE if .cfi_register specifies SP register

[Jens Remus]

The stack-pointer (SP) register contents on entry can be reconstructed
from the CFA base register tracking information from the current SFrame
FRE and initial FRE from the FDE:

1. Compute CFA from the current CFA base register (SP or FP) and CFA
   offset from the SFrame CFA base register tracking information from
   the SFrame FRE for the current instruction address:

     CFA = <current_base_reg> + <current_cfa_offset>

2. Compute SP from the current CFA and the CFA offset from the SFrame
   CFA tracking information from the initial SFrame FRE of the FDE:

     SP = CFA - <initial_cfa_offset>

While at it add a comment to the processing of .cfi_val_offset that the
SP can be reconstructed from the CFA base register tracking information.

gas/
	* gen-sframe.c (sframe_xlate_do_register): Do not skip SFrame
	FDE if .cfi_register specifies SP register.
	(sframe_xlate_do_val_offset): Add comment that this is likewise.

Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---

Notes (jremus):
    Changes v2 -> v3:
    - New patch.

 gas/gen-sframe.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index 61c846f214ee..12b523a8d59a 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -1136,6 +1136,7 @@ sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
 #ifdef SFRAME_FRE_RA_TRACKING
       || (sframe_ra_tracking_p () && cfi_insn->u.r == SFRAME_CFA_RA_REG)
 #endif
+      /* Ignore SP reg, as it can be recovered from the CFA tracking info.  */
       )
     {
       as_warn (_("skipping SFrame FDE due to .cfi_val_offset specifying %s register"),
@@ -1155,14 +1156,15 @@ sframe_xlate_do_register (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
 			  struct cfi_insn_data *cfi_insn)
 {
   /* Previous value of register1 is register2.  However, if the specified
-     register1 is not interesting (SP, FP, or RA reg), the current DW_CFA_register
+     register1 is not interesting (FP or RA reg), the current DW_CFA_register
      instruction can be safely skipped without sacrificing the asynchronicity of
      stack trace information.  */
-  if (cfi_insn->u.rr.reg1 == SFRAME_CFA_SP_REG
+  if (cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG
 #ifdef SFRAME_FRE_RA_TRACKING
       || (sframe_ra_tracking_p () && cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG)
 #endif
-      || cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
+      /* Ignore SP reg, as it can be recovered from the CFA tracking info.  */
+      )
     {
       as_warn (_("skipping SFrame FDE due to .cfi_register specifying %s register"),
 	       sframe_register_name (cfi_insn->u.rr.reg1));
-- 
2.40.1

[PATCH v3 14/15] gas: Test predicate whether SFrame RA tracking is used

[Jens Remus]

The existence of the macro SFRAME_FRE_RA_TRACKING only ensures the
existence of the macro SFRAME_CFA_RA_REG and the predicate function
sframe_ra_tracking_p. It does not indicate whether SFrame RA tracking
is actually used.

Test the return value of the SFrame RA tracking predicate function
sframe_ra_tracking_p to determine whether RA tracking is used.

This aligns the logic in functions get_fre_num_offsets and
output_sframe_row_entry to the one used in all other places.

gas/
	* gen-sframe.c (get_fre_num_offsets, output_sframe_row_entry):
	Test predicate to determine whether SFrame RA tracking is used.

Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---

Notes (jremus):
    Changes v2 -> v3:
    - New patch.

 gas/gen-sframe.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index 12b523a8d59a..ca6565b0e45e 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -346,7 +346,8 @@ get_fre_num_offsets (struct sframe_row_entry *sframe_fre)
   if (sframe_fre->bp_loc == SFRAME_FRE_ELEM_LOC_STACK)
     fre_num_offsets++;
 #ifdef SFRAME_FRE_RA_TRACKING
-  if (sframe_fre->ra_loc == SFRAME_FRE_ELEM_LOC_STACK)
+  if (sframe_ra_tracking_p ()
+      && sframe_fre->ra_loc == SFRAME_FRE_ELEM_LOC_STACK)
     fre_num_offsets++;
 #endif
   return fre_num_offsets;
@@ -536,7 +537,8 @@ output_sframe_row_entry (symbolS *fde_start_addr,
   fre_write_offsets++;
 
 #ifdef SFRAME_FRE_RA_TRACKING
-  if (sframe_fre->ra_loc == SFRAME_FRE_ELEM_LOC_STACK)
+  if (sframe_ra_tracking_p ()
+      && sframe_fre->ra_loc == SFRAME_FRE_ELEM_LOC_STACK)
     {
       fre_offset_func_map[idx].out_func (sframe_fre->ra_offset);
       fre_write_offsets++;
-- 
2.40.1

[PATCH v3 15/15] gas: Validate SFrame RA tracking and fixed RA offset

[Jens Remus]

If an architecture uses SFrame return-address (RA) tracking it must
specify the fixed RA offset as invalid. Otherwise, if an architecture
does not use RA tracking, it must specify a valid fixed RA offset.

gas/
	* gen-sframe.c: Validate SFrame RA tracking and fixed
	RA offset.

Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---

Notes (jremus):
    Changes v2 -> v3:
    - New patch.
    
    This could be made dependent on ENABLE_CHECKING (configure option
    --enable-checking).

 gas/gen-sframe.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index ca6565b0e45e..7e815f9603ef 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -1532,6 +1532,18 @@ output_sframe (segT sframe_seg)
   /* Setup the version specific access functions.  */
   sframe_set_version (SFRAME_VERSION_2);
 
+#ifdef SFRAME_FRE_RA_TRACKING
+  if (sframe_ra_tracking_p ())
+    /* With RA tracking the fixed RA offset must be invalid.  */
+    gas_assert (sframe_cfa_ra_offset () == SFRAME_CFA_FIXED_RA_INVALID);
+  else
+    /* Without RA tracking the fixed RA offset may not be invalid.  */
+    gas_assert (sframe_cfa_ra_offset () != SFRAME_CFA_FIXED_RA_INVALID);
+#else
+  /* Without RA tracking the fixed RA offset may not be invalid.  */
+  gas_assert (sframe_cfa_ra_offset () != SFRAME_CFA_FIXED_RA_INVALID);
+#endif
+
   /* Process all fdes and create SFrame stack trace information.  */
   create_sframe_all ();
 
-- 
2.40.1

Re: [PATCH v3 10/15] gas: Skip SFrame FDE if FP without RA on stack

[Jens Remus]

Hello Indu,

Am 12.04.2024 um 16:47 schrieb Jens Remus:
> The SFrame format cannot represent the frame pointer (FP) being saved
> on the stack without the return address (RA) also being saved on the
> stack, if RA tracking is used.

[...]

> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index a3b6f75cfe85..87be3eb05ad2 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -1439,6 +1439,25 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
>   	= get_dw_fde_end_addrS (xlate_ctx->dw_fde);
>       }
>   
> +#ifdef SFRAME_FRE_RA_TRACKING
> +  if (sframe_ra_tracking_p ())
> +    {
> +      struct sframe_row_entry *fre;
> +
> +      /* Iterate over the scratchpad FREs and validate them.  */
> +      for (fre = xlate_ctx->first_fre; fre; fre = fre->next)
> +	{
> +	  /* SFrame format cannot represent FP on stack without RA on stack.  */
> +	  if (fre->ra_loc != SFRAME_FRE_ELEM_LOC_STACK
> +	      && fre->bp_loc == SFRAME_FRE_ELEM_LOC_STACK)
> +	    {
> +	      as_warn (_("skipping SFrame FDE due to FP without RA on stack"));
> +	      return SFRAME_XLATE_ERR_NOTREPRESENTED;
> +	    }
> +	}
> +    }
> +#endif /* SFRAME_FRE_RA_TRACKING  */
> +
>     return SFRAME_XLATE_OK;
>   }

I noticed that above new warning is erroneously emitted when assembling 
the following CFI directive sequence with option "-alh" (to output a 
listing of the assembly; probably any "-a[...]") on a SFrame enabled 
target, that uses FP and RA tracking.

.cfi_offset <fp-regno>, <fp-offset>
.cfi_offset <ra-regno>, <ra-offset>

The reason is that with listings enabled there is an additional DWARF 
DW_CFA_advance_loc CFI instruction (with a zero advance) between both 
DW_CFA_offset instructions, that the DWARF .eh_frame generator is able 
to process correctly, but causes the .sframe generator to choke.

Additionally with this patch reverted "bad" SFrame information is 
generated (see example below), where there are multiple SFrame FREs for 
the same PC start address.
Note that the FP-tracking information erroneously being displayed in the 
RA-tracking column, is why I introduced this new warning message. I will 
send two alternative patches how to potentially resolve that soon.

$ cat test_fpra_min.s
         .cfi_sections .sframe, .eh_frame
         .cfi_startproc
         stmg    %r11,%r15,88(%r15)
         .cfi_rel_offset 11, 88
         .cfi_rel_offset 14, 112
         la      %r11,0
         la      %r14,0
.Lreturn:
         lmg     %r11,%r15,88(%r15)
         .cfi_restore 14
         .cfi_restore 11
         br      %r14
         .cfi_endproc

$ ojbdump --sframe test_fpra_without-alh.o
...
   Function Index :

     func idx [0]: pc = 0x0, size = 22 bytes
     STARTPC         CFA       FP        RA
     0000000000000000  sp+160    u         u
     0000000000000006  sp+160    c-72      c-48
     0000000000000014  sp+160    u         u

$ objdump --sframe test_fpra_with_alh.o
...
   Function Index :

     func idx [0]: pc = 0x0, size = 22 bytes
     STARTPC         CFA       FP        RA
     0000000000000000  sp+160    u         u
     0000000000000006  sp+160    u         c-72
     0000000000000006  sp+160    c-72      c-48
     0000000000000014  sp+160    u         c-72
     0000000000000014  sp+160    u         u

Note that the outputs of "objdump -Wf" and "objdump -WF" are identical 
in both cases (with and without option "-alh").

Debugging of the SFrame processing of the DWARF CFI instructions shows 
that with option "-a" there are additional DW_CFA_advance_loc:

DW_CFA_def_cfa: reg=15 offset=160
DW_CFA_advance_loc: lab1=L0, lab2=L0
DW_CFA_offset: reg=11 offset=-72
DW_CFA_advance_loc: lab1=L0, lab2=L0   <-- only with -a
DW_CFA_offset: reg=14 offset=-48
DW_CFA_advance_loc: lab1=L0, lab2=L0
DW_CFA_restore: reg=14
DW_CFA_advance_loc: lab1=L0, lab2=L0   <-- only with -a
DW_CFA_restore: reg=11

Debugging of the CFI directive processing in gas/dw2gencfi.c shows the 
following:

- With option "-a" cfi_add_advance_loc() is invoked more often in 
dot_cfi() due to the condition (symbol_get_frag 
(frchain_now->frch_cfi_data->last_address) != frag_now) evaluating to true.

- output_cfi_insn() of case DW_CFA_advance_loc enters the condition 
(symbol_get_frag (to) == symbol_get_frag (from)) without option "-a" and 
enters the else condition with option "-a". The else path has an 
interesting comment that suggests that there is logic to relax an 
advance by zero at a later stage:

"... Call frag_grow with the sum of room needed by frag_more and 
frag_var to preallocate space ensuring that the DW_CFA_advance_loc4 is 
in the fixed part of the rs_cfa frag, so that the relax machinery can 
remove the advance_loc should it advance by zero."

I don't have a clue how to resolve this potential issue in the SFrame 
generation. I could not figure out how to detect the advance of zero in 
the SFrame processing of DW_CFA_advance_loc, so that it could be treated 
special.
I can open a ticket in the Sourceware Bugzilla, if you agree that this 
is an issue.

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 v3 10/15] gas: Skip SFrame FDE if FP without RA on stack

[Indu Bhagat]

On 4/16/24 06:14, Jens Remus wrote:
> Hello Indu,
> 
> Am 12.04.2024 um 16:47 schrieb Jens Remus:
>> The SFrame format cannot represent the frame pointer (FP) being saved
>> on the stack without the return address (RA) also being saved on the
>> stack, if RA tracking is used.
> 
> [...]
> 
>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>> index a3b6f75cfe85..87be3eb05ad2 100644
>> --- a/gas/gen-sframe.c
>> +++ b/gas/gen-sframe.c
>> @@ -1439,6 +1439,25 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
>>       = get_dw_fde_end_addrS (xlate_ctx->dw_fde);
>>       }
>> +#ifdef SFRAME_FRE_RA_TRACKING
>> +  if (sframe_ra_tracking_p ())
>> +    {
>> +      struct sframe_row_entry *fre;
>> +
>> +      /* Iterate over the scratchpad FREs and validate them.  */
>> +      for (fre = xlate_ctx->first_fre; fre; fre = fre->next)
>> +    {
>> +      /* SFrame format cannot represent FP on stack without RA on 
>> stack.  */
>> +      if (fre->ra_loc != SFRAME_FRE_ELEM_LOC_STACK
>> +          && fre->bp_loc == SFRAME_FRE_ELEM_LOC_STACK)
>> +        {
>> +          as_warn (_("skipping SFrame FDE due to FP without RA on 
>> stack"));
>> +          return SFRAME_XLATE_ERR_NOTREPRESENTED;
>> +        }
>> +    }
>> +    }
>> +#endif /* SFRAME_FRE_RA_TRACKING  */
>> +
>>     return SFRAME_XLATE_OK;
>>   }
> 
> I noticed that above new warning is erroneously emitted when assembling 
> the following CFI directive sequence with option "-alh" (to output a 
> listing of the assembly; probably any "-a[...]") on a SFrame enabled 
> target, that uses FP and RA tracking.
> 
> .cfi_offset <fp-regno>, <fp-offset>
> .cfi_offset <ra-regno>, <ra-offset>
> 
> The reason is that with listings enabled there is an additional DWARF 
> DW_CFA_advance_loc CFI instruction (with a zero advance) between both 
> DW_CFA_offset instructions, that the DWARF .eh_frame generator is able 
> to process correctly, but causes the .sframe generator to choke.
> 
> Additionally with this patch reverted "bad" SFrame information is 
> generated (see example below), where there are multiple SFrame FREs for 
> the same PC start address.
> Note that the FP-tracking information erroneously being displayed in the 
> RA-tracking column, is why I introduced this new warning message. I will 
> send two alternative patches how to potentially resolve that soon.
> 
> $ cat test_fpra_min.s
>          .cfi_sections .sframe, .eh_frame
>          .cfi_startproc
>          stmg    %r11,%r15,88(%r15)
>          .cfi_rel_offset 11, 88
>          .cfi_rel_offset 14, 112
>          la      %r11,0
>          la      %r14,0
> .Lreturn:
>          lmg     %r11,%r15,88(%r15)
>          .cfi_restore 14
>          .cfi_restore 11
>          br      %r14
>          .cfi_endproc
> 
> $ ojbdump --sframe test_fpra_without-alh.o
> ...
>    Function Index :
> 
>      func idx [0]: pc = 0x0, size = 22 bytes
>      STARTPC         CFA       FP        RA
>      0000000000000000  sp+160    u         u
>      0000000000000006  sp+160    c-72      c-48
>      0000000000000014  sp+160    u         u
> 
> $ objdump --sframe test_fpra_with_alh.o
> ...
>    Function Index :
> 
>      func idx [0]: pc = 0x0, size = 22 bytes
>      STARTPC         CFA       FP        RA
>      0000000000000000  sp+160    u         u
>      0000000000000006  sp+160    u         c-72
>      0000000000000006  sp+160    c-72      c-48
>      0000000000000014  sp+160    u         c-72
>      0000000000000014  sp+160    u         u
> 
> Note that the outputs of "objdump -Wf" and "objdump -WF" are identical 
> in both cases (with and without option "-alh").
> 
> Debugging of the SFrame processing of the DWARF CFI instructions shows 
> that with option "-a" there are additional DW_CFA_advance_loc:
> 
> DW_CFA_def_cfa: reg=15 offset=160
> DW_CFA_advance_loc: lab1=L0, lab2=L0
> DW_CFA_offset: reg=11 offset=-72
> DW_CFA_advance_loc: lab1=L0, lab2=L0   <-- only with -a
> DW_CFA_offset: reg=14 offset=-48
> DW_CFA_advance_loc: lab1=L0, lab2=L0
> DW_CFA_restore: reg=14
> DW_CFA_advance_loc: lab1=L0, lab2=L0   <-- only with -a
> DW_CFA_restore: reg=11
> 
> Debugging of the CFI directive processing in gas/dw2gencfi.c shows the 
> following:
> 
> - With option "-a" cfi_add_advance_loc() is invoked more often in 
> dot_cfi() due to the condition (symbol_get_frag 
> (frchain_now->frch_cfi_data->last_address) != frag_now) evaluating to true.
> 
> - output_cfi_insn() of case DW_CFA_advance_loc enters the condition 
> (symbol_get_frag (to) == symbol_get_frag (from)) without option "-a" and 
> enters the else condition with option "-a". The else path has an 
> interesting comment that suggests that there is logic to relax an 
> advance by zero at a later stage:
> 
> "... Call frag_grow with the sum of room needed by frag_more and 
> frag_var to preallocate space ensuring that the DW_CFA_advance_loc4 is 
> in the fixed part of the rs_cfa frag, so that the relax machinery can 
> remove the advance_loc should it advance by zero."
> 
> I don't have a clue how to resolve this potential issue in the SFrame 
> generation. I could not figure out how to detect the advance of zero in 
> the SFrame processing of DW_CFA_advance_loc, so that it could be treated 
> special.
> I can open a ticket in the Sourceware Bugzilla, if you agree that this 
> is an issue.
> 

Hi Jens,

Confirming that its reproducible and that this is an issue in the 
existing implementation.  Please go ahead and create a bugzilla.

Thanks

Re: [PATCH v3 02/15] gas: Enhance arch-specific SFrame configuration descriptions

[Indu Bhagat]

On 4/12/24 07:47, Jens Remus wrote:
> Explicitly mention "SFrame" in the descriptions for the architecture-
> specific SFrame configuration macros, variables, and functions.
> 
> 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.
> 
> 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
> 
> Align the descriptions for definitions in the source files to the
> declarations in the header files.
> 
> gas/
> 	* config/tc-aarch64.h: Enhance architecture-specific SFrame
> 	configuration descriptions.
> 	* config/tc-aarch64.c: Likewise.
> 	* config/tc-i386.h: Likewise.
> 	* config/tc-i386.c: Likewise.
> 
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
> 
> Notes (jremus):
>      Changes v2 -> v3:
>      - Add "SFrame" to architecture-specific SFrame macro, variable, and
>        function descriptions as suggested by Indu. Do so for all and not
>        only those previously touched.
>      - Reword further SFrame macro, variable, and function descriptions
>        to align with those previously touched.
>      - Align description of definition in source with declaration in header.
>      - Corrected formatting of ChangeLog in commit message.
>      - Changed commit subject prefix from "sframe" to "gas".
> 
>   gas/config/tc-aarch64.c |  6 +++---
>   gas/config/tc-aarch64.h | 12 ++++++------
>   gas/config/tc-i386.c    |  5 +++++
>   gas/config/tc-i386.h    | 10 +++++-----
>   4 files changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/gas/config/tc-aarch64.c b/gas/config/tc-aarch64.c
> index 6ad4fae8b0ec..077cbd485979 100644
> --- a/gas/config/tc-aarch64.c
> +++ b/gas/config/tc-aarch64.c
> @@ -8870,7 +8870,7 @@ aarch64_support_sframe_p (void)
>     return (aarch64_abi == AARCH64_ABI_LP64);
>   }
>   
> -/* Specify if RA tracking is needed.  */
> +/* Whether SFrame return-address tracking is needed.  */
>   

Nit.  But then given the nature of this commit: I see that this commit 
uses both "return-address" and "return address" keywords.

Also, the code already was already using "stack-pointer" and 
"frame-pointer" (my bad), lets use the same style, and use
  - "frame pointer"
  - "stack pointer"
  - "return address"

OK with that change.

Thanks for your patch.

>   bool
>   aarch64_sframe_ra_tracking_p (void)
> @@ -8878,8 +8878,8 @@ aarch64_sframe_ra_tracking_p (void)
>     return true;
>   }
>   
> -/* Specify the fixed offset to recover RA from CFA.
> -   (useful only when RA tracking is not needed).  */
> +/* The fixed offset from CFA for SFrame to recover the return address.
> +   (useful only when SFrame RA tracking is not needed).  */
>   
>   offsetT
>   aarch64_sframe_cfa_ra_offset (void)
> diff --git a/gas/config/tc-aarch64.h b/gas/config/tc-aarch64.h
> index 1b8badad9fdc..63acbd46a379 100644
> --- a/gas/config/tc-aarch64.h
> +++ b/gas/config/tc-aarch64.h
> @@ -267,24 +267,24 @@ 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 DWARF register number for SFrame CFA tracking.  */
>   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 DWARF register number for SFrame 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 CFA stack trace info.  */
> +/* The return-address DWARF register number for SFrame RA tracking.  */
>   extern unsigned int aarch64_sframe_cfa_ra_reg;
>   #define SFRAME_CFA_RA_REG aarch64_sframe_cfa_ra_reg
>   
> -/* Specify if RA tracking is needed.  */
> +/* Whether SFrame return-address tracking is needed.  */
>   extern bool aarch64_sframe_ra_tracking_p (void);
>   #define sframe_ra_tracking_p aarch64_sframe_ra_tracking_p
>   
> -/* Specify the fixed offset to recover RA from CFA.
> -   (useful only when RA tracking is not needed).  */
> +/* The fixed offset from CFA for SFrame to recover the return address.
> +   (useful only when SFrame RA tracking is not needed).  */
>   extern offsetT aarch64_sframe_cfa_ra_offset (void);
>   #define sframe_cfa_ra_offset aarch64_sframe_cfa_ra_offset
>   
> diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
> index cd20e0c9415e..08a8eb2aeb69 100644
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -11181,6 +11181,7 @@ x86_cleanup (void)
>       subseg_set (seg, subseg);
>   }
>   
> +/* Whether SFrame stack trace info is supported.  */
>   bool
>   x86_support_sframe_p (void)
>   {
> @@ -11188,6 +11189,7 @@ x86_support_sframe_p (void)
>     return (x86_elf_abi == X86_64_ABI);
>   }
>   
> +/* Whether SFrame return-address tracking is needed.  */
>   bool
>   x86_sframe_ra_tracking_p (void)
>   {
> @@ -11197,6 +11199,8 @@ x86_sframe_ra_tracking_p (void)
>     return false;
>   }
>   
> +/* The fixed offset from CFA for SFrame to recover the return address.
> +   (useful only when SFrame RA tracking is not needed).  */
>   offsetT
>   x86_sframe_cfa_ra_offset (void)
>   {
> @@ -11204,6 +11208,7 @@ x86_sframe_cfa_ra_offset (void)
>     return (offsetT) -8;
>   }
>   
> +/* The abi/arch indentifier for SFrame.  */
>   unsigned char
>   x86_sframe_get_abi_arch (void)
>   {
> diff --git a/gas/config/tc-i386.h b/gas/config/tc-i386.h
> index 7aae7a33dc14..e98362111698 100644
> --- a/gas/config/tc-i386.h
> +++ b/gas/config/tc-i386.h
> @@ -441,20 +441,20 @@ 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 DWARF register number for SFrame 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 DWARF register number for SFrame CFA and FP tracking.  */
>   extern unsigned int x86_sframe_cfa_fp_reg;
>   #define SFRAME_CFA_FP_REG x86_sframe_cfa_fp_reg
>   
> -/* Specify if RA tracking is needed.  */
> +/* Whether SFrame return-address tracking is needed.  */
>   extern bool x86_sframe_ra_tracking_p (void);
>   #define sframe_ra_tracking_p x86_sframe_ra_tracking_p
>   
> -/* Specify the fixed offset to recover RA from CFA.
> -   (useful only when RA tracking is not needed).  */
> +/* The fixed offset from CFA for SFrame to recover the return address.
> +   (useful only when SFrame RA tracking is not needed).  */
>   extern offsetT x86_sframe_cfa_ra_offset (void);
>   #define sframe_cfa_ra_offset x86_sframe_cfa_ra_offset
>   

Re: [PATCH v3 03/15] readelf/objdump: Dump SFrame CFA fixed FP and RA offsets

[Indu Bhagat]

On 4/12/24 07:47, 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/
> 	* 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.
> 
> ld/testsuite/
> 	* ld-x86-64/sframe-plt-1.d: Test for SFrame fixed RA offset.
> 	* ld-x86-64/sframe-simple-1.d: Likewise.
> 
> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>

LGTM.

Thanks for your patch.

> ---
> 
> Notes (jremus):
>      Changes v2 -> v3:
>      - Updated gas synthesized CFI test cases for x86 AMD64 to test for
>        architecture-specific fixed RA offset instead of using a pattern.
>      - Updated ld SFrame test cases for x86 AMD64 to test for architecture-
>        specific fixed RA offset.
>      - Corrected formatting of ChangeLog in commit message.
> 
>   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 |  1 +
>   gas/testsuite/gas/scfi/x86_64/scfi-dyn-stack-1.d    |  1 +
>   ld/testsuite/ld-x86-64/sframe-plt-1.d               |  1 +
>   ld/testsuite/ld-x86-64/sframe-simple-1.d            |  1 +
>   libsframe/sframe-dump.c                             | 10 ++++++++++
>   17 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..c45933b72edc 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,7 @@ Contents of the SFrame section .sframe:
>   
>       Version: SFRAME_VERSION_2
>       Flags: NONE
> +    CFA fixed RA offset: \-8
>       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..6cd0484d5793 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,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/ld/testsuite/ld-x86-64/sframe-plt-1.d b/ld/testsuite/ld-x86-64/sframe-plt-1.d
> index 5e734610b970..9d123a73826d 100644
> --- a/ld/testsuite/ld-x86-64/sframe-plt-1.d
> +++ b/ld/testsuite/ld-x86-64/sframe-plt-1.d
> @@ -12,6 +12,7 @@ Contents of the SFrame section .sframe:
>   
>       Version: SFRAME_VERSION_2
>       Flags: SFRAME_F_FDE_SORTED
> +    CFA fixed RA offset: \-8
>   #...
>   
>     Function Index :
> diff --git a/ld/testsuite/ld-x86-64/sframe-simple-1.d b/ld/testsuite/ld-x86-64/sframe-simple-1.d
> index 7f4db31fe1b7..ce5f94386ac2 100644
> --- a/ld/testsuite/ld-x86-64/sframe-simple-1.d
> +++ b/ld/testsuite/ld-x86-64/sframe-simple-1.d
> @@ -12,6 +12,7 @@ Contents of the SFrame section .sframe:
>   
>       Version: SFRAME_VERSION_2
>       Flags: SFRAME_F_FDE_SORTED
> +    CFA fixed RA offset: \-8
>   #...
>   
>     Function Index :
> 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 v3 04/15] readelf/objdump: Display SFrame fixed RA offset as 'f' in dump

[Indu Bhagat]

On 4/12/24 07:47, Jens Remus wrote:
> For the SFrame FRE frame-pointer (FP) offset from CFA a 'u' is displayed
> if it is unavailable.
> 
> For the SFrame FRE return-address (RA) offset from CFA a 'u' was
> displayed if the ABI uses a fixed RA offset from CFA. By chance a
> 'u' was also displayed if the RA offset is unavailable, as the string
> buffer was not initialized after formatting the FP offset. Note that it
> could not occur that the FP offset was erroneously displayed as RA
> offset, as the SFrame format cannot have a FRE with FP offset without
> RA offset.
> 
> For the FRE RA offset display 'f' if the ABI uses a fixed RA offset
> from CFA. Display a 'u' if it is unavailable.
> 
> libsframe/
> 	* sframe-dump.c: Display SFrame fixed RA offset as 'f' in dump.
> 
> gas/testsuite/
> 	* gas/cfi-sframe/cfi-sframe-common-4.d: Test for RA displayed
> 	either as 'u' (if RA tracking) or as 'f' (fixed RA offset if no
> 	RA tracking).
> 	* 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 RA displayed
> 	as 'f' (fixed RA offset), as x86-64 does not use RA tracking.
> 	* gas/scfi/x86_64/scfi-cfi-sections-1.d: Likewise.
> 	* gas/scfi/x86_64/scfi-dyn-stack-1.d: Likewise.
> 
> ld/testsuite/
> 	* ld-x86-64/sframe-plt-1.d: Test for RA displayed as 'f' (fixed
> 	RA offset), as x86-64 does not use RA tracking.
> 	* ld-x86-64/sframe-simple-1.d: Likewise.
> 
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>

LGTM.

Thanks for your patch.

> ---
> 
> Notes (jremus):
>      Changes v2 -> v3:
>      - New patch. If the change in display of fixed RA offset as 'f' is
>        undesired the patch can be reduced to fixing the logic to display the
>        FP offset.
> 
>   .../gas/cfi-sframe/cfi-sframe-common-4.d         |  4 ++--
>   .../gas/cfi-sframe/cfi-sframe-common-5.d         |  4 ++--
>   .../gas/cfi-sframe/cfi-sframe-common-6.d         |  4 ++--
>   .../gas/cfi-sframe/cfi-sframe-common-7.d         |  4 ++--
>   .../gas/cfi-sframe/cfi-sframe-common-8.d         |  2 +-
>   .../gas/cfi-sframe/cfi-sframe-x86_64-1.d         |  8 ++++----
>   .../gas/scfi/x86_64/scfi-cfi-sections-1.d        | 10 +++++-----
>   gas/testsuite/gas/scfi/x86_64/scfi-dyn-stack-1.d | 10 +++++-----
>   ld/testsuite/ld-x86-64/sframe-plt-1.d            |  8 ++++----
>   ld/testsuite/ld-x86-64/sframe-simple-1.d         | 16 ++++++++--------
>   libsframe/sframe-dump.c                          |  8 +++++---
>   11 files changed, 40 insertions(+), 38 deletions(-)
> 
> 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 ca559bd0a029..8632613f532f 100644
> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-4.d
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-4.d
> @@ -17,7 +17,7 @@ Contents of the SFrame section .sframe:
>       func idx \[0\]: pc = 0x0, size = 12 bytes
>       STARTPC + CFA + FP + RA +
>   #...
> -    0+0004 +sp\+16 +u +u +
> -    0+0008 +sp\+32 +u +u +
> +    0+0004 +sp\+16 +u +[uf] +
> +    0+0008 +sp\+32 +u +[uf] +
>   
>   #pass
> 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 ee82053e13db..dd2c32d3d9fc 100644
> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-5.d
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-5.d
> @@ -17,7 +17,7 @@ Contents of the SFrame section .sframe:
>       func idx \[0\]: pc = 0x0, size = 12 bytes
>       STARTPC + CFA + FP + RA +
>   #...
> -    0+0004 +sp\+16 +u +u +
> -    0+0008 +sp\+24 +u +u +
> +    0+0004 +sp\+16 +u +[uf] +
> +    0+0008 +sp\+24 +u +[uf] +
>   
>   #pass
> 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 9d54b98552bf..34390c46a074 100644
> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-6.d
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-6.d
> @@ -17,7 +17,7 @@ Contents of the SFrame section .sframe:
>       func idx \[0\]: pc = 0x0, size = 12 bytes
>       STARTPC + CFA + FP + RA +
>   #...
> -    0+0004 +sp\+8 +u +u +
> -    0+0008 +sp\+8 +u +u +
> +    0+0004 +sp\+8 +u +[uf] +
> +    0+0008 +sp\+8 +u +[uf] +
>   
>   #pass
> 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 2b7fe3aec8f4..61efb9c4ed12 100644
> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-7.d
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-7.d
> @@ -17,7 +17,7 @@ Contents of the SFrame section .sframe:
>       func idx \[0\]: pc = 0x0, size = 12 bytes
>       STARTPC + CFA + FP + RA +
>   #...
> -    0+0004 +sp\+8 +u +u +
> -    0+0008 +sp\+8 +u +u +
> +    0+0004 +sp\+8 +u +[uf] +
> +    0+0008 +sp\+8 +u +[uf] +
>   
>   #pass
> 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 d654e1d0bcd4..d77645636b36 100644
> --- a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-8.d
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-8.d
> @@ -17,6 +17,6 @@ Contents of the SFrame section .sframe:
>       func idx \[0\]: pc = 0x0, size = 8 bytes
>       STARTPC + CFA + FP + RA +
>   #...
> -    0+0004 +sp\+16 +u +u +
> +    0+0004 +sp\+16 +u +[uf] +
>   
>   #pass
> 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 c8b5e6adfea0..88b4cc63dbaa 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
> @@ -16,8 +16,8 @@ Contents of the SFrame section .sframe:
>   
>       func idx \[0\]: pc = 0x0, size = 25 bytes
>       STARTPC +CFA +FP +RA +
> -    0+0000 +sp\+8 +u +u +
> -    0+0001 +sp\+16 +c\-16 +u +
> -    0+0004 +fp\+16 +c\-16 +u +
> -    0+0018 +sp\+8 +c\-16 +u +
> +    0+0000 +sp\+8 +u +f +
> +    0+0001 +sp\+16 +c\-16 +f +
> +    0+0004 +fp\+16 +c\-16 +f +
> +    0+0018 +sp\+8 +c\-16 +f +
>   #pass
> 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 c45933b72edc..7c247e33a6e8 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
> @@ -16,10 +16,10 @@ Contents of the SFrame section .sframe:
>   
>       func idx \[0\]: pc = 0x0, size = 12 bytes
>       STARTPC + CFA + FP + RA +
> -    0+0000 +sp\+8 +u +u +
> -    0+0001 +sp\+16 +c\-16 +u +
> -    0+0004 +fp\+16 +c-16 +u +
> -    0+000a +sp\+16 +c\-16 +u +
> -    0+000b +sp\+8 +u +u +
> +    0+0000 +sp\+8 +u +f +
> +    0+0001 +sp\+16 +c\-16 +f +
> +    0+0004 +fp\+16 +c-16 +f +
> +    0+000a +sp\+16 +c\-16 +f +
> +    0+000b +sp\+8 +u +f +
>   
>   #pass
> 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 6cd0484d5793..c6a9b53f4e09 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
> @@ -16,10 +16,10 @@ Contents of the SFrame section .sframe:
>     Function Index :
>   
>       func idx \[0\]: pc = 0x0, size = 87 bytes
> -    STARTPC + CFA + FP + RA
> -    0+0000 + sp\+8 + u + u
> -    0+0001 + sp\+16 + c-16 + u
> -    0+0004 + fp\+16 + c-16 + u
> -    0+0056 + sp\+8 + u + u
> +    STARTPC + CFA + FP + RA +
> +    0+0000 + sp\+8 + u + f +
> +    0+0001 + sp\+16 + c-16 + f +
> +    0+0004 + fp\+16 + c-16 + f +
> +    0+0056 + sp\+8 + u + f +
>   
>   #pass
> diff --git a/ld/testsuite/ld-x86-64/sframe-plt-1.d b/ld/testsuite/ld-x86-64/sframe-plt-1.d
> index 9d123a73826d..617fb9950acc 100644
> --- a/ld/testsuite/ld-x86-64/sframe-plt-1.d
> +++ b/ld/testsuite/ld-x86-64/sframe-plt-1.d
> @@ -19,12 +19,12 @@ Contents of the SFrame section .sframe:
>   
>       func idx \[0\]: pc = 0x1000, size = 16 bytes
>       STARTPC +CFA +FP +RA +
> -    0+1000 +sp\+16 +u +u +
> -    0+1006 +sp\+24 +u +u +
> +    0+1000 +sp\+16 +u +f +
> +    0+1006 +sp\+24 +u +f +
>   
>       func idx \[1\]: pc = 0x1010, size = 16 bytes
>       STARTPC\[m\] +CFA +FP +RA +
> -    0+0000 +sp\+8 +u +u +
> -    0+000b +sp\+16 +u +u +
> +    0+0000 +sp\+8 +u +f +
> +    0+000b +sp\+16 +u +f +
>   
>   #...
> diff --git a/ld/testsuite/ld-x86-64/sframe-simple-1.d b/ld/testsuite/ld-x86-64/sframe-simple-1.d
> index ce5f94386ac2..18c863938d39 100644
> --- a/ld/testsuite/ld-x86-64/sframe-simple-1.d
> +++ b/ld/testsuite/ld-x86-64/sframe-simple-1.d
> @@ -23,14 +23,14 @@ Contents of the SFrame section .sframe:
>   
>       func idx \[2\]: pc = 0x1020, size = 53 bytes
>       STARTPC +CFA +FP +RA +
> -    0+1020 +sp\+8 +u +u +
> -    0+1021 +sp\+16 +c-16 +u +
> -    0+1024 +fp\+16 +c-16 +u +
> -    0+1054 +sp\+8 +c-16 +u +
> +    0+1020 +sp\+8 +u +f +
> +    0+1021 +sp\+16 +c-16 +f +
> +    0+1024 +fp\+16 +c-16 +f +
> +    0+1054 +sp\+8 +c-16 +f +
>   
>       func idx \[3\]: pc = 0x1055, size = 37 bytes
>       STARTPC +CFA +FP +RA +
> -    0+1055 +sp\+8 +u +u +
> -    0+1056 +sp\+16 +c-16 +u +
> -    0+1059 +fp\+16 +c-16 +u +
> -    0+1079 +sp\+8 +c-16 +u +
> +    0+1055 +sp\+8 +u +f +
> +    0+1056 +sp\+16 +c-16 +f +
> +    0+1059 +fp\+16 +c-16 +f +
> +    0+1079 +sp\+8 +c-16 +f +
> diff --git a/libsframe/sframe-dump.c b/libsframe/sframe-dump.c
> index 493d052ce91f..69633d53a33a 100644
> --- a/libsframe/sframe-dump.c
> +++ b/libsframe/sframe-dump.c
> @@ -181,13 +181,15 @@ dump_sframe_func_with_fres (sframe_decoder_ctx *sfd_ctx,
>         printf ("%-10s", temp);
>   
>         /* Dump RA info.
> -	 If an ABI does not track RA offset, e.g., AMD64, display a 'u',
> +	 If an ABI does not track RA offset, e.g., AMD64, display 'f',
>   	 else display the offset d as 'c+-d'.  */
> -      if (sframe_decoder_get_fixed_ra_offset(sfd_ctx)
> +      if (sframe_decoder_get_fixed_ra_offset (sfd_ctx)
>   	  != SFRAME_CFA_FIXED_RA_INVALID)
> -	strcpy (temp, "u");
> +	strcpy (temp, "f");
>         else if (err[2] == 0)
>   	sprintf (temp, "c%+d", ra_offset);
> +      else
> +	strcpy (temp, "u");
>   
>         /* Mark SFrame FRE's RA information with "[s]" if the RA is mangled
>   	 with signature bits.  */

Re: [PATCH v3 05/15] gas: Print DWARF call frame insn name in SFrame warning message

[Indu Bhagat]

On 4/12/24 07:47, 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.
> 
> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
> Reviewed-by: Indu Bhagat <indu.bhagat@oracle.com>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
> 
> Notes (jremus):
>      Changes v2 -> v3:
>      - Removed stale ChangeLog entries from commit message.
>      - Corrected formatting of ChangeLog in commit message.
> 
>   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);

Nit again: I realized when looking at this patch again that DWARF 
standard uses the terminology of "instruction" for these DW_CFA_*.  I 
looked in the existing codebase and there is precedent to using "CFI 
instruction" in error messages.  How about we change this to "due to CFI 
insn" ?

OK with that change.

Thanks for the patch.

> +    }
>   
>     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 v3 06/15] gas: Skip SFrame FDE if CFI specifies non-FP/SP base register

[Indu Bhagat]

On 4/12/24 07:47, 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.
> 

OK.

Thanks

> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
> Reviewed-by: Indu Bhagat <indu.bhagat@oracle.com>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
> 
> Notes (jremus):
>      Changes v2 -> v3:
>      - Corrected formatting of ChangeLog in commit message.
>      
>      Changes 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 v3 07/15] gas: Warn if SFrame FDE is skipped due to non-default return column

[Indu Bhagat]

On 4/12/24 07:47, 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.
> 

OK.

Thanks

> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
> Reviewed-by: Indu Bhagat <indu.bhagat@oracle.com>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
> 
> Notes (jremus):
>      Changes v2 -> v3:
>      - Enhance comment in code as suggested by Indu.
>      - Corrected formatting of ChangeLog in commit message.
>      
>      Without this patch the assembler would generate incomplete SFrame
>      information without warning 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                              | 10 +++++++---
>   gas/testsuite/gas/cfi-sframe/common-empty-3.d |  1 +
>   2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index 1269b2b77c54..fe8b24906c1b 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -1343,9 +1343,12 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
>   
>     xlate_ctx->dw_fde = dw_fde;
>   
> -  /* If the return column is not RIP, SFrame format cannot represent it.  */
> +  /* 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 v3 08/15] gas: Refactor SFrame CFI opcode DW_CFA_register processing

[Indu Bhagat]

On 4/12/24 07:47, Jens Remus wrote:
> Refactor SFrame processing of CFI opcode DW_CFA_register into a separate
> function. This harmonizes the CFI opcode processing.
> 
> While at it reword the comment on CFI opcodes that are not processed.
> 
> This is a purely mechanical change.
> 
> gas/
> 	* gen-sframe.c (sframe_do_cfi_insn, sframe_xlate_do_register):
> 	Refactor SFrame CFI opcode DW_CFA_register processing.
> 

OK.

Thanks

> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
> 
> Notes (jremus):
>      Changes v2 -> v3:
>      - New patch.
> 
>   gas/gen-sframe.c | 37 ++++++++++++++++++++++++++-----------
>   1 file changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index fe8b24906c1b..f47f51c7a354 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -1108,6 +1108,28 @@ sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>     return SFRAME_XLATE_OK;
>   }
>   
> +/* Translate DW_CFA_register into SFrame context.
> +   Return SFRAME_XLATE_OK if success.  */
> +
> +static int
> +sframe_xlate_do_register (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
> +			  struct cfi_insn_data *cfi_insn)
> +{
> +  /* Previous value of register1 is register2.  However, if the specified
> +     register1 is not interesting (SP, FP, or RA reg), the current DW_CFA_register
> +     instruction can be safely skipped without sacrificing the asynchronicity of
> +     stack trace information.  */
> +  if (cfi_insn->u.rr.reg1 == SFRAME_CFA_SP_REG
> +#ifdef SFRAME_FRE_RA_TRACKING
> +      || (cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG)
> +#endif
> +      || cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
> +    return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
> +
> +  /* Safe to skip.  */
> +  return SFRAME_XLATE_OK;
> +}
> +
>   /* Translate DW_CFA_remember_state into SFrame context.
>      Return SFRAME_XLATE_OK if success.  */
>   
> @@ -1296,19 +1318,12 @@ sframe_do_cfi_insn (struct sframe_xlate_ctx *xlate_ctx,
>       case DW_CFA_GNU_window_save:
>         err = sframe_xlate_do_gnu_window_save (xlate_ctx, cfi_insn);
>         break;
> -    /* Other CFI opcodes are not processed at this time.
> -       These do not impact the coverage of the basic stack tracing
> -       information as conveyed in the SFrame format.
> -	- DW_CFA_register,
> -	- etc.  */
>       case DW_CFA_register:
> -      if (cfi_insn->u.rr.reg1 == SFRAME_CFA_SP_REG
> -#ifdef SFRAME_FRE_RA_TRACKING
> -	  || cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG
> -#endif
> -	  || cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
> -	err = SFRAME_XLATE_ERR_NOTREPRESENTED;
> +      err = sframe_xlate_do_register (xlate_ctx, cfi_insn);
>         break;
> +    /* Following CFI opcodes are not processed at this time.
> +       These do not impact the coverage of the basic stack tracing
> +       information as conveyed in the SFrame format.  */
>       case DW_CFA_undefined:
>       case DW_CFA_same_value:
>         break;

Re: [PATCH v3 10/15] gas: Skip SFrame FDE if FP without RA on stack

[Jens Remus]

Am 18.04.2024 um 01:56 schrieb Indu Bhagat:
> On 4/16/24 06:14, Jens Remus wrote:
>> Hello Indu,
>>
>> Am 12.04.2024 um 16:47 schrieb Jens Remus:
>>> The SFrame format cannot represent the frame pointer (FP) being saved
>>> on the stack without the return address (RA) also being saved on the
>>> stack, if RA tracking is used.
>>
>> [...]
>>
>>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>>> index a3b6f75cfe85..87be3eb05ad2 100644
>>> --- a/gas/gen-sframe.c
>>> +++ b/gas/gen-sframe.c
>>> @@ -1439,6 +1439,25 @@ sframe_do_fde (struct sframe_xlate_ctx 
>>> *xlate_ctx,
>>>       = get_dw_fde_end_addrS (xlate_ctx->dw_fde);
>>>       }
>>> +#ifdef SFRAME_FRE_RA_TRACKING
>>> +  if (sframe_ra_tracking_p ())
>>> +    {
>>> +      struct sframe_row_entry *fre;
>>> +
>>> +      /* Iterate over the scratchpad FREs and validate them.  */
>>> +      for (fre = xlate_ctx->first_fre; fre; fre = fre->next)
>>> +    {
>>> +      /* SFrame format cannot represent FP on stack without RA on 
>>> stack.  */
>>> +      if (fre->ra_loc != SFRAME_FRE_ELEM_LOC_STACK
>>> +          && fre->bp_loc == SFRAME_FRE_ELEM_LOC_STACK)
>>> +        {
>>> +          as_warn (_("skipping SFrame FDE due to FP without RA on 
>>> stack"));
>>> +          return SFRAME_XLATE_ERR_NOTREPRESENTED;
>>> +        }
>>> +    }
>>> +    }
>>> +#endif /* SFRAME_FRE_RA_TRACKING  */
>>> +
>>>     return SFRAME_XLATE_OK;
>>>   }
>>
>> I noticed that above new warning is erroneously emitted when 
>> assembling the following CFI directive sequence with option "-alh" (to 
>> output a listing of the assembly; probably any "-a[...]") on a SFrame 
>> enabled target, that uses FP and RA tracking.
>>
>> .cfi_offset <fp-regno>, <fp-offset>
>> .cfi_offset <ra-regno>, <ra-offset>
>>
>> The reason is that with listings enabled there is an additional DWARF 
>> DW_CFA_advance_loc CFI instruction (with a zero advance) between both 
>> DW_CFA_offset instructions, that the DWARF .eh_frame generator is able 
>> to process correctly, but causes the .sframe generator to choke.
>>
>> Additionally with this patch reverted "bad" SFrame information is 
>> generated (see example below), where there are multiple SFrame FREs 
>> for the same PC start address.
>> Note that the FP-tracking information erroneously being displayed in 
>> the RA-tracking column, is why I introduced this new warning message. 
>> I will send two alternative patches how to potentially resolve that soon.
>>
>> $ cat test_fpra_min.s
>>          .cfi_sections .sframe, .eh_frame
>>          .cfi_startproc
>>          stmg    %r11,%r15,88(%r15)
>>          .cfi_rel_offset 11, 88
>>          .cfi_rel_offset 14, 112
>>          la      %r11,0
>>          la      %r14,0
>> .Lreturn:
>>          lmg     %r11,%r15,88(%r15)
>>          .cfi_restore 14
>>          .cfi_restore 11
>>          br      %r14
>>          .cfi_endproc
>>
>> $ ojbdump --sframe test_fpra_without-alh.o
>> ...
>>    Function Index :
>>
>>      func idx [0]: pc = 0x0, size = 22 bytes
>>      STARTPC         CFA       FP        RA
>>      0000000000000000  sp+160    u         u
>>      0000000000000006  sp+160    c-72      c-48
>>      0000000000000014  sp+160    u         u
>>
>> $ objdump --sframe test_fpra_with_alh.o
>> ...
>>    Function Index :
>>
>>      func idx [0]: pc = 0x0, size = 22 bytes
>>      STARTPC         CFA       FP        RA
>>      0000000000000000  sp+160    u         u
>>      0000000000000006  sp+160    u         c-72
>>      0000000000000006  sp+160    c-72      c-48
>>      0000000000000014  sp+160    u         c-72
>>      0000000000000014  sp+160    u         u
>>
>> Note that the outputs of "objdump -Wf" and "objdump -WF" are identical 
>> in both cases (with and without option "-alh").
>>
>> Debugging of the SFrame processing of the DWARF CFI instructions shows 
>> that with option "-a" there are additional DW_CFA_advance_loc:
>>
>> DW_CFA_def_cfa: reg=15 offset=160
>> DW_CFA_advance_loc: lab1=L0, lab2=L0
>> DW_CFA_offset: reg=11 offset=-72
>> DW_CFA_advance_loc: lab1=L0, lab2=L0   <-- only with -a
>> DW_CFA_offset: reg=14 offset=-48
>> DW_CFA_advance_loc: lab1=L0, lab2=L0
>> DW_CFA_restore: reg=14
>> DW_CFA_advance_loc: lab1=L0, lab2=L0   <-- only with -a
>> DW_CFA_restore: reg=11
>>
>> Debugging of the CFI directive processing in gas/dw2gencfi.c shows the 
>> following:
>>
>> - With option "-a" cfi_add_advance_loc() is invoked more often in 
>> dot_cfi() due to the condition (symbol_get_frag 
>> (frchain_now->frch_cfi_data->last_address) != frag_now) evaluating to 
>> true.
>>
>> - output_cfi_insn() of case DW_CFA_advance_loc enters the condition 
>> (symbol_get_frag (to) == symbol_get_frag (from)) without option "-a" 
>> and enters the else condition with option "-a". The else path has an 
>> interesting comment that suggests that there is logic to relax an 
>> advance by zero at a later stage:
>>
>> "... Call frag_grow with the sum of room needed by frag_more and 
>> frag_var to preallocate space ensuring that the DW_CFA_advance_loc4 is 
>> in the fixed part of the rs_cfa frag, so that the relax machinery can 
>> remove the advance_loc should it advance by zero."
>>
>> I don't have a clue how to resolve this potential issue in the SFrame 
>> generation. I could not figure out how to detect the advance of zero 
>> in the SFrame processing of DW_CFA_advance_loc, so that it could be 
>> treated special.
>> I can open a ticket in the Sourceware Bugzilla, if you agree that this 
>> is an issue.
>>
> 
> Hi Jens,
> 
> Confirming that its reproducible and that this is an issue in the 
> existing implementation.  Please go ahead and create a bugzilla.
> 
> Thanks

Hello Indu,

thanks for confirming! Opened:

https://sourceware.org/bugzilla/show_bug.cgi?id=31654

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 v3 09/15] gas: User readable warnings if SFrame FDE is not generated

[Indu Bhagat]

On 4/12/24 07:47, 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:
> 

My intention below is to keep warning messages terse but useful.

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

.cfi_def_cfa _is_ used to specify the CFA base register. So I suggest we 
re-word this to following (and hence, removing the redundancy):

skipping SFrame FDE; non-SP/FP register <regno> in .cfi_def_cfa

or if (;) is unappetizing:

skipping SFrame FDE due to non-SP/FP register <regno> in .cfi_def_cfa

(replacing ; with "due to", if you prefer, applies to all comments below)

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

skipping SFrame FDE; non-SP/FP register <regno> in .cfi_def_cfa_register

> - skipping SFrame FDE due to .cfi_def_cfa_offset without CFA base
>    register in effect

skipping SFrame FDE; .cfi_def_cfa_offset specified when no CFA base 
register in effect

> - skipping SFrame FDE due to .cfi_def_cfa_offset with non-SP/FP register
>    <regno> as CFA base register in effect

I find this very confusing.  Can we not add this warning?  Such a case 
should not happen as we have handled the respective cases in 
sframe_xlate_do_def_cfa and sframe_xlate_do_def_cfa_register ?

> - skipping SFrame FDE due to .cfi_val_offset specifying {FP|RA} register

skipping SFrame FDE; FP/RA register <regno> in .cfi_val_offset

> - skipping SFrame FDE due to .cfi_remember_state without SFrame FRE
>    state

skipping SFrame FDE; .cfi_remember_state without prior SFrame FRE state

> - skipping SFrame FDE due to .cfi_register specifying {SP|FP|RA}
>    register

skipping SFrame FDE; FP/RA register <regno> in .cfi_register

> - skipping SFrame FDE due to non-default return-address register <regno>

skipping SFrame FDE; non-default RA register <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.
> 
> gas/testsuite/
> 	* gas/cfi-sframe/common-empty-1.d: Update generic SFrame test
> 	case to updated warning message texts.
> 	* gas/cfi-sframe/common-empty-2.d: Likewise.
> 	* gas/cfi-sframe/common-empty-3.d: Likewise.
> 
> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
> 
> Notes (jremus):
>      Changes v2 -> v3:
>      - Updated SFrame warning message texts as suggested by Indu, except for
>        the .cfi_def_cfa[_register] ones. 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? Anyhow, Indu, if you still prefer your suggestions I
>        would go with yours.
>        The ".cfi_remember_state without SFrame FRE state" warning needs to
>        remain, as there would otherwise not be any warning at all. The reason
>        is that sframe_do_cfi_insn now expects any of its called CFI
>        processing functions to emit a warning, if they return with an error,
>        such as SFRAME_XLATE_ERR_NOTREPRESENTED. Silently skipping SFrame FDE
>        does not seem an acceptable approach to me.
>      - Do not test sframe_ra_tracking_p when determining the SFrame register
>        name in sframe_register_name. The RA register name does not depend on
>        whether RA tracking is used or not.
>      - Corrected formatting of ChangeLog in commit message.
> 
>   gas/gen-sframe.c                              | 95 ++++++++++++++-----
>   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, 75 insertions(+), 28 deletions(-)
> 
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index f47f51c7a354..a3b6f75cfe85 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,23 @@ 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 (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 +1007,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 defining "
> +		 "non-SP/FP register %u as CFA base register"),
> +	       cfi_insn->u.r);
> +      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 +1037,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 defining "
> +		 "non-SP/FP register %u as CFA base register"),
> +	       cfi_insn->u.r);
> +      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 +1073,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 with "
> +		   "non-SP/FP register %u as CFA base register in effect"),
> +		 cur_fre->cfa_base_reg);
> +      return SFRAME_XLATE_ERR_NOTREPRESENTED;
> +    }
>   
>     return SFRAME_XLATE_OK;
>   }
> @@ -1096,13 +1132,16 @@ sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>        register is not interesting (FP or RA reg), the current DW_CFA_val_offset
>        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.  */
> +  if (cfi_insn->u.r == SFRAME_CFA_FP_REG
>   #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.  */
> +      || (sframe_ra_tracking_p () && cfi_insn->u.r == SFRAME_CFA_RA_REG)
>   #endif
> +      )
> +    {
> +      as_warn (_("skipping SFrame FDE due to .cfi_val_offset specifying %s register"),
> +	       sframe_register_name (cfi_insn->u.r));
> +      return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
> +    }
>   
>     /* Safe to skip.  */
>     return SFRAME_XLATE_OK;
> @@ -1124,7 +1163,11 @@ sframe_xlate_do_register (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>         || (cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG)
>   #endif
>         || cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
> -    return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
> +    {
> +      as_warn (_("skipping SFrame FDE due to .cfi_register specifying %s register"),
> +	       sframe_register_name (cfi_insn->u.rr.reg1));
> +      return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
> +    }
>   
>     /* Safe to skip.  */
>     return SFRAME_XLATE_OK;
> @@ -1142,7 +1185,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"));

"without SFrame FRE state" --> "without prior SFrame FRE state" ?

> +      return SFRAME_XLATE_ERR_INVAL;
> +    }
>   
>     if (!xlate_ctx->remember_fre)
>       xlate_ctx->remember_fre = sframe_row_entry_new ();
> @@ -1330,21 +1376,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;

<>/GCC/gcc-upstream/gcc/contrib/check_GNU_style.py patch gives:

=== ERROR type #1: blocks of 8 spaces should be replaced with tabs (1 
error(s)) ===
gas/gen-sframe.c:1386:0:        err = SFRAME_XLATE_ERR_NOTREPRESENTED;

I dont think it is a must-be-used-script for binutils project, but for 
my commits I have used this script to keep code base aligned to same 
formatting style.

PS: Not all errors reported by the script need to be fixed.

> +      }
>       }
>   
> +  /* Any error will cause no SFrame FDE later.  The user has already been
> +     warned.  */
>     return err;
>   }
>   
> @@ -1361,7 +1405,8 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
>     /* SFrame format cannot represent a non-default DWARF return column reg.  */
>     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 return-address register %u"),
> +	       xlate_ctx->dw_fde->return_column);

return-address -> "return address" or even just RA.

>         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..5cc37d5ff440 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 return-address register 0
>   #objdump: --sframe=.sframe
>   #name: SFrame supports only default return column
>   #...

Re: [PATCH v3 10/15] gas: Skip SFrame FDE if FP without RA on stack

[Indu Bhagat]

On 4/12/24 07:47, Jens Remus wrote:
> The SFrame format cannot represent the frame pointer (FP) being saved
> on the stack without the return address (RA) also being saved on the
> stack, if RA tracking is used.
> 
> A SFrame FDE is followed by 1-3 offsets with the following information:
> 
> Without RA tracking:
> 1. Offset from base pointer (SP or FP) to locate the CFA
> 2. Optional: Offset to CFA to restore the frame pointer (FP)
> 
> With RA tracking:
> 1. Offset from base pointer (SP or FP) to locate the CFA
> 2. Optional: Offset to CFA to restore the return address (RA)
> 3. Optional: Offset to CFA to restore the frame pointer (FP)
> 
> When RA tracking is used and a FDE is followed by two offsets the
> SFrame format does not provide any information to distinguish whether
> the second offset is the RA or FP offset. SFrame assumes the offset to
> be the RA offset, which may be wrong.
> 
> Therefore skip generation of SFrame FDE information and print the
> following warning, if RA tracking is used and the FP is saved on the
> stack without the RA being saved as well:
> 
>    skipping SFrame FDE due to FP without RA on stack
> 

OK.

One comment below, otherwise LGTM.

Thanks

> gas/
> 	* gen-sframe.c (sframe_do_fde): Skip SFrame FDE if FP without RA
> 	on stack, as the SFrame format cannot represent this case.
> 
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
> 
> Notes (jremus):
>      Changes v2 -> v3:
>      - New patch.
>      
>      Without this patch the assembler would generate incorrect SFrame
>      information without warning for the s390-specific SFrame error test
>      case 5, that gets introduced by patch "s390: Initial support to
>      generate .sframe from CFI directives in assembler". The FRE would
>      be followed by two offsets for the CFA and FP. SFrame would
>      erroneously interpret them as CFA and RA offsets, as it cannot
>      represent FP without RA on stack.
> 
>   gas/gen-sframe.c | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
> 
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index a3b6f75cfe85..87be3eb05ad2 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -1439,6 +1439,25 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
>   	= get_dw_fde_end_addrS (xlate_ctx->dw_fde);
>       }
>   
> +#ifdef SFRAME_FRE_RA_TRACKING
> +  if (sframe_ra_tracking_p ())
> +    {
> +      struct sframe_row_entry *fre;
> +
> +      /* Iterate over the scratchpad FREs and validate them.  */
> +      for (fre = xlate_ctx->first_fre; fre; fre = fre->next)
> +	{
> +	  /* SFrame format cannot represent FP on stack without RA on stack.  */
> +	  if (fre->ra_loc != SFRAME_FRE_ELEM_LOC_STACK
> +	      && fre->bp_loc == SFRAME_FRE_ELEM_LOC_STACK)
> +	    {
> +	      as_warn (_("skipping SFrame FDE due to FP without RA on stack"));
> +	      return SFRAME_XLATE_ERR_NOTREPRESENTED;
> +	    }
> +	}
> +    }
> +#endif /* SFRAME_FRE_RA_TRACKING  */
> +

There is a comment "/* No errors encountered.  */" at line 1452 (few 
lines above the proposed diff after applying).  I suggest we also remove 
/ move that comment too.

>     return SFRAME_XLATE_OK;
>   }
>   

Re: [PATCH v3 11/15] gas: Skip SFrame FDE if .cfi_window_save

[Indu Bhagat]

On 4/12/24 07:47, Jens Remus wrote:
> CFI opcode DW_CFA_AARCH64_negate_ra_state is multiplexed with
> DW_CFA_GNU_window_save. Process DW_CFA_AARCH64_negate_ra_state on
> AArch64. Skip generation of SFrame FDE otherwise.
> 
> gas/
> 	* gen-sframe.c: Skip SFrame FDE if .cfi_window_save.
> 

LGTM.

Thanks

> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
> 
> Notes (jremus):
>      Changes v2 -> v3:
>      - New patch. The intention is to skip all "unknown" CFI opcodes,
>        which SFrame does not explicitly handle. DW_CFA_GNU_window_save
>        seems to be handled only for the AArch64-specific multiplexed
>        DW_CFA_AARCH64_negate_ra_state. The logic could be changed to be
>        dependent on TC_AARCH64 at build-time instead of
>        sframe_get_abi_arch() at run-time.
> 
>   gas/gen-sframe.c | 25 ++++++++++++++++++++++---
>   1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index 87be3eb05ad2..98166de8cc01 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -1257,12 +1257,12 @@ sframe_xlate_do_restore (struct sframe_xlate_ctx *xlate_ctx,
>     return SFRAME_XLATE_OK;
>   }
>   
> -/* Translate DW_CFA_GNU_window_save into SFrame context.
> +/* Translate DW_CFA_AARCH64_negate_ra_state into SFrame context.
>      Return SFRAME_XLATE_OK if success.  */
>   
>   static int
> -sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
> -				 struct cfi_insn_data *cfi_insn ATTRIBUTE_UNUSED)
> +sframe_xlate_do_aarch64_negate_ra_state (struct sframe_xlate_ctx *xlate_ctx,
> +					 struct cfi_insn_data *cfi_insn ATTRIBUTE_UNUSED)
>   {
>     struct sframe_row_entry *cur_fre = xlate_ctx->cur_fre;
>   
> @@ -1274,6 +1274,25 @@ sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
>     return SFRAME_XLATE_OK;
>   }
>   
> +/* Translate DW_CFA_GNU_window_save into SFrame context.
> +   DW_CFA_AARCH64_negate_ra_state is multiplexed with DW_CFA_GNU_window_save.
> +   Return SFRAME_XLATE_OK if success.  */
> +
> +static int
> +sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
> +				 struct cfi_insn_data *cfi_insn)
> +{
> +  unsigned char abi_arch = sframe_get_abi_arch ();
> +
> +  /* Translate DW_CFA_AARCH64_negate_ra_state into SFrame context.  */
> +  if (abi_arch == SFRAME_ABI_AARCH64_ENDIAN_BIG
> +      || abi_arch == SFRAME_ABI_AARCH64_ENDIAN_LITTLE)
> +    return sframe_xlate_do_aarch64_negate_ra_state (xlate_ctx, cfi_insn);
> +
> +  as_warn (_("skipping SFrame FDE due to .cfi_window_save"));
> +  return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
> +}
> +
>   /* Returns the DWARF call frame instruction name or fake CFI name for the
>      specified CFI opcode, or NULL if the value is not recognized.  */
>   

Re: [PATCH v3 12/15] gas: Don't skip SFrame FDE if .cfi_register specifies RA w/o tracking

[Indu Bhagat]

On 4/12/24 07:47, Jens Remus wrote:
> Do not skip SFrame FDE if .cfi_register specifies RA register without
> RA tracking being actually used. Without RA tracking the register
> contents can always be restored from the stack using the fixed
> RA offset from CFA.
> 
> gas/
> 	* gen-sframe.c (sframe_xlate_do_register): Do not skip SFrame
> 	FDE if .cfi_register specifies RA register without RA tracking
> 	being used.
> 

OK.

Thanks

> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
> 
> Notes (jremus):
>      Changes v2 -> v3:
>      - New patch.
> 
>   gas/gen-sframe.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index 98166de8cc01..61c846f214ee 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -1160,7 +1160,7 @@ sframe_xlate_do_register (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>        stack trace information.  */
>     if (cfi_insn->u.rr.reg1 == SFRAME_CFA_SP_REG
>   #ifdef SFRAME_FRE_RA_TRACKING
> -      || (cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG)
> +      || (sframe_ra_tracking_p () && cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG)
>   #endif
>         || cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
>       {

Re: [PATCH v3 13/15] gas: Don't skip SFrame FDE if .cfi_register specifies SP register

[Indu Bhagat]

On 4/12/24 07:47, Jens Remus wrote:
> The stack-pointer (SP) register contents on entry can be reconstructed
> from the CFA base register tracking information from the current SFrame
> FRE and initial FRE from the FDE:
> 
> 1. Compute CFA from the current CFA base register (SP or FP) and CFA
>     offset from the SFrame CFA base register tracking information from
>     the SFrame FRE for the current instruction address:
> 
>       CFA = <current_base_reg> + <current_cfa_offset>
> 
> 2. Compute SP from the current CFA and the CFA offset from the SFrame
>     CFA tracking information from the initial SFrame FRE of the FDE:
> 
>       SP = CFA - <initial_cfa_offset>
> 
> While at it add a comment to the processing of .cfi_val_offset that the
> SP can be reconstructed from the CFA base register tracking information.
> 

Sorry, but I am not sure I follow.  Yes, SP can be reconstructed using 
the means you outline, but so can FP.  And also RA when applicable.

In other words, I dont follow why treat SP differently from FP / RA when 
it comes to .cfi_register (or even .cfi_val_offset) for reporting the 
warning.

> gas/
> 	* gen-sframe.c (sframe_xlate_do_register): Do not skip SFrame
> 	FDE if .cfi_register specifies SP register.
> 	(sframe_xlate_do_val_offset): Add comment that this is likewise.
> 
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
> 
> Notes (jremus):
>      Changes v2 -> v3:
>      - New patch.
> 
>   gas/gen-sframe.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index 61c846f214ee..12b523a8d59a 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -1136,6 +1136,7 @@ sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>   #ifdef SFRAME_FRE_RA_TRACKING
>         || (sframe_ra_tracking_p () && cfi_insn->u.r == SFRAME_CFA_RA_REG)
>   #endif
> +      /* Ignore SP reg, as it can be recovered from the CFA tracking info.  */
>         )
>       {
>         as_warn (_("skipping SFrame FDE due to .cfi_val_offset specifying %s register"),
> @@ -1155,14 +1156,15 @@ sframe_xlate_do_register (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>   			  struct cfi_insn_data *cfi_insn)
>   {
>     /* Previous value of register1 is register2.  However, if the specified
> -     register1 is not interesting (SP, FP, or RA reg), the current DW_CFA_register
> +     register1 is not interesting (FP or RA reg), the current DW_CFA_register
>        instruction can be safely skipped without sacrificing the asynchronicity of
>        stack trace information.  */
> -  if (cfi_insn->u.rr.reg1 == SFRAME_CFA_SP_REG
> +  if (cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG
>   #ifdef SFRAME_FRE_RA_TRACKING
>         || (sframe_ra_tracking_p () && cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG)
>   #endif
> -      || cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
> +      /* Ignore SP reg, as it can be recovered from the CFA tracking info.  */
> +      )
>       {
>         as_warn (_("skipping SFrame FDE due to .cfi_register specifying %s register"),
>   	       sframe_register_name (cfi_insn->u.rr.reg1));

Re: [PATCH v3 14/15] gas: Test predicate whether SFrame RA tracking is used

[Indu Bhagat]

On 4/12/24 07:47, Jens Remus wrote:
> The existence of the macro SFRAME_FRE_RA_TRACKING only ensures the
> existence of the macro SFRAME_CFA_RA_REG and the predicate function
> sframe_ra_tracking_p. It does not indicate whether SFrame RA tracking
> is actually used.
> 
> Test the return value of the SFrame RA tracking predicate function
> sframe_ra_tracking_p to determine whether RA tracking is used.
> 
> This aligns the logic in functions get_fre_num_offsets and
> output_sframe_row_entry to the one used in all other places.
> 

LGTM.

Thanks

> gas/
> 	* gen-sframe.c (get_fre_num_offsets, output_sframe_row_entry):
> 	Test predicate to determine whether SFrame RA tracking is used.
> 
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
> 
> Notes (jremus):
>      Changes v2 -> v3:
>      - New patch.
> 
>   gas/gen-sframe.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index 12b523a8d59a..ca6565b0e45e 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -346,7 +346,8 @@ get_fre_num_offsets (struct sframe_row_entry *sframe_fre)
>     if (sframe_fre->bp_loc == SFRAME_FRE_ELEM_LOC_STACK)
>       fre_num_offsets++;
>   #ifdef SFRAME_FRE_RA_TRACKING
> -  if (sframe_fre->ra_loc == SFRAME_FRE_ELEM_LOC_STACK)
> +  if (sframe_ra_tracking_p ()
> +      && sframe_fre->ra_loc == SFRAME_FRE_ELEM_LOC_STACK)
>       fre_num_offsets++;
>   #endif
>     return fre_num_offsets;
> @@ -536,7 +537,8 @@ output_sframe_row_entry (symbolS *fde_start_addr,
>     fre_write_offsets++;
>   
>   #ifdef SFRAME_FRE_RA_TRACKING
> -  if (sframe_fre->ra_loc == SFRAME_FRE_ELEM_LOC_STACK)
> +  if (sframe_ra_tracking_p ()
> +      && sframe_fre->ra_loc == SFRAME_FRE_ELEM_LOC_STACK)
>       {
>         fre_offset_func_map[idx].out_func (sframe_fre->ra_offset);
>         fre_write_offsets++;

Re: [PATCH v3 15/15] gas: Validate SFrame RA tracking and fixed RA offset

[Indu Bhagat]

On 4/12/24 07:47, Jens Remus wrote:
> If an architecture uses SFrame return-address (RA) tracking it must
> specify the fixed RA offset as invalid. Otherwise, if an architecture
> does not use RA tracking, it must specify a valid fixed RA offset.
> 
> gas/
> 	* gen-sframe.c: Validate SFrame RA tracking and fixed
> 	RA offset.
> 
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
> 
> Notes (jremus):
>      Changes v2 -> v3:
>      - New patch.
>      
>      This could be made dependent on ENABLE_CHECKING (configure option
>      --enable-checking).
> 
>   gas/gen-sframe.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index ca6565b0e45e..7e815f9603ef 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -1532,6 +1532,18 @@ output_sframe (segT sframe_seg)
>     /* Setup the version specific access functions.  */
>     sframe_set_version (SFRAME_VERSION_2);
>   
> +#ifdef SFRAME_FRE_RA_TRACKING
> +  if (sframe_ra_tracking_p ())
> +    /* With RA tracking the fixed RA offset must be invalid.  */
> +    gas_assert (sframe_cfa_ra_offset () == SFRAME_CFA_FIXED_RA_INVALID);
> +  else
> +    /* Without RA tracking the fixed RA offset may not be invalid.  */
> +    gas_assert (sframe_cfa_ra_offset () != SFRAME_CFA_FIXED_RA_INVALID);
> +#else
> +  /* Without RA tracking the fixed RA offset may not be invalid.  */
> +  gas_assert (sframe_cfa_ra_offset () != SFRAME_CFA_FIXED_RA_INVALID);
> +#endif
> +

I am not sure if the detailed checks are worth it here (simply because 
of code patterns that follow).

We use the sframe_cfa_ra_offset () function later and only in 
output_sframe_internal () (shown below).  How about we simply put an 
assert there (and get rid of the proposed thunk above):

#ifdef sframe_ra_tracking_p
   if (!sframe_ra_tracking_p ())
     {
       fixed_ra_offset = sframe_cfa_ra_offset ();
       gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
     }
#endif
   out_one (fixed_ra_offset);

fixed_ra_offset is initialized to SFRAME_CFA_FIXED_RA_INVALID in 
output_sframe_internal ().

>     /* Process all fdes and create SFrame stack trace information.  */
>     create_sframe_all ();
>   

Re: [PATCH v3 13/15] gas: Don't skip SFrame FDE if .cfi_register specifies SP register

[Jens Remus]

Am 18.04.2024 um 22:37 schrieb Indu Bhagat:
> On 4/12/24 07:47, Jens Remus wrote:
>> The stack-pointer (SP) register contents on entry can be reconstructed
>> from the CFA base register tracking information from the current SFrame
>> FRE and initial FRE from the FDE:
>>
>> 1. Compute CFA from the current CFA base register (SP or FP) and CFA
>>     offset from the SFrame CFA base register tracking information from
>>     the SFrame FRE for the current instruction address:
>>
>>       CFA = <current_base_reg> + <current_cfa_offset>
>>
>> 2. Compute SP from the current CFA and the CFA offset from the SFrame
>>     CFA tracking information from the initial SFrame FRE of the FDE:
>>
>>       SP = CFA - <initial_cfa_offset>
>>
>> While at it add a comment to the processing of .cfi_val_offset that the
>> SP can be reconstructed from the CFA base register tracking information.
>>
> 
> Sorry, but I am not sure I follow.  Yes, SP can be reconstructed using 
> the means you outline, but so can FP.  And also RA when applicable.
> 
> In other words, I dont follow why treat SP differently from FP / RA when 
> it comes to .cfi_register (or even .cfi_val_offset) for reporting the 
> warning.
> 

I did not find any explanation how unwinding using SFrame is exactly 
expected to be performed. Following is how I assume it to work in 
general and on s390x specifically. Details of how to restore the stack 
pointer register value at the call site are probably architecture specific.

1. Use the program counter (PC) or subsequently return address (RA)
    as address to do a binary search through the SFrame FDEs to determine
    the FDE for the current function. Binary search can be used, as the
    FDEs are of fixed size.

2. Note/print the function start address taken from the FDE or lookup
    its symbol for the stack trace.

3. Do a linear search through the SFrame FREs associated with the FDE
    to determine the FRE for the address within the function. Linear
    search must be used, since the FREs are of different size and
    different numbers of offsets may follow each.

3. Unwind the stack-pointer (SP), frame-pointer (FP), and return-
    address (RA) register values at function entry:

    - SP: Since SFrame does not track the SP register, the mean
      to restore its contents at entry to the function must be to
      use the SFrame CFA trackig information from the current FRE
      and initial FRE of the function/FDE to restore the SP value:

         CFA = <CFA-base-reg> + <current-CFA-offset>
         SP = CFA - <initial-CFA-offset>

         NOTE: Above does not restore the SP value from the stack,
               as SFrame does not know whether and where it would
               be saved. Instead it computes the SP value.

      The use of both the current and initial CFA offsets is valid,
      as the DWARF standard mandates the CFA to be fixed for a
      function:

         "The algorithm to compute CFA changes as you progress
         through the prologue and epilogue code. (__By definition,
         the CFA value does not change.__)"

    - FP: Without SFrame FP tracking use the fixed FP offset as follows.
      With FP tacking (currently the default), if available use the
      FP offset to recover the FP value as follows, otherwise the
      FP value is still in the FP register.

     CFA = <CFA-base-reg> + <current-CFA-offset>
     FP = [CFA + FP-offset]

     NOTE: Above restores the FP value from the stack.

    - RA: Without SFrame RA tracking use the fixed RA offset as follows.
      With RA tracking (depending on architecture), if available use the
      RA offset to recover the RA value as follows, otherwise the
      RA value is still in the RA register.

         CFA = <CFA-base-reg> + <current-CFA-offset>
         RA = [CFA + RA-offset]

         NOTE: Same as for FP.

4. Unwind the SP register value at the call site. This step be merged
    with the previous one.

    On s390x the value of the stack pointer on entry to a function is
    identical to the value of the stack pointer at the call site. Thus
    nothing has to be done.
    Note that on s390x the CFA is not defined to be the value of the
    stack pointer at the call site, as it is for other architectures.
    Instead it is usually defined as the upper bound (exclusive) of
    the current stack frame.
    This is valid as the DWARF standard does not mandate the CFA to
    be defined as the value of the SP at the call site.

       "__Typically__, the CFA is defined to be the value of the stack
       pointer at the call site in the previous frame (which may be
       different from its value on entry to the current frame)."

    On x86-64 the SP at the call site can be determined using a fixed
    offset, or SP = CFA can be used, if the CFA is defined as the SP
    value at the call site on x86-64.

5. Repeat at step 1 using the RA value as address to perform the next
    FDE lookup.


With that to your question why I assume it would be acceptable to treat 
SP different from FP and RA when it comes to .cfi_offset, 
.cfi_val_offset, and .cfi_register:

On s390x, and I assume this to apply to all other architectures, the SP
value on function entry and from that also at the call site can always 
be restored using above steps. It is irrelevant whether the SP value on 
function entry is saved on the stack (.cfi_offset), in another register 
(.cfi_register), or is defined as offset from CFA (.cfi_val_offset).

Actually the SP is inherently defined as an offset from the CFA that can 
be deducted from the CFA tracking information (see above).
An exception would be if a compiler would generate code that defines a 
non-SP register as CFA base register on function entry. I have not 
observed that on s390x.

Regarding the aforementioned CFI directives I observed the following on 
s390x:

The SP register value is saved on the stack in the function prologue and 
restored in the epilogue, when allocating stack space of fixed size for 
local variables. The compiler generates respective .cfi_offset 
<SP-regno>, ... and .cfi_restore <SP-regno> CFI directives. Note that 
the compiler could alternatively use arithmetic instructions to restore 
the initial SP value. Due to the availability of Load/Store Multiple 
instructions those are generally preferred.
An unwinder using .eh_frame information would most likely use the 
.cfi_offset information for the SP register.
An unwinder using .sframe information would instead need to perform 
above mentioned computations, as SFrame does not track the SP register.

As for the .cfi_val_offset <SP-regno>, ... instances in glibc it is similar.
An unwinder using .eh_frame information would use the .cfi_val_offset 
information.
An unwinder using .sframe information would instead again need to 
perform above mentioned computations.

Does that make sense? If I have missed something, then SFrame generation 
would probably need to skip FDE when the SP register is specified in 
.cfi_offset (new), .cfi_register, and .cfi_val_offset, no?


Btw., besides the RFC Linux Kernel perf patch series "[PATCH RFC 00/10] 
perf: user space sframe unwinding" [1] is there any reference 
implementation of a SFrame unwinder I could use as base for testing 
SFrame unwinding on s390x? Maybe a GDB Python script to do unwinding 
using SFrame (not sure how to access the .sframe section)?

[1] [PATCH RFC 00/10] perf: user space sframe unwinding,
https://lore.kernel.org/all/cover.1699487758.git.jpoimboe@kernel.org/

>> gas/
>>     * gen-sframe.c (sframe_xlate_do_register): Do not skip SFrame
>>     FDE if .cfi_register specifies SP register.
>>     (sframe_xlate_do_val_offset): Add comment that this is likewise.
>>
>> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
>> ---
>>
>> Notes (jremus):
>>      Changes v2 -> v3:
>>      - New patch.
>>
>>   gas/gen-sframe.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>> index 61c846f214ee..12b523a8d59a 100644
>> --- a/gas/gen-sframe.c
>> +++ b/gas/gen-sframe.c
>> @@ -1136,6 +1136,7 @@ sframe_xlate_do_val_offset (struct 
>> sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>>   #ifdef SFRAME_FRE_RA_TRACKING
>>         || (sframe_ra_tracking_p () && cfi_insn->u.r == 
>> SFRAME_CFA_RA_REG)
>>   #endif
>> +      /* Ignore SP reg, as it can be recovered from the CFA tracking 
>> info.  */
>>         )
>>       {
>>         as_warn (_("skipping SFrame FDE due to .cfi_val_offset 
>> specifying %s register"),
>> @@ -1155,14 +1156,15 @@ sframe_xlate_do_register (struct 
>> sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>>                 struct cfi_insn_data *cfi_insn)
>>   {
>>     /* Previous value of register1 is register2.  However, if the 
>> specified
>> -     register1 is not interesting (SP, FP, or RA reg), the current 
>> DW_CFA_register
>> +     register1 is not interesting (FP or RA reg), the current 
>> DW_CFA_register
>>        instruction can be safely skipped without sacrificing the 
>> asynchronicity of
>>        stack trace information.  */
>> -  if (cfi_insn->u.rr.reg1 == SFRAME_CFA_SP_REG
>> +  if (cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG
>>   #ifdef SFRAME_FRE_RA_TRACKING
>>         || (sframe_ra_tracking_p () && cfi_insn->u.rr.reg1 == 
>> SFRAME_CFA_RA_REG)
>>   #endif
>> -      || cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
>> +      /* Ignore SP reg, as it can be recovered from the CFA tracking 
>> info.  */
>> +      )
>>       {
>>         as_warn (_("skipping SFrame FDE due to .cfi_register 
>> specifying %s register"),
>>              sframe_register_name (cfi_insn->u.rr.reg1));
> 

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 v3 13/15] gas: Don't skip SFrame FDE if .cfi_register specifies SP register

[Indu Bhagat]

On 4/19/24 06:13, Jens Remus wrote:
> Am 18.04.2024 um 22:37 schrieb Indu Bhagat:
>> On 4/12/24 07:47, Jens Remus wrote:
>>> The stack-pointer (SP) register contents on entry can be reconstructed
>>> from the CFA base register tracking information from the current SFrame
>>> FRE and initial FRE from the FDE:
>>>
>>> 1. Compute CFA from the current CFA base register (SP or FP) and CFA
>>>     offset from the SFrame CFA base register tracking information from
>>>     the SFrame FRE for the current instruction address:
>>>
>>>       CFA = <current_base_reg> + <current_cfa_offset>
>>>
>>> 2. Compute SP from the current CFA and the CFA offset from the SFrame
>>>     CFA tracking information from the initial SFrame FRE of the FDE:
>>>
>>>       SP = CFA - <initial_cfa_offset>
>>>
>>> While at it add a comment to the processing of .cfi_val_offset that the
>>> SP can be reconstructed from the CFA base register tracking information.
>>>
>>
>> Sorry, but I am not sure I follow.  Yes, SP can be reconstructed using 
>> the means you outline, but so can FP.  And also RA when applicable.
>>
>> In other words, I dont follow why treat SP differently from FP / RA 
>> when it comes to .cfi_register (or even .cfi_val_offset) for reporting 
>> the warning.
>>
> 
> I did not find any explanation how unwinding using SFrame is exactly 
> expected to be performed. Following is how I assume it to work in 
> general and on s390x specifically. Details of how to restore the stack 
> pointer register value at the call site are probably architecture specific.
> 
> 1. Use the program counter (PC) or subsequently return address (RA)
>     as address to do a binary search through the SFrame FDEs to determine
>     the FDE for the current function. Binary search can be used, as the
>     FDEs are of fixed size.
> 
> 2. Note/print the function start address taken from the FDE or lookup
>     its symbol for the stack trace.
> 
> 3. Do a linear search through the SFrame FREs associated with the FDE
>     to determine the FRE for the address within the function. Linear
>     search must be used, since the FREs are of different size and
>     different numbers of offsets may follow each.
> 
> 3. Unwind the stack-pointer (SP), frame-pointer (FP), and return-
>     address (RA) register values at function entry:
> 
>     - SP: Since SFrame does not track the SP register, the mean
>       to restore its contents at entry to the function must be to
>       use the SFrame CFA trackig information from the current FRE
>       and initial FRE of the function/FDE to restore the SP value:
> 
>          CFA = <CFA-base-reg> + <current-CFA-offset>
>          SP = CFA - <initial-CFA-offset>
> 
>          NOTE: Above does not restore the SP value from the stack,
>                as SFrame does not know whether and where it would
>                be saved. Instead it computes the SP value.
> 
>       The use of both the current and initial CFA offsets is valid,
>       as the DWARF standard mandates the CFA to be fixed for a
>       function:
> 
>          "The algorithm to compute CFA changes as you progress
>          through the prologue and epilogue code. (__By definition,
>          the CFA value does not change.__)"
> 
>     - FP: Without SFrame FP tracking use the fixed FP offset as follows.
>       With FP tacking (currently the default), if available use the
>       FP offset to recover the FP value as follows, otherwise the
>       FP value is still in the FP register.
> 
>      CFA = <CFA-base-reg> + <current-CFA-offset>
>      FP = [CFA + FP-offset]
> 
>      NOTE: Above restores the FP value from the stack.
> 
>     - RA: Without SFrame RA tracking use the fixed RA offset as follows.
>       With RA tracking (depending on architecture), if available use the
>       RA offset to recover the RA value as follows, otherwise the
>       RA value is still in the RA register.
> 
>          CFA = <CFA-base-reg> + <current-CFA-offset>
>          RA = [CFA + RA-offset]
> 
>          NOTE: Same as for FP.
> 
> 4. Unwind the SP register value at the call site. This step be merged
>     with the previous one.
> 
>     On s390x the value of the stack pointer on entry to a function is
>     identical to the value of the stack pointer at the call site. Thus
>     nothing has to be done.
>     Note that on s390x the CFA is not defined to be the value of the
>     stack pointer at the call site, as it is for other architectures.
>     Instead it is usually defined as the upper bound (exclusive) of
>     the current stack frame.
>     This is valid as the DWARF standard does not mandate the CFA to
>     be defined as the value of the SP at the call site.
> 
>        "__Typically__, the CFA is defined to be the value of the stack
>        pointer at the call site in the previous frame (which may be
>        different from its value on entry to the current frame)."
> 
>     On x86-64 the SP at the call site can be determined using a fixed
>     offset, or SP = CFA can be used, if the CFA is defined as the SP
>     value at the call site on x86-64.
> 
> 5. Repeat at step 1 using the RA value as address to perform the next
>     FDE lookup.
> 
> 
> With that to your question why I assume it would be acceptable to treat 
> SP different from FP and RA when it comes to .cfi_offset, 
> .cfi_val_offset, and .cfi_register:
> 
> On s390x, and I assume this to apply to all other architectures, the SP
> value on function entry and from that also at the call site can always 
> be restored using above steps. It is irrelevant whether the SP value on 
> function entry is saved on the stack (.cfi_offset), in another register 
> (.cfi_register), or is defined as offset from CFA (.cfi_val_offset).
> 
> Actually the SP is inherently defined as an offset from the CFA that can 
> be deducted from the CFA tracking information (see above).
> An exception would be if a compiler would generate code that defines a 
> non-SP register as CFA base register on function entry. I have not 
> observed that on s390x.
> 
> Regarding the aforementioned CFI directives I observed the following on 
> s390x:
> 
> The SP register value is saved on the stack in the function prologue and 
> restored in the epilogue, when allocating stack space of fixed size for 
> local variables. The compiler generates respective .cfi_offset 
> <SP-regno>, ... and .cfi_restore <SP-regno> CFI directives. Note that 
> the compiler could alternatively use arithmetic instructions to restore 
> the initial SP value. Due to the availability of Load/Store Multiple 
> instructions those are generally preferred.
> An unwinder using .eh_frame information would most likely use the 
> .cfi_offset information for the SP register.
> An unwinder using .sframe information would instead need to perform 
> above mentioned computations, as SFrame does not track the SP register.
> 
> As for the .cfi_val_offset <SP-regno>, ... instances in glibc it is 
> similar.
> An unwinder using .eh_frame information would use the .cfi_val_offset 
> information.
> An unwinder using .sframe information would instead again need to 
> perform above mentioned computations.
> 
> Does that make sense? If I have missed something, then SFrame generation 
> would probably need to skip FDE when the SP register is specified in 
> .cfi_offset (new), .cfi_register, and .cfi_val_offset, no?
> 

I think it is dawning on me: If '.cfi_register SP, reg2' is seen, this 
does not alter the tracking to recover prev-SP (doing so will need an 
explicit .cfi_def_cfa which we track).

Whereas if .cfi_register FP, reg3 is seen, the FDE needs to be skipped 
because we cannot represent this information.  A later instruction may 
be sabotaging FP, and later recovering back FP from reg3 via a 
.cfi_register reg3, FP.

I think some SCFI related work was polluting my thoughts.

> 
> Btw., besides the RFC Linux Kernel perf patch series "[PATCH RFC 00/10] 
> perf: user space sframe unwinding" [1] is there any reference 
> implementation of a SFrame unwinder I could use as base for testing 
> SFrame unwinding on s390x? Maybe a GDB Python script to do unwinding 
> using SFrame (not sure how to access the .sframe section)?
> 
> [1] [PATCH RFC 00/10] perf: user space sframe unwinding,
> https://lore.kernel.org/all/cover.1699487758.git.jpoimboe@kernel.org/
> 

We did have an "SFrame unwinder library" patchset for binutils, which we 
used for testing SFrame for x86 and aarch64 support.  It wasn't 
upstreamed because a) it had open issues, b) in hindsight, it doesn't 
belong to binutils-gdp repo in the current shape.

I will push it in a personal branch upstream and share some notes on its 
status and open issues. At the least, it should be helpful in testing 
your s390 enhancements to the SFrame format.

>>> gas/
>>>     * gen-sframe.c (sframe_xlate_do_register): Do not skip SFrame
>>>     FDE if .cfi_register specifies SP register.
>>>     (sframe_xlate_do_val_offset): Add comment that this is likewise.
>>>
>>> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
>>> ---
>>>
>>> Notes (jremus):
>>>      Changes v2 -> v3:
>>>      - New patch.
>>>
>>>   gas/gen-sframe.c | 8 +++++---
>>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>>> index 61c846f214ee..12b523a8d59a 100644
>>> --- a/gas/gen-sframe.c
>>> +++ b/gas/gen-sframe.c
>>> @@ -1136,6 +1136,7 @@ sframe_xlate_do_val_offset (struct 
>>> sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>>>   #ifdef SFRAME_FRE_RA_TRACKING
>>>         || (sframe_ra_tracking_p () && cfi_insn->u.r == 
>>> SFRAME_CFA_RA_REG)
>>>   #endif
>>> +      /* Ignore SP reg, as it can be recovered from the CFA tracking 
>>> info.  */
>>>         )
>>>       {
>>>         as_warn (_("skipping SFrame FDE due to .cfi_val_offset 
>>> specifying %s register"),
>>> @@ -1155,14 +1156,15 @@ sframe_xlate_do_register (struct 
>>> sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>>>                 struct cfi_insn_data *cfi_insn)
>>>   {
>>>     /* Previous value of register1 is register2.  However, if the 
>>> specified
>>> -     register1 is not interesting (SP, FP, or RA reg), the current 
>>> DW_CFA_register
>>> +     register1 is not interesting (FP or RA reg), the current 
>>> DW_CFA_register
>>>        instruction can be safely skipped without sacrificing the 
>>> asynchronicity of
>>>        stack trace information.  */
>>> -  if (cfi_insn->u.rr.reg1 == SFRAME_CFA_SP_REG
>>> +  if (cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG
>>>   #ifdef SFRAME_FRE_RA_TRACKING
>>>         || (sframe_ra_tracking_p () && cfi_insn->u.rr.reg1 == 
>>> SFRAME_CFA_RA_REG)
>>>   #endif
>>> -      || cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
>>> +      /* Ignore SP reg, as it can be recovered from the CFA tracking 
>>> info.  */
>>> +      )
>>>       {
>>>         as_warn (_("skipping SFrame FDE due to .cfi_register 
>>> specifying %s register"),
>>>              sframe_register_name (cfi_insn->u.rr.reg1));
>>
> 
> Thanks and regards,
> Jens

Re: [PATCH v3 13/15] gas: Don't skip SFrame FDE if .cfi_register specifies SP register

[Indu Bhagat]

On 4/23/24 01:15, Indu Bhagat wrote:
>> Btw., besides the RFC Linux Kernel perf patch series "[PATCH RFC 
>> 00/10] perf: user space sframe unwinding" [1] is there any reference 
>> implementation of a SFrame unwinder I could use as base for testing 
>> SFrame unwinding on s390x? Maybe a GDB Python script to do unwinding 
>> using SFrame (not sure how to access the .sframe section)?
>>
>> [1] [PATCH RFC 00/10] perf: user space sframe unwinding,
>> https://lore.kernel.org/all/cover.1699487758.git.jpoimboe@kernel.org/
>>
> 
> We did have an "SFrame unwinder library" patchset for binutils, which we 
> used for testing SFrame for x86 and aarch64 support.  It wasn't 
> upstreamed because a) it had open issues, b) in hindsight, it doesn't 
> belong to binutils-gdp repo in the current shape.
> 
> I will push it in a personal branch upstream and share some notes on its 
> status and open issues. At the least, it should be helpful in testing 
> your s390 enhancements to the SFrame format.

I have pushed a user branch users/ibhagat/sframe-stacktracer-testsuite 
upstream.

The testsuite is in libsframe/testsuite/libsframe.stacktrace.  The 
"SFrame stack tracer library" is now a non-installed, 
testsuite-purpose-only, test library (see 
libsframe/testsuite/libsframe.stacktrace/libsfrast). Hope that helps.

The stacktracer testsuite itself needs bit more grooming before it can 
be presented for inclusion into libsframe's testsuite upstream.  If you 
have patches for this branch over time, please feel free to push them to 
the branch :)

Thanks
Indu

Re: [PATCH v3 09/15] gas: User readable warnings if SFrame FDE is not generated

[Jens Remus]

Am 18.04.2024 um 22:33 schrieb Indu Bhagat:
> On 4/12/24 07:47, 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:
>>
> 
> My intention below is to keep warning messages terse but useful.
> 
>> - skipping SFrame FDE due to .cfi_def_cfa defining non-SP/FP register
>>    <regno> as CFA base register
> 
> .cfi_def_cfa _is_ used to specify the CFA base register. So I suggest we 
> re-word this to following (and hence, removing the redundancy):
> 
> skipping SFrame FDE; non-SP/FP register <regno> in .cfi_def_cfa

Done.

> 
> or if (;) is unappetizing:
> 
> skipping SFrame FDE due to non-SP/FP register <regno> in .cfi_def_cfa
> 
> (replacing ; with "due to", if you prefer, applies to all comments below)

I like the terse format. Especially as it saves some characters for the longer reason descriptions.

> 
>> - skipping SFrame FDE due to .cfi_def_cfa_register defining non-SP/FP
>>    register <regno> as CFA base register
> 
> skipping SFrame FDE; non-SP/FP register <regno> in .cfi_def_cfa_register

Done.

> 
>> - skipping SFrame FDE due to .cfi_def_cfa_offset without CFA base
>>    register in effect
> 
> skipping SFrame FDE; .cfi_def_cfa_offset specified when no CFA base 
> register in effect

"when no" or "without" (see "without prior SFrame FRE state" below)?

> 
>> - skipping SFrame FDE due to .cfi_def_cfa_offset with non-SP/FP register
>>    <regno> as CFA base register in effect
> 
> I find this very confusing.  Can we not add this warning?  Such a case 
> should not happen as we have handled the respective cases in 
> sframe_xlate_do_def_cfa and sframe_xlate_do_def_cfa_register ?

Done. I finally got the point. I have added the following comment as explanation:

/* No CFA base register in effect.  Non-SP/FP CFA base register should
    not occur, as sframe_xlate_do_def_cfa[_register] would detect this.  */

> 
>> - skipping SFrame FDE due to .cfi_val_offset specifying {FP|RA} register
> 
> skipping SFrame FDE; FP/RA register <regno> in .cfi_val_offset

Done.

> 
>> - skipping SFrame FDE due to .cfi_remember_state without SFrame FRE
>>    state
> 
> skipping SFrame FDE; .cfi_remember_state without prior SFrame FRE state

Done.

> 
>> - skipping SFrame FDE due to .cfi_register specifying {SP|FP|RA}
>>    register
> 
> skipping SFrame FDE; FP/RA register <regno> in .cfi_register

Done.

> 
>> - skipping SFrame FDE due to non-default return-address register <regno>
> 
> skipping SFrame FDE; non-default RA register <regno>

Done. Using the abbreviation "RA" instead of "return address" aligns well with the other warnings.


For consistency I have changed the following SFrame warnings as well:

skipping SFrame FDE; CFI insn <name> (<hexval>)

skipping SFrame FDE; FP without RA on stack

skipping SFrame FDE; .cfi_window_save

> 
>>
>> 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.
>>
>> gas/testsuite/
>>     * gas/cfi-sframe/common-empty-1.d: Update generic SFrame test
>>     case to updated warning message texts.
>>     * gas/cfi-sframe/common-empty-2.d: Likewise.
>>     * gas/cfi-sframe/common-empty-3.d: Likewise.
>>
>> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
>> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
>> ---
>>
>> Notes (jremus):
>>      Changes v2 -> v3:
>>      - Updated SFrame warning message texts as suggested by Indu, 
>> except for
>>        the .cfi_def_cfa[_register] ones. 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? Anyhow, Indu, if you still prefer your suggestions I
>>        would go with yours.
>>        The ".cfi_remember_state without SFrame FRE state" warning 
>> needs to
>>        remain, as there would otherwise not be any warning at all. The 
>> reason
>>        is that sframe_do_cfi_insn now expects any of its called CFI
>>        processing functions to emit a warning, if they return with an 
>> error,
>>        such as SFRAME_XLATE_ERR_NOTREPRESENTED. Silently skipping 
>> SFrame FDE
>>        does not seem an acceptable approach to me.
>>      - Do not test sframe_ra_tracking_p when determining the SFrame 
>> register
>>        name in sframe_register_name. The RA register name does not 
>> depend on
>>        whether RA tracking is used or not.
>>      - Corrected formatting of ChangeLog in commit message.
>>
>>   gas/gen-sframe.c                              | 95 ++++++++++++++-----
>>   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, 75 insertions(+), 28 deletions(-)
>>
>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>> index f47f51c7a354..a3b6f75cfe85 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,23 @@ 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 (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 +1007,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 defining "
>> +         "non-SP/FP register %u as CFA base register"),
>> +           cfi_insn->u.r);
>> +      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 +1037,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 
>> defining "
>> +         "non-SP/FP register %u as CFA base register"),
>> +           cfi_insn->u.r);
>> +      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 +1073,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 with "
>> +           "non-SP/FP register %u as CFA base register in effect"),
>> +         cur_fre->cfa_base_reg);
>> +      return SFRAME_XLATE_ERR_NOTREPRESENTED;
>> +    }
>>     return SFRAME_XLATE_OK;
>>   }
>> @@ -1096,13 +1132,16 @@ sframe_xlate_do_val_offset (struct 
>> sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>>        register is not interesting (FP or RA reg), the current 
>> DW_CFA_val_offset
>>        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.  */
>> +  if (cfi_insn->u.r == SFRAME_CFA_FP_REG
>>   #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.  */
>> +      || (sframe_ra_tracking_p () && cfi_insn->u.r == SFRAME_CFA_RA_REG)
>>   #endif
>> +      )
>> +    {
>> +      as_warn (_("skipping SFrame FDE due to .cfi_val_offset 
>> specifying %s register"),
>> +           sframe_register_name (cfi_insn->u.r));
>> +      return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
>> +    }
>>     /* Safe to skip.  */
>>     return SFRAME_XLATE_OK;
>> @@ -1124,7 +1163,11 @@ sframe_xlate_do_register (struct 
>> sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>>         || (cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG)
>>   #endif
>>         || cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
>> -    return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
>> +    {
>> +      as_warn (_("skipping SFrame FDE due to .cfi_register specifying 
>> %s register"),
>> +           sframe_register_name (cfi_insn->u.rr.reg1));
>> +      return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
>> +    }
>>     /* Safe to skip.  */
>>     return SFRAME_XLATE_OK;
>> @@ -1142,7 +1185,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"));
> 
> "without SFrame FRE state" --> "without prior SFrame FRE state" ?
> 
>> +      return SFRAME_XLATE_ERR_INVAL;
>> +    }
>>     if (!xlate_ctx->remember_fre)
>>       xlate_ctx->remember_fre = sframe_row_entry_new ();
>> @@ -1330,21 +1376,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;
> 
> <>/GCC/gcc-upstream/gcc/contrib/check_GNU_style.py patch gives:
> 
> === ERROR type #1: blocks of 8 spaces should be replaced with tabs (1 
> error(s)) ===
> gas/gen-sframe.c:1386:0:        err = SFRAME_XLATE_ERR_NOTREPRESENTED;

Corrected.

> 
> I dont think it is a must-be-used-script for binutils project, but for 
> my commits I have used this script to keep code base aligned to same 
> formatting style.

Thanks for sharing! That is really helful!

> 
> PS: Not all errors reported by the script need to be fixed.
> 
>> +      }
>>       }
>> +  /* Any error will cause no SFrame FDE later.  The user has already 
>> been
>> +     warned.  */
>>     return err;
>>   }
>> @@ -1361,7 +1405,8 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
>>     /* SFrame format cannot represent a non-default DWARF return 
>> column reg.  */
>>     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 
>> return-address register %u"),
>> +           xlate_ctx->dw_fde->return_column);
> 
> return-address -> "return address" or even just RA.
> 
>>         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..5cc37d5ff440 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 return-address 
>> register 0
>>   #objdump: --sframe=.sframe
>>   #name: SFrame supports only default return column
>>   #...
> 

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 v3 02/15] gas: Enhance arch-specific SFrame configuration descriptions

[Jens Remus]

Am 18.04.2024 um 09:39 schrieb Indu Bhagat:
> On 4/12/24 07:47, Jens Remus wrote:
>> Explicitly mention "SFrame" in the descriptions for the architecture-
>> specific SFrame configuration macros, variables, and functions.
>>
>> 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.
>>
>> 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
>>
>> Align the descriptions for definitions in the source files to the
>> declarations in the header files.
>>
>> gas/
>>     * config/tc-aarch64.h: Enhance architecture-specific SFrame
>>     configuration descriptions.
>>     * config/tc-aarch64.c: Likewise.
>>     * config/tc-i386.h: Likewise.
>>     * config/tc-i386.c: Likewise.
>>
>> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
>> ---
>>
>> Notes (jremus):
>>      Changes v2 -> v3:
>>      - Add "SFrame" to architecture-specific SFrame macro, variable, and
>>        function descriptions as suggested by Indu. Do so for all and not
>>        only those previously touched.
>>      - Reword further SFrame macro, variable, and function descriptions
>>        to align with those previously touched.
>>      - Align description of definition in source with declaration in 
>> header.
>>      - Corrected formatting of ChangeLog in commit message.
>>      - Changed commit subject prefix from "sframe" to "gas".
>>
>>   gas/config/tc-aarch64.c |  6 +++---
>>   gas/config/tc-aarch64.h | 12 ++++++------
>>   gas/config/tc-i386.c    |  5 +++++
>>   gas/config/tc-i386.h    | 10 +++++-----
>>   4 files changed, 19 insertions(+), 14 deletions(-)
>>
>> diff --git a/gas/config/tc-aarch64.c b/gas/config/tc-aarch64.c
>> index 6ad4fae8b0ec..077cbd485979 100644
>> --- a/gas/config/tc-aarch64.c
>> +++ b/gas/config/tc-aarch64.c
>> @@ -8870,7 +8870,7 @@ aarch64_support_sframe_p (void)
>>     return (aarch64_abi == AARCH64_ABI_LP64);
>>   }
>> -/* Specify if RA tracking is needed.  */
>> +/* Whether SFrame return-address tracking is needed.  */
> 
> Nit.  But then given the nature of this commit: I see that this commit 
> uses both "return-address" and "return address" keywords.

I wrongly assumed them to be compound adjectives [1], which usually 
require a dash, when used in conjunction with e.g. "register". That is 
why I differentiated between e.g. "stack pointer" (noun) and 
"stack-pointer register" (falsely assumed "stack-pointer" to be a 
compound adjective). I just learned that they are compound nouns [2] and 
thus are always written without dash.

[1] https://www.grammarly.com/blog/compound-adjectives/
[2] https://www.grammarly.com/blog/compound-nouns/

> 
> Also, the code already was already using "stack-pointer" and 
> "frame-pointer" (my bad), lets use the same style, and use
>   - "frame pointer"
>   - "stack pointer"
>   - "return address"
> 
> OK with that change.
> 
> Thanks for your patch.

Done.

> 
>>   bool
>>   aarch64_sframe_ra_tracking_p (void)
>> @@ -8878,8 +8878,8 @@ aarch64_sframe_ra_tracking_p (void)
>>     return true;
>>   }
>> -/* Specify the fixed offset to recover RA from CFA.
>> -   (useful only when RA tracking is not needed).  */
>> +/* The fixed offset from CFA for SFrame to recover the return address.
>> +   (useful only when SFrame RA tracking is not needed).  */
>>   offsetT
>>   aarch64_sframe_cfa_ra_offset (void)
>> diff --git a/gas/config/tc-aarch64.h b/gas/config/tc-aarch64.h
>> index 1b8badad9fdc..63acbd46a379 100644
>> --- a/gas/config/tc-aarch64.h
>> +++ b/gas/config/tc-aarch64.h
>> @@ -267,24 +267,24 @@ 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 DWARF register number for SFrame CFA tracking.  */
>>   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 DWARF register number for SFrame 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 CFA stack trace info.  */
>> +/* The return-address DWARF register number for SFrame RA tracking.  */
>>   extern unsigned int aarch64_sframe_cfa_ra_reg;
>>   #define SFRAME_CFA_RA_REG aarch64_sframe_cfa_ra_reg
>> -/* Specify if RA tracking is needed.  */
>> +/* Whether SFrame return-address tracking is needed.  */
>>   extern bool aarch64_sframe_ra_tracking_p (void);
>>   #define sframe_ra_tracking_p aarch64_sframe_ra_tracking_p
>> -/* Specify the fixed offset to recover RA from CFA.
>> -   (useful only when RA tracking is not needed).  */
>> +/* The fixed offset from CFA for SFrame to recover the return address.
>> +   (useful only when SFrame RA tracking is not needed).  */
>>   extern offsetT aarch64_sframe_cfa_ra_offset (void);
>>   #define sframe_cfa_ra_offset aarch64_sframe_cfa_ra_offset
>> diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
>> index cd20e0c9415e..08a8eb2aeb69 100644
>> --- a/gas/config/tc-i386.c
>> +++ b/gas/config/tc-i386.c
>> @@ -11181,6 +11181,7 @@ x86_cleanup (void)
>>       subseg_set (seg, subseg);
>>   }
>> +/* Whether SFrame stack trace info is supported.  */
>>   bool
>>   x86_support_sframe_p (void)
>>   {
>> @@ -11188,6 +11189,7 @@ x86_support_sframe_p (void)
>>     return (x86_elf_abi == X86_64_ABI);
>>   }
>> +/* Whether SFrame return-address tracking is needed.  */
>>   bool
>>   x86_sframe_ra_tracking_p (void)
>>   {
>> @@ -11197,6 +11199,8 @@ x86_sframe_ra_tracking_p (void)
>>     return false;
>>   }
>> +/* The fixed offset from CFA for SFrame to recover the return address.
>> +   (useful only when SFrame RA tracking is not needed).  */
>>   offsetT
>>   x86_sframe_cfa_ra_offset (void)
>>   {
>> @@ -11204,6 +11208,7 @@ x86_sframe_cfa_ra_offset (void)
>>     return (offsetT) -8;
>>   }
>> +/* The abi/arch indentifier for SFrame.  */
>>   unsigned char
>>   x86_sframe_get_abi_arch (void)
>>   {
>> diff --git a/gas/config/tc-i386.h b/gas/config/tc-i386.h
>> index 7aae7a33dc14..e98362111698 100644
>> --- a/gas/config/tc-i386.h
>> +++ b/gas/config/tc-i386.h
>> @@ -441,20 +441,20 @@ 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 DWARF register number for SFrame 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 DWARF register number for SFrame CFA and FP 
>> tracking.  */
>>   extern unsigned int x86_sframe_cfa_fp_reg;
>>   #define SFRAME_CFA_FP_REG x86_sframe_cfa_fp_reg
>> -/* Specify if RA tracking is needed.  */
>> +/* Whether SFrame return-address tracking is needed.  */
>>   extern bool x86_sframe_ra_tracking_p (void);
>>   #define sframe_ra_tracking_p x86_sframe_ra_tracking_p
>> -/* Specify the fixed offset to recover RA from CFA.
>> -   (useful only when RA tracking is not needed).  */
>> +/* The fixed offset from CFA for SFrame to recover the return address.
>> +   (useful only when SFrame RA tracking is not needed).  */
>>   extern offsetT x86_sframe_cfa_ra_offset (void);
>>   #define sframe_cfa_ra_offset x86_sframe_cfa_ra_offset
> 

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 v3 15/15] gas: Validate SFrame RA tracking and fixed RA offset

[Jens Remus]

Am 18.04.2024 um 22:38 schrieb Indu Bhagat:
> On 4/12/24 07:47, Jens Remus wrote:
>> If an architecture uses SFrame return-address (RA) tracking it must
>> specify the fixed RA offset as invalid. Otherwise, if an architecture
>> does not use RA tracking, it must specify a valid fixed RA offset.
>>
>> gas/
>>     * gen-sframe.c: Validate SFrame RA tracking and fixed
>>     RA offset.
>>
>> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
>> ---
>>
>> Notes (jremus):
>>      Changes v2 -> v3:
>>      - New patch.
>>      This could be made dependent on ENABLE_CHECKING (configure option
>>      --enable-checking).
>>
>>   gas/gen-sframe.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>> index ca6565b0e45e..7e815f9603ef 100644
>> --- a/gas/gen-sframe.c
>> +++ b/gas/gen-sframe.c
>> @@ -1532,6 +1532,18 @@ output_sframe (segT sframe_seg)
>>     /* Setup the version specific access functions.  */
>>     sframe_set_version (SFRAME_VERSION_2);
>> +#ifdef SFRAME_FRE_RA_TRACKING
>> +  if (sframe_ra_tracking_p ())
>> +    /* With RA tracking the fixed RA offset must be invalid.  */
>> +    gas_assert (sframe_cfa_ra_offset () == SFRAME_CFA_FIXED_RA_INVALID);
>> +  else
>> +    /* Without RA tracking the fixed RA offset may not be invalid.  */
>> +    gas_assert (sframe_cfa_ra_offset () != SFRAME_CFA_FIXED_RA_INVALID);
>> +#else
>> +  /* Without RA tracking the fixed RA offset may not be invalid.  */
>> +  gas_assert (sframe_cfa_ra_offset () != SFRAME_CFA_FIXED_RA_INVALID);
>> +#endif
>> +
> 
> I am not sure if the detailed checks are worth it here (simply because 
> of code patterns that follow).

I agree, provided the checks are performed elsewhere as you suggest.

My intention was to have checks that assist with getting SFrame support 
for another architecture implemented correctly, without having to chase 
subtle issues.

> 
> We use the sframe_cfa_ra_offset () function later and only in 
> output_sframe_internal () (shown below).  How about we simply put an 
> assert there (and get rid of the proposed thunk above):
> 
> #ifdef sframe_ra_tracking_p
>    if (!sframe_ra_tracking_p ())

See below.

>      {
>        fixed_ra_offset = sframe_cfa_ra_offset ();
>        gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);

That is clever and accounts for one potential implementation issue!

>      }
> #endif
>    out_one (fixed_ra_offset);
> 
> fixed_ra_offset is initialized to SFRAME_CFA_FIXED_RA_INVALID in 
> output_sframe_internal ().

Above logic requires sframe_ra_tracking_p to be defined by an 
architecture that is not using RA tracking. Not defining 
sframe_ra_tracking_p would result in fixed_ra_offset being unexpectedly 
initialized to SFRAME_CFA_FIXED_RA_INVALID instead of being set to 
sframe_cfa_ra_offset().

All checks but this do test SFRAME_FRE_RA_TRACKING first, which ensures 
both sframe_ra_tracking_p and SFRAME_CFA_RA_REG are defined, and then 
the predicate sframe_ra_tracking_p to determine whether RA tracking is used.
If SFRAME_FRE_RA_TRACKING is defined and sframe_ra_tracking_p returns 
true, then RA tracking is used.
Likewise, if SFRAME_FRE_RA_TRACKING is not defined or if 
sframe_ra_tracking_p returns false (evaluating lazily) RA tracking is 
not used.

What about making the following change to make all RA tracking tests 
consistent depend on SFRAME_FRE_RA_TRACKING?

#ifdef SFRAME_FRE_RA_TRACKING
   if (!sframe_ra_tracking_p ())
#endif
     {
       fixed_ra_offset = sframe_cfa_ra_offset ();
       /* Without RA tracking the fixed RA offset may not be invalid.  */
       gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
     }
   out_one (fixed_ra_offset);

What would still not be checked is the implementation error to define 
sframe_ra_tracking_p and have it return true without also defining 
SFRAME_CFA_RA_REG. This would be treated as if RA tracking was not used.

Would it therefore make sense to add the following?

#if defined (sframe_ra_tracking_p) && !defined (SFRAME_CFA_RA_REG)
   gas_assert (!sframe_ra_tracking_p ())
#endif

Also when using RA tracking an architecture should implement 
sframe_cfa_ra_offset to return SFRAME_CFA_FIXED_RA_INVALID.

Would it therefore make sense to add the following?

#ifdef SFRAME_FRE_RA_TRACKING
   if (sframe_ra_tracking_p ())
     gas_assert (sframe_cfa_ra_offset () == SFRAME_CFA_FIXED_RA_INVALID);
#endif

> 
>>     /* Process all fdes and create SFrame stack trace information.  */
>>     create_sframe_all ();
> 

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 v3 09/15] gas: User readable warnings if SFrame FDE is not generated

[Indu Bhagat]

On 5/3/24 05:30, Jens Remus wrote:
>>
>>> - skipping SFrame FDE due to .cfi_def_cfa_offset without CFA base
>>>    register in effect
>>
>> skipping SFrame FDE; .cfi_def_cfa_offset specified when no CFA base 
>> register in effect
> 
> "when no" or "without" (see "without prior SFrame FRE state" below)?

I think "without" is better.

Re: [PATCH v3 15/15] gas: Validate SFrame RA tracking and fixed RA offset

[Indu Bhagat]

On 5/3/24 09:40, Jens Remus wrote:
> Am 18.04.2024 um 22:38 schrieb Indu Bhagat:
>> On 4/12/24 07:47, Jens Remus wrote:
>>> If an architecture uses SFrame return-address (RA) tracking it must
>>> specify the fixed RA offset as invalid. Otherwise, if an architecture
>>> does not use RA tracking, it must specify a valid fixed RA offset.
>>>
>>> gas/
>>>     * gen-sframe.c: Validate SFrame RA tracking and fixed
>>>     RA offset.
>>>
>>> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
>>> ---
>>>
>>> Notes (jremus):
>>>      Changes v2 -> v3:
>>>      - New patch.
>>>      This could be made dependent on ENABLE_CHECKING (configure option
>>>      --enable-checking).
>>>
>>>   gas/gen-sframe.c | 12 ++++++++++++
>>>   1 file changed, 12 insertions(+)
>>>
>>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>>> index ca6565b0e45e..7e815f9603ef 100644
>>> --- a/gas/gen-sframe.c
>>> +++ b/gas/gen-sframe.c
>>> @@ -1532,6 +1532,18 @@ output_sframe (segT sframe_seg)
>>>     /* Setup the version specific access functions.  */
>>>     sframe_set_version (SFRAME_VERSION_2);
>>> +#ifdef SFRAME_FRE_RA_TRACKING
>>> +  if (sframe_ra_tracking_p ())
>>> +    /* With RA tracking the fixed RA offset must be invalid.  */
>>> +    gas_assert (sframe_cfa_ra_offset () == 
>>> SFRAME_CFA_FIXED_RA_INVALID);
>>> +  else
>>> +    /* Without RA tracking the fixed RA offset may not be invalid.  */
>>> +    gas_assert (sframe_cfa_ra_offset () != 
>>> SFRAME_CFA_FIXED_RA_INVALID);
>>> +#else
>>> +  /* Without RA tracking the fixed RA offset may not be invalid.  */
>>> +  gas_assert (sframe_cfa_ra_offset () != SFRAME_CFA_FIXED_RA_INVALID);
>>> +#endif
>>> +
>>
>> I am not sure if the detailed checks are worth it here (simply because 
>> of code patterns that follow).
> 
> I agree, provided the checks are performed elsewhere as you suggest.
> 
> My intention was to have checks that assist with getting SFrame support 
> for another architecture implemented correctly, without having to chase 
> subtle issues.
> 
>>
>> We use the sframe_cfa_ra_offset () function later and only in 
>> output_sframe_internal () (shown below).  How about we simply put an 
>> assert there (and get rid of the proposed thunk above):
>>
>> #ifdef sframe_ra_tracking_p
>>    if (!sframe_ra_tracking_p ())
> 
> See below.
> 
>>      {
>>        fixed_ra_offset = sframe_cfa_ra_offset ();
>>        gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
> 
> That is clever and accounts for one potential implementation issue!
> 
>>      }
>> #endif
>>    out_one (fixed_ra_offset);
>>
>> fixed_ra_offset is initialized to SFRAME_CFA_FIXED_RA_INVALID in 
>> output_sframe_internal ().
> 
> Above logic requires sframe_ra_tracking_p to be defined by an 
> architecture that is not using RA tracking. Not defining 
> sframe_ra_tracking_p would result in fixed_ra_offset being unexpectedly 
> initialized to SFRAME_CFA_FIXED_RA_INVALID instead of being set to 
> sframe_cfa_ra_offset().
> 
> All checks but this do test SFRAME_FRE_RA_TRACKING first, which ensures 
> both sframe_ra_tracking_p and SFRAME_CFA_RA_REG are defined, and then 
> the predicate sframe_ra_tracking_p to determine whether RA tracking is 
> used.
> If SFRAME_FRE_RA_TRACKING is defined and sframe_ra_tracking_p returns 
> true, then RA tracking is used.
> Likewise, if SFRAME_FRE_RA_TRACKING is not defined or if 
> sframe_ra_tracking_p returns false (evaluating lazily) RA tracking is 
> not used.
> 
> What about making the following change to make all RA tracking tests 
> consistent depend on SFRAME_FRE_RA_TRACKING?
> 
> #ifdef SFRAME_FRE_RA_TRACKING
>    if (!sframe_ra_tracking_p ())
> #endif
>      {
>        fixed_ra_offset = sframe_cfa_ra_offset ();
>        /* Without RA tracking the fixed RA offset may not be invalid.  */
>        gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
>      }
>    out_one (fixed_ra_offset);
> 

Oops, that's my bad.  Guarding with SFRAME_FRE_RA_TRACKING is more 
appropriate.

But, I think calling the sframe_cfa_ra_offset () out of 
SFRAME_FRE_RA_TRACKING portrays an imprecise meaning.  Only backends 
which opt in for SFrame define these vars/functions. (The cross build 
will likely pass because of the way code is written, but I think you get 
the idea).

I would do:

#ifdef SFRAME_FRE_RA_TRACKING
    if (!sframe_ra_tracking_p ())
      {
        fixed_ra_offset = sframe_cfa_ra_offset ();
        /* Without RA tracking the fixed RA offset may not be invalid.  */
        gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
      }
#endif
   out_one (fixed_ra_offset);


> What would still not be checked is the implementation error to define 
> sframe_ra_tracking_p and have it return true without also defining 
> SFRAME_CFA_RA_REG. This would be treated as if RA tracking was not used.
> 
> Would it therefore make sense to add the following?
> 
> #if defined (sframe_ra_tracking_p) && !defined (SFRAME_CFA_RA_REG)
>    gas_assert (!sframe_ra_tracking_p ())
> #endif
> 
> Also when using RA tracking an architecture should implement 
> sframe_cfa_ra_offset to return SFRAME_CFA_FIXED_RA_INVALID.
> 
> Would it therefore make sense to add the following?
> 
> #ifdef SFRAME_FRE_RA_TRACKING
>    if (sframe_ra_tracking_p ())
>      gas_assert (sframe_cfa_ra_offset () == SFRAME_CFA_FIXED_RA_INVALID);
> #endif
> 

All these checks are around guarding against implementation errors, 
opinions may vary. If you feel these add value, then it makes sense to 
add them.

(That said, I am thinking the name sframe_cfa_ra_offset is confusing; 
perhaps sframe_cfa_ra_fixed_offset () is better? I will think about it 
and may be include this in my list of sframe-next patches.)

>>
>>>     /* Process all fdes and create SFrame stack trace information.  */
>>>     create_sframe_all ();
>>
> 
> Thanks and regards,
> Jens

Re: [PATCH v3 15/15] gas: Validate SFrame RA tracking and fixed RA offset

[Jens Remus]

Am 04.05.2024 um 02:22 schrieb Indu Bhagat:
> On 5/3/24 09:40, Jens Remus wrote:
>> Am 18.04.2024 um 22:38 schrieb Indu Bhagat:
>>> On 4/12/24 07:47, Jens Remus wrote:
>>>> If an architecture uses SFrame return-address (RA) tracking it must
>>>> specify the fixed RA offset as invalid. Otherwise, if an architecture
>>>> does not use RA tracking, it must specify a valid fixed RA offset.
>>>>
>>>> gas/
>>>>     * gen-sframe.c: Validate SFrame RA tracking and fixed
>>>>     RA offset.
>>>>
>>>> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
>>>> ---
>>>>
>>>> Notes (jremus):
>>>>      Changes v2 -> v3:
>>>>      - New patch.
>>>>      This could be made dependent on ENABLE_CHECKING (configure option
>>>>      --enable-checking).
>>>>
>>>>   gas/gen-sframe.c | 12 ++++++++++++
>>>>   1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>>>> index ca6565b0e45e..7e815f9603ef 100644
>>>> --- a/gas/gen-sframe.c
>>>> +++ b/gas/gen-sframe.c
>>>> @@ -1532,6 +1532,18 @@ output_sframe (segT sframe_seg)
>>>>     /* Setup the version specific access functions.  */
>>>>     sframe_set_version (SFRAME_VERSION_2);
>>>> +#ifdef SFRAME_FRE_RA_TRACKING
>>>> +  if (sframe_ra_tracking_p ())
>>>> +    /* With RA tracking the fixed RA offset must be invalid.  */
>>>> +    gas_assert (sframe_cfa_ra_offset () == 
>>>> SFRAME_CFA_FIXED_RA_INVALID);
>>>> +  else
>>>> +    /* Without RA tracking the fixed RA offset may not be invalid.  */
>>>> +    gas_assert (sframe_cfa_ra_offset () != 
>>>> SFRAME_CFA_FIXED_RA_INVALID);
>>>> +#else
>>>> +  /* Without RA tracking the fixed RA offset may not be invalid.  */
>>>> +  gas_assert (sframe_cfa_ra_offset () != SFRAME_CFA_FIXED_RA_INVALID);
>>>> +#endif
>>>> +
>>>
>>> I am not sure if the detailed checks are worth it here (simply 
>>> because of code patterns that follow).
>>
>> I agree, provided the checks are performed elsewhere as you suggest.
>>
>> My intention was to have checks that assist with getting SFrame 
>> support for another architecture implemented correctly, without having 
>> to chase subtle issues.
>>
>>>
>>> We use the sframe_cfa_ra_offset () function later and only in 
>>> output_sframe_internal () (shown below).  How about we simply put an 
>>> assert there (and get rid of the proposed thunk above):
>>>
>>> #ifdef sframe_ra_tracking_p
>>>    if (!sframe_ra_tracking_p ())
>>
>> See below.
>>
>>>      {
>>>        fixed_ra_offset = sframe_cfa_ra_offset ();
>>>        gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
>>
>> That is clever and accounts for one potential implementation issue!
>>
>>>      }
>>> #endif
>>>    out_one (fixed_ra_offset);
>>>
>>> fixed_ra_offset is initialized to SFRAME_CFA_FIXED_RA_INVALID in 
>>> output_sframe_internal ().
>>
>> Above logic requires sframe_ra_tracking_p to be defined by an 
>> architecture that is not using RA tracking. Not defining 
>> sframe_ra_tracking_p would result in fixed_ra_offset being 
>> unexpectedly initialized to SFRAME_CFA_FIXED_RA_INVALID instead of 
>> being set to sframe_cfa_ra_offset().
>>
>> All checks but this do test SFRAME_FRE_RA_TRACKING first, which 
>> ensures both sframe_ra_tracking_p and SFRAME_CFA_RA_REG are defined, 
>> and then the predicate sframe_ra_tracking_p to determine whether RA 
>> tracking is used.
>> If SFRAME_FRE_RA_TRACKING is defined and sframe_ra_tracking_p returns 
>> true, then RA tracking is used.
>> Likewise, if SFRAME_FRE_RA_TRACKING is not defined or if 
>> sframe_ra_tracking_p returns false (evaluating lazily) RA tracking is 
>> not used.
>>
>> What about making the following change to make all RA tracking tests 
>> consistent depend on SFRAME_FRE_RA_TRACKING?
>>
>> #ifdef SFRAME_FRE_RA_TRACKING
>>    if (!sframe_ra_tracking_p ())
>> #endif
>>      {
>>        fixed_ra_offset = sframe_cfa_ra_offset ();
>>        /* Without RA tracking the fixed RA offset may not be invalid.  */
>>        gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
>>      }
>>    out_one (fixed_ra_offset);
>>
> 
> Oops, that's my bad.  Guarding with SFRAME_FRE_RA_TRACKING is more 
> appropriate.

Included this in the previous patch in this series.

> 
> But, I think calling the sframe_cfa_ra_offset () out of 
> SFRAME_FRE_RA_TRACKING portrays an imprecise meaning.  Only backends 
> which opt in for SFrame define these vars/functions. (The cross build 
> will likely pass because of the way code is written, but I think you get 
> the idea).
> 
> I would do:
> 
> #ifdef SFRAME_FRE_RA_TRACKING
>     if (!sframe_ra_tracking_p ())
>       {
>         fixed_ra_offset = sframe_cfa_ra_offset ();
>         /* Without RA tracking the fixed RA offset may not be invalid.  */
>         gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
>       }
> #endif
>    out_one (fixed_ra_offset);

Good catch! I did not consider that SFrame may not be implemented by an 
architecture at all.

> 
> 
>> What would still not be checked is the implementation error to define 
>> sframe_ra_tracking_p and have it return true without also defining 
>> SFRAME_CFA_RA_REG. This would be treated as if RA tracking was not used.
>>
>> Would it therefore make sense to add the following?
>>
>> #if defined (sframe_ra_tracking_p) && !defined (SFRAME_CFA_RA_REG)
>>    gas_assert (!sframe_ra_tracking_p ())
>> #endif
>>
>> Also when using RA tracking an architecture should implement 
>> sframe_cfa_ra_offset to return SFRAME_CFA_FIXED_RA_INVALID.
>>
>> Would it therefore make sense to add the following?
>>
>> #ifdef SFRAME_FRE_RA_TRACKING
>>    if (sframe_ra_tracking_p ())
>>      gas_assert (sframe_cfa_ra_offset () == SFRAME_CFA_FIXED_RA_INVALID);
>> #endif
>>
> 
> All these checks are around guarding against implementation errors, 
> opinions may vary. If you feel these add value, then it makes sense to 
> add them.
> 
> (That said, I am thinking the name sframe_cfa_ra_offset is confusing; 
> perhaps sframe_cfa_ra_fixed_offset () is better? I will think about it 
> and may be include this in my list of sframe-next patches.)

I'll leave it up to you then.

> 
>>>
>>>>     /* Process all fdes and create SFrame stack trace information.  */
>>>>     create_sframe_all ();
>>>
>>
>> Thanks and regards,
>> Jens
> 

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 v3 15/15] gas: Validate SFrame RA tracking and fixed RA offset

[Jens Remus]

Am 06.05.2024 um 13:41 schrieb Jens Remus:
> Am 04.05.2024 um 02:22 schrieb Indu Bhagat:
>> On 5/3/24 09:40, Jens Remus wrote:
>>> Am 18.04.2024 um 22:38 schrieb Indu Bhagat:
>>>> On 4/12/24 07:47, Jens Remus wrote:
>>>>> If an architecture uses SFrame return-address (RA) tracking it must
>>>>> specify the fixed RA offset as invalid. Otherwise, if an architecture
>>>>> does not use RA tracking, it must specify a valid fixed RA offset.
>>>>>
>>>>> gas/
>>>>>     * gen-sframe.c: Validate SFrame RA tracking and fixed
>>>>>     RA offset.
>>>>>
>>>>> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
>>>>> ---
>>>>>
>>>>> Notes (jremus):
>>>>>      Changes v2 -> v3:
>>>>>      - New patch.
>>>>>      This could be made dependent on ENABLE_CHECKING (configure option
>>>>>      --enable-checking).
>>>>>
>>>>>   gas/gen-sframe.c | 12 ++++++++++++
>>>>>   1 file changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>>>>> index ca6565b0e45e..7e815f9603ef 100644
>>>>> --- a/gas/gen-sframe.c
>>>>> +++ b/gas/gen-sframe.c
>>>>> @@ -1532,6 +1532,18 @@ output_sframe (segT sframe_seg)
>>>>>     /* Setup the version specific access functions.  */
>>>>>     sframe_set_version (SFRAME_VERSION_2);
>>>>> +#ifdef SFRAME_FRE_RA_TRACKING
>>>>> +  if (sframe_ra_tracking_p ())
>>>>> +    /* With RA tracking the fixed RA offset must be invalid.  */
>>>>> +    gas_assert (sframe_cfa_ra_offset () == 
>>>>> SFRAME_CFA_FIXED_RA_INVALID);
>>>>> +  else
>>>>> +    /* Without RA tracking the fixed RA offset may not be 
>>>>> invalid.  */
>>>>> +    gas_assert (sframe_cfa_ra_offset () != 
>>>>> SFRAME_CFA_FIXED_RA_INVALID);
>>>>> +#else
>>>>> +  /* Without RA tracking the fixed RA offset may not be invalid.  */
>>>>> +  gas_assert (sframe_cfa_ra_offset () != 
>>>>> SFRAME_CFA_FIXED_RA_INVALID);
>>>>> +#endif
>>>>> +
>>>>
>>>> I am not sure if the detailed checks are worth it here (simply 
>>>> because of code patterns that follow).
>>>
>>> I agree, provided the checks are performed elsewhere as you suggest.
>>>
>>> My intention was to have checks that assist with getting SFrame 
>>> support for another architecture implemented correctly, without 
>>> having to chase subtle issues.
>>>
>>>>
>>>> We use the sframe_cfa_ra_offset () function later and only in 
>>>> output_sframe_internal () (shown below).  How about we simply put an 
>>>> assert there (and get rid of the proposed thunk above):
>>>>
>>>> #ifdef sframe_ra_tracking_p
>>>>    if (!sframe_ra_tracking_p ())
>>>
>>> See below.
>>>
>>>>      {
>>>>        fixed_ra_offset = sframe_cfa_ra_offset ();
>>>>        gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
>>>
>>> That is clever and accounts for one potential implementation issue!
>>>
>>>>      }
>>>> #endif
>>>>    out_one (fixed_ra_offset);
>>>>
>>>> fixed_ra_offset is initialized to SFRAME_CFA_FIXED_RA_INVALID in 
>>>> output_sframe_internal ().
>>>
>>> Above logic requires sframe_ra_tracking_p to be defined by an 
>>> architecture that is not using RA tracking. Not defining 
>>> sframe_ra_tracking_p would result in fixed_ra_offset being 
>>> unexpectedly initialized to SFRAME_CFA_FIXED_RA_INVALID instead of 
>>> being set to sframe_cfa_ra_offset().
>>>
>>> All checks but this do test SFRAME_FRE_RA_TRACKING first, which 
>>> ensures both sframe_ra_tracking_p and SFRAME_CFA_RA_REG are defined, 
>>> and then the predicate sframe_ra_tracking_p to determine whether RA 
>>> tracking is used.
>>> If SFRAME_FRE_RA_TRACKING is defined and sframe_ra_tracking_p returns 
>>> true, then RA tracking is used.
>>> Likewise, if SFRAME_FRE_RA_TRACKING is not defined or if 
>>> sframe_ra_tracking_p returns false (evaluating lazily) RA tracking is 
>>> not used.
>>>
>>> What about making the following change to make all RA tracking tests 
>>> consistent depend on SFRAME_FRE_RA_TRACKING?
>>>
>>> #ifdef SFRAME_FRE_RA_TRACKING
>>>    if (!sframe_ra_tracking_p ())
>>> #endif
>>>      {
>>>        fixed_ra_offset = sframe_cfa_ra_offset ();
>>>        /* Without RA tracking the fixed RA offset may not be 
>>> invalid.  */
>>>        gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
>>>      }
>>>    out_one (fixed_ra_offset);
>>>
>>
>> Oops, that's my bad.  Guarding with SFRAME_FRE_RA_TRACKING is more 
>> appropriate.
> 
> Included this in the previous patch in this series.

This breaks x86-64, as gas/config/tc-i386.h does not define SFRAME_CFA_RA_REG.

Either this specific check must stay, possibly with an additional comment:

   /* Offset for the return address from CFA is fixed for some ABIs
      (e.g., AMD64), output a SFRAME_CFA_FIXED_RA_INVALID otherwise.
      NOTE: sframe_ra_tracking_p may be defined without SFRAME_CFA_RA_REG
            (e.g., AMD64), so that SFRAME_FRE_RA_TRACKING won't be defined.  */
#ifdef sframe_ra_tracking_p
   if (!sframe_ra_tracking_p ())
     {
       fixed_ra_offset = sframe_cfa_ra_offset ();
       /* Without RA tracking the fixed RA offset may not be invalid.  */
       gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
     }
#endif
   out_one (fixed_ra_offset);

or gas/config/tc-i386.h needs to define SFRAME_CFA_RA_REG, for instance as follows:

#define SFRAME_CFA_RA_REG DWARF2_DEFAULT_RETURN_COLUMN

Since an architecture may not have a RA register I wonder whether keeping the existing logic as is would be better. What is your opinion?

> 
>>
>> But, I think calling the sframe_cfa_ra_offset () out of 
>> SFRAME_FRE_RA_TRACKING portrays an imprecise meaning.  Only backends 
>> which opt in for SFrame define these vars/functions. (The cross build 
>> will likely pass because of the way code is written, but I think you 
>> get the idea).
>>
>> I would do:
>>
>> #ifdef SFRAME_FRE_RA_TRACKING
>>     if (!sframe_ra_tracking_p ())
>>       {
>>         fixed_ra_offset = sframe_cfa_ra_offset ();
>>         /* Without RA tracking the fixed RA offset may not be 
>> invalid.  */
>>         gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
>>       }
>> #endif
>>    out_one (fixed_ra_offset);
> 
> Good catch! I did not consider that SFrame may not be implemented by an 
> architecture at all.
> 
>>
>>
>>> What would still not be checked is the implementation error to define 
>>> sframe_ra_tracking_p and have it return true without also defining 
>>> SFRAME_CFA_RA_REG. This would be treated as if RA tracking was not used.
>>>
>>> Would it therefore make sense to add the following?
>>>
>>> #if defined (sframe_ra_tracking_p) && !defined (SFRAME_CFA_RA_REG)
>>>    gas_assert (!sframe_ra_tracking_p ())
>>> #endif
>>>
>>> Also when using RA tracking an architecture should implement 
>>> sframe_cfa_ra_offset to return SFRAME_CFA_FIXED_RA_INVALID.
>>>
>>> Would it therefore make sense to add the following?
>>>
>>> #ifdef SFRAME_FRE_RA_TRACKING
>>>    if (sframe_ra_tracking_p ())
>>>      gas_assert (sframe_cfa_ra_offset () == 
>>> SFRAME_CFA_FIXED_RA_INVALID);
>>> #endif
>>>
>>
>> All these checks are around guarding against implementation errors, 
>> opinions may vary. If you feel these add value, then it makes sense to 
>> add them.
>>
>> (That said, I am thinking the name sframe_cfa_ra_offset is confusing; 
>> perhaps sframe_cfa_ra_fixed_offset () is better? I will think about it 
>> and may be include this in my list of sframe-next patches.)
> 
> I'll leave it up to you then.
> 
>>
>>>>
>>>>>     /* Process all fdes and create SFrame stack trace information.  */
>>>>>     create_sframe_all ();
>>>>
>>>
>>> Thanks and regards,
>>> Jens
>>
> 
> Regards,
> Jens

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/