[PATCH,V2 0/2] sframe: partially process some .cfi_escape

[PATCH,V2 0/2] sframe: partially process some .cfi_escape

[Indu Bhagat]

Hi,

This version addresses all the review comments so far except the
following:
  - Use pointer-to-const for args
This will be addressed in a subsequent patch.

Thanks,
Indu Bhagat (2):
  gas: sframe: adjust warning text for DW_CFA_val_offset case
  gas: sframe: partially process DWARF expressions in CFI_escape

 gas/dw2gencfi.c                               |   6 -
 gas/dw2gencfi.h                               |   6 +
 gas/gen-sframe.c                              | 181 +++++++++++++++++-
 .../gas/cfi-sframe/cfi-sframe-common-9.d      |  22 +++
 .../gas/cfi-sframe/cfi-sframe-common-9.s      |  17 ++
 .../cfi-sframe/cfi-sframe-x86_64-empty-1.d    |  17 ++
 .../cfi-sframe/cfi-sframe-x86_64-empty-1.s    |  10 +
 .../cfi-sframe/cfi-sframe-x86_64-empty-2.d    |  17 ++
 .../cfi-sframe/cfi-sframe-x86_64-empty-2.s    |  12 ++
 gas/testsuite/gas/cfi-sframe/cfi-sframe.exp   |   3 +
 10 files changed, 280 insertions(+), 11 deletions(-)
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-common-9.d
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-common-9.s
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-empty-1.d
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-empty-1.s
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-empty-2.d
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-empty-2.s

-- 
2.43.0

[PATCH,V2 1/2] gas: sframe: adjust warning text for DW_CFA_val_offset case

[Indu Bhagat]

[New in V2]

Soon we will invoke the same API for handling DW_CFA_val_offset in a
.cfi_escape directive.  The current substring ".cfi_val_offset" can be
confusing for the user to see.  Change it.

gas/
	* gen-sframe.c (sframe_xlate_do_val_offset): Adjust the warning
	message.
---
 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 13478efab6b..a592e0f718d 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -1134,7 +1134,7 @@ sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
       /* Ignore SP reg, if offset matches assumed default rule.  */
       || (cfi_insn->u.ri.reg == SFRAME_CFA_SP_REG && cfi_insn->u.ri.offset != 0))
     {
-      as_warn (_("skipping SFrame FDE; %s register %u in .cfi_val_offset"),
+      as_warn (_("skipping SFrame FDE; DW_CFA_val_offset with %s register %u"),
 	       sframe_register_name (cfi_insn->u.ri.reg), cfi_insn->u.ri.reg);
       return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
     }
-- 
2.43.0

[PATCH,V2 2/2] gas: sframe: partially process DWARF expressions in CFI_escape

[Indu Bhagat]

[Changes in V2]
 - Use pointer-to-const when applicable for local vars.
 - Early exit if there is no cfi_escape expression.
 - Restructure functionality to accommodate the fact that a valid
   unwind info may be split across multiple .cfi_escape. If this is the
   case, the current implementation bails out.  Handling this case does
   not appear to be worth the effort.
 - Include checking with SFRAME_CFA_RA_REG because we need to detect the
   cases when .cfi_escape can affect RA.
 - Use if conditional instead of assert block.
[End of changes in V2]

CFI_escape is most commonly used to include DWARF expressions in the
unwind information.  One may also use CFI_escape to add OS-specific CFI
opcodes.  Up until now, SFrame generation process would skip generating
SFrame FDE at the mere sight of a CFI_escape opcode.

Fine tune the handling of CFI_escape for SFrame generation by explicitly
checking for some "harmless" (in context of SFrame generation)
CFI_escape DWARF expressions:
  - DW_CFA_expression affecting registers of no significance to SFrame
    stack trace info
  - DW_CFA_value_offset affecting registers of no significance to SFrame
    stack trace info

Expose the current cfi_escape_data structure in dw2gencfi.c to the
relevant header file to allow SFrame generation APIs to use it too.

In future, SFrame stack trace may support non-SP/FP as base register
(albeit in limited form).  Add an explicit check in
sframe_xlate_do_escape_expr (to test against the current CFA register)
to ensure the functionality continues to work.

Also, add a common test with DWARF reg 12 which is non SP / FP on both
x86_64 and aarch64.

gas/
	* gas/dw2gencfi.c (struct cfi_escape_data): Move from ...
	* gas/dw2gencfi.h (struct cfi_escape_data): ... to.
	* gas/gen-sframe.c (sframe_xlate_do_val_offset): Include string
	for .cfi_escape conditionally.
	(sframe_xlate_do_escape_expr): New definition.
	(sframe_xlate_do_escape_val_offset): Likewise.
	(sframe_xlate_do_cfi_escape): Likewise.
	(sframe_do_cfi_insn): Handle CFI_escape explicitly.

gas/testsuite/
	* gas/cfi-sframe/cfi-sframe.exp: Add new tests.
	* gas/cfi-sframe/cfi-sframe-common-9.d: New test.
	* gas/cfi-sframe/cfi-sframe-common-9.s: New test.
	* gas/cfi-sframe/cfi-sframe-x86_64-empty-1.d: New test.
	* gas/cfi-sframe/cfi-sframe-x86_64-empty-1.s: New test.
	* gas/cfi-sframe/cfi-sframe-x86_64-empty-2.d: New test.
	* gas/cfi-sframe/cfi-sframe-x86_64-empty-2.s: New test.
---
 gas/dw2gencfi.c                               |   6 -
 gas/dw2gencfi.h                               |   6 +
 gas/gen-sframe.c                              | 181 +++++++++++++++++-
 .../gas/cfi-sframe/cfi-sframe-common-9.d      |  22 +++
 .../gas/cfi-sframe/cfi-sframe-common-9.s      |  17 ++
 .../cfi-sframe/cfi-sframe-x86_64-empty-1.d    |  17 ++
 .../cfi-sframe/cfi-sframe-x86_64-empty-1.s    |  10 +
 .../cfi-sframe/cfi-sframe-x86_64-empty-2.d    |  17 ++
 .../cfi-sframe/cfi-sframe-x86_64-empty-2.s    |  12 ++
 gas/testsuite/gas/cfi-sframe/cfi-sframe.exp   |   3 +
 10 files changed, 280 insertions(+), 11 deletions(-)
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-common-9.d
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-common-9.s
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-empty-1.d
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-empty-1.s
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-empty-2.d
 create mode 100644 gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-empty-2.s

diff --git a/gas/dw2gencfi.c b/gas/dw2gencfi.c
index 012af0ff33e..41e15cd1463 100644
--- a/gas/dw2gencfi.c
+++ b/gas/dw2gencfi.c
@@ -368,12 +368,6 @@ generic_dwarf2_emit_offset (symbolS *symbol, unsigned int size)
 }
 #endif
 
-struct cfi_escape_data
-{
-  struct cfi_escape_data *next;
-  expressionS exp;
-};
-
 struct cie_entry
 {
   struct cie_entry *next;
diff --git a/gas/dw2gencfi.h b/gas/dw2gencfi.h
index 42eb3863a0a..9b0e5979c0d 100644
--- a/gas/dw2gencfi.h
+++ b/gas/dw2gencfi.h
@@ -93,6 +93,12 @@ extern void cfi_add_CFA_restore_state (void);
 #define MULTIPLE_FRAME_SECTIONS (SUPPORT_FRAME_LINKONCE || SUPPORT_COMPACT_EH \
 				 || TARGET_MULTIPLE_EH_FRAME_SECTIONS)
 
+struct cfi_escape_data
+{
+  struct cfi_escape_data *next;
+  expressionS exp;
+};
+
 struct cfi_insn_data
 {
   struct cfi_insn_data *next;
diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index a592e0f718d..c06ae6b5a45 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -1121,7 +1121,8 @@ sframe_xlate_do_offset (struct sframe_xlate_ctx *xlate_ctx,
 
 static int
 sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
-			    struct cfi_insn_data *cfi_insn)
+			    struct cfi_insn_data *cfi_insn,
+			    bool cfi_escape_p)
 {
   /* Previous value of register is CFA + offset.  However, if the specified
      register is not interesting (SP, FP, or RA reg), the current
@@ -1134,7 +1135,8 @@ sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
       /* Ignore SP reg, if offset matches assumed default rule.  */
       || (cfi_insn->u.ri.reg == SFRAME_CFA_SP_REG && cfi_insn->u.ri.offset != 0))
     {
-      as_warn (_("skipping SFrame FDE; DW_CFA_val_offset with %s register %u"),
+      as_warn (_("skipping SFrame FDE; %sDW_CFA_val_offset with %s reg %u"),
+	       cfi_escape_p ? ".cfi_escape " : "",
 	       sframe_register_name (cfi_insn->u.ri.reg), cfi_insn->u.ri.reg);
       return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
     }
@@ -1310,6 +1312,173 @@ sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
   return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
 }
 
+/* Handle DW_CFA_expression in .cfi_escape.  */
+
+static int
+sframe_xlate_do_escape_expr (struct sframe_xlate_ctx *xlate_ctx,
+			     struct cfi_insn_data *cfi_insn)
+{
+  const struct cfi_escape_data *e = cfi_insn->u.esc;
+  const struct sframe_row_entry *cur_fre = NULL;
+  unsigned int reg = 0;
+  int err = SFRAME_XLATE_OK;
+  int i = 0;
+
+  if (!e || !e->next)
+    return SFRAME_XLATE_ERR_INVAL;
+
+  /* Check roughly for expression of the kind
+     DW_CFA_expression: r1 (rdx) (DW_OP_bregN (reg): XXX) */
+#define CFI_ESC_NUM_EXP 4
+  offsetT items[CFI_ESC_NUM_EXP] = {0};
+  while (e->next)
+    {
+      e = e->next;
+      if (i >= CFI_ESC_NUM_EXP)
+	return SFRAME_XLATE_ERR_NOTREPRESENTED;
+      items[i] = e->exp.X_add_number;
+      i++;
+    }
+
+  if (i <= CFI_ESC_NUM_EXP - 1)
+    return SFRAME_XLATE_ERR_NOTREPRESENTED;
+
+  cur_fre = xlate_ctx->cur_fre;
+  reg = items[0];
+#undef CFI_ESC_NUM_EXP
+
+  if (reg == SFRAME_CFA_SP_REG || reg == SFRAME_CFA_FP_REG
+#ifdef SFRAME_FRE_RA_TRACKING
+      || (sframe_ra_tracking_p () && reg == SFRAME_CFA_RA_REG)
+#endif
+      || reg == cur_fre->cfa_base_reg)
+    {
+      as_warn (_("skipping SFrame FDE; "
+		 ".cfi_escape DW_CFA_expression with %s reg %u"),
+	       sframe_register_name (reg), reg);
+      err = SFRAME_XLATE_ERR_NOTREPRESENTED;
+    }
+
+  return err;
+}
+
+/* Handle DW_CFA_val_offset in .cfi_escape.  */
+
+static int
+sframe_xlate_do_escape_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
+				   struct cfi_insn_data *cfi_insn)
+{
+  const struct cfi_escape_data *e = cfi_insn->u.esc;
+  unsigned int reg = 0;
+  offsetT offset = 0;
+  int err = SFRAME_XLATE_OK;
+  int i = 0;
+
+  if (!e || !e->next)
+    return SFRAME_XLATE_ERR_INVAL;
+
+  /* Check for (DW_CFA_val_offset reg scaled_offset) sequence.  */
+#define CFI_ESC_NUM_EXP 2
+  offsetT items[CFI_ESC_NUM_EXP] = {0};
+  while (e->next)
+    {
+      e = e->next;
+      if (i >= CFI_ESC_NUM_EXP)
+	return SFRAME_XLATE_ERR_NOTREPRESENTED;
+      items[i] = e->exp.X_add_number;
+      i++;
+    }
+  if (i <= CFI_ESC_NUM_EXP - 1)
+    return SFRAME_XLATE_ERR_NOTREPRESENTED;
+
+  reg = items[0];
+  offset = items[1];
+#undef CFI_ESC_NUM_EXP
+
+  /* Invoke sframe_xlate_do_val_offset itself for checking.  */
+  struct cfi_insn_data *temp = XCNEW (struct cfi_insn_data);
+  temp->insn = DW_CFA_val_offset;
+  temp->u.ri.reg = reg;
+  temp->u.ri.offset = offset;
+
+  err = sframe_xlate_do_val_offset (xlate_ctx, temp, true);
+  XDELETE (temp);
+
+  return err;
+}
+
+/* Handle CFI_escape in SFrame context.
+
+   .cfi_escape CFI directive allows the user to add arbitrary bytes to the
+   unwind info.  DWARF expressions commonly follow after CFI_escape (fake CFI)
+   DWARF opcode.  One might also use CFI_escape to add OS-specific CFI opcodes
+   even.
+
+   Complex unwind info added using .cfi_escape directive _may_ be of no
+   consequence for SFrame when the affected registers are not SP, FP, RA or
+   CFA.  The challenge in confirming the afore-mentioned is that it needs full
+   parsing and validation of the bytes presented after .cfi_escape.  Here we
+   take a case-by-case approach towards skipping _some_ instances of
+   .cfi_escape: skip those that can be *easily* determined to be harmless in
+   the context of SFrame stack trace information.
+
+   This function partially processes bytes following .cfi_escape and returns
+   SFRAME_XLATE_OK if OK to skip.  */
+
+static int
+sframe_xlate_do_cfi_escape (struct sframe_xlate_ctx *xlate_ctx,
+			    struct cfi_insn_data *cfi_insn)
+{
+  const struct cfi_escape_data *e;
+  int err = SFRAME_XLATE_OK;
+  offsetT firstop;
+
+  e = cfi_insn->u.esc;
+
+  if (!e)
+    return SFRAME_XLATE_ERR_INVAL;
+
+  firstop = e->exp.X_add_number;
+  switch (firstop)
+    {
+    case DW_CFA_nop:
+      while (e->next)
+	{
+	  e = e->next;
+	  if (e->exp.X_add_number != DW_CFA_nop)
+	    {
+	      err = SFRAME_XLATE_ERR_NOTREPRESENTED;
+	      break;
+	    }
+	}
+      break;
+
+    case DW_CFA_expression:
+      return sframe_xlate_do_escape_expr (xlate_ctx, cfi_insn);
+
+    case DW_CFA_val_offset:
+      return sframe_xlate_do_escape_val_offset (xlate_ctx, cfi_insn);
+
+    /* FIXME - Also add processing for DW_CFA_GNU_args_size in future?  */
+
+    default:
+      err = SFRAME_XLATE_ERR_NOTREPRESENTED;
+      break;
+    }
+
+  if (err == SFRAME_XLATE_ERR_NOTREPRESENTED)
+    {
+      /* In all other cases (e.g., DW_CFA_def_cfa_expression or other
+	 OS-specific CFI opcodes), skip inspecting the DWARF expression.
+	 This may impact the asynchronicity due to loss of coverage.
+	 Continue to warn the user and bail out.  */
+      as_warn (_("skipping SFrame FDE; .cfi_escape with op (%#lx)"),
+	       (unsigned long)firstop);
+    }
+
+  return err;
+}
+
 /* Returns the DWARF call frame instruction name or fake CFI name for the
    specified CFI opcode, or NULL if the value is not recognized.  */
 
@@ -1384,7 +1553,7 @@ sframe_do_cfi_insn (struct sframe_xlate_ctx *xlate_ctx,
       err = sframe_xlate_do_offset (xlate_ctx, cfi_insn);
       break;
     case DW_CFA_val_offset:
-      err = sframe_xlate_do_val_offset (xlate_ctx, cfi_insn);
+      err = sframe_xlate_do_val_offset (xlate_ctx, cfi_insn, false);
       break;
     case DW_CFA_remember_state:
       err = sframe_xlate_do_remember_state (xlate_ctx);
@@ -1406,6 +1575,9 @@ sframe_do_cfi_insn (struct sframe_xlate_ctx *xlate_ctx,
     case DW_CFA_register:
       err = sframe_xlate_do_register (xlate_ctx, cfi_insn);
       break;
+    case CFI_escape:
+      err = sframe_xlate_do_cfi_escape (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.  */
@@ -1413,8 +1585,7 @@ sframe_do_cfi_insn (struct sframe_xlate_ctx *xlate_ctx,
     case DW_CFA_same_value:
       break;
     default:
-      /* Following skipped operations do, however, impact the asynchronicity:
-	  - CFI_escape.  */
+      /* Other skipped operations may, however, impact the asynchronicity.  */
       {
 	const char *cfi_name = sframe_get_cfi_name (op);
 
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-9.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-9.d
new file mode 100644
index 00000000000..80c92357073
--- /dev/null
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-9.d
@@ -0,0 +1,22 @@
+#as: --gsframe
+#objdump: --sframe=.sframe
+#name: SFrame cfi_escape test
+#...
+Contents of the SFrame section .sframe:
+
+  Header :
+
+    Version: SFRAME_VERSION_2
+    Flags: NONE
+#?    CFA fixed FP offset: \-?\d+
+#?    CFA fixed RA offset: \-?\d+
+    Num FDEs: 1
+    Num FREs: 2
+
+  Function Index :
+    func idx \[0\]: pc = 0x0, size = 8 bytes
+    STARTPC + CFA + FP + RA +
+#...
+    0+0004 +sp\+16 +u +[uf] +
+
+#pass
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-9.s b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-9.s
new file mode 100644
index 00000000000..c93b8e66155
--- /dev/null
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-9.s
@@ -0,0 +1,17 @@
+## CFI_escape may be used to encode DWARF expressions among other things.
+## Depending on the register applicable for the DWARF expression, skipping
+## SFrame FDE may be OK: SFrame stack trace information is relevant for SP, FP
+## and RA only.  In this test, CFI_escape is safe to skip (does not affect
+## correctness of SFrame data).  The register 0xc is non SP / FP on both
+## aarch64 and x86_64.
+	.cfi_startproc
+	.long 0
+	.cfi_def_cfa_offset 16
+# DW_CFA_expression for reg 0xc
+	.cfi_escape 0x10,0xc,0x2,0x76,0x78
+	.cfi_escape 0x0
+	.cfi_escape 0x0,0x0,0x0,0x0
+# DW_CFA_val_offset for reg 0xc
+	.cfi_escape 0x14,0xc,0x4
+	.long 0
+	.cfi_endproc
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-empty-1.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-empty-1.d
new file mode 100644
index 00000000000..01239944863
--- /dev/null
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-empty-1.d
@@ -0,0 +1,17 @@
+#as: --gsframe
+#warning: skipping SFrame FDE; \.cfi_escape DW\_CFA\_expression with SP reg 7
+#objdump: --sframe=.sframe
+#name: CFI_escape with register of significance to SFrame
+#...
+Contents of the SFrame section .sframe:
+
+  Header :
+
+    Version: SFRAME_VERSION_2
+    Flags: NONE
+#?    CFA fixed FP offset: \-?\d+
+#?    CFA fixed RA offset: \-?\d+
+    Num FDEs: 0
+    Num FREs: 0
+
+#pass
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-empty-1.s b/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-empty-1.s
new file mode 100644
index 00000000000..168ac06bc4f
--- /dev/null
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-empty-1.s
@@ -0,0 +1,10 @@
+## CFI_escape may be used to encode DWARF expressions among other things.
+## In this test, a DWARF expression involving a register of interest (REG_SP on
+## x86_64, which is also the CFA in the current FRE) is seen.  SFrame
+## generation, is skipped and a warning issued to the user.
+	.cfi_startproc
+	.long 0
+	.cfi_def_cfa_offset 16
+	.cfi_escape 0x10,0x7,0x2,0x76,0x78
+	.long 0
+	.cfi_endproc
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-empty-2.d b/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-empty-2.d
new file mode 100644
index 00000000000..482803b65a2
--- /dev/null
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-empty-2.d
@@ -0,0 +1,17 @@
+#as: --gsframe
+#warning: skipping SFrame FDE; \.cfi\_escape DW\_CFA\_val\_offset with FP reg 6
+#objdump: --sframe=.sframe
+#name: CFI_escape with register of significance to SFrame II
+#...
+Contents of the SFrame section .sframe:
+
+  Header :
+
+    Version: SFRAME_VERSION_2
+    Flags: NONE
+#?    CFA fixed FP offset: \-?\d+
+#?    CFA fixed RA offset: \-?\d+
+    Num FDEs: 0
+    Num FREs: 0
+
+#pass
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-empty-2.s b/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-empty-2.s
new file mode 100644
index 00000000000..90e1a7e0f28
--- /dev/null
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-x86_64-empty-2.s
@@ -0,0 +1,12 @@
+## CFI_escape may be used to encode DWARF expressions among other things.
+## In this test, a DWARF expression involving a register of interest (REG_SP on
+## x86_64, which is also the CFA in the current FRE) is seen.  SFrame
+## generation, is skipped and a warning issued to the user.
+	.cfi_startproc
+	.long 0
+	.cfi_def_cfa_offset 16
+# Equivalent to .cfi_val_offset rbp, -16
+# DW_CFA_val_offset, rbp, scaled offset
+	.cfi_escape 0x14,0x6,0x2
+	.long 0
+	.cfi_endproc
diff --git a/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp b/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
index b119b9da73d..fdf7d9518cb 100644
--- a/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
@@ -78,6 +78,7 @@ if  { ([istarget "x86_64-*-*"] || [istarget "aarch64*-*-*"]) \
     run_dump_test "cfi-sframe-common-6"
     run_dump_test "cfi-sframe-common-7"
     run_dump_test "cfi-sframe-common-8"
+    run_dump_test "cfi-sframe-common-9"
 
     run_dump_test "common-empty-1"
     run_dump_test "common-empty-2"
@@ -89,6 +90,8 @@ if { [istarget "x86_64-*-*"] && [gas_sframe_check] } then {
     if { [gas_x86_64_check] } then {
 	set ASFLAGS "$ASFLAGS --64"
 	run_dump_test "cfi-sframe-x86_64-1"
+	run_dump_test "cfi-sframe-x86_64-empty-1"
+	run_dump_test "cfi-sframe-x86_64-empty-2"
 	set ASFLAGS "$old_ASFLAGS"
     }
 }
-- 
2.43.0

Re: [PATCH,V2 2/2] gas: sframe: partially process DWARF expressions in CFI_escape

[Jens Remus]

On 05.02.2025 00:10, Indu Bhagat wrote:

>   - Restructure functionality to accommodate the fact that a valid
>     unwind info may be split across multiple .cfi_escape. If this is the
>     case, the current implementation bails out.  Handling this case does
>     not appear to be worth the effort.

Is that worth to be mentioned in the commit message?  How is multiple
unwind info within one .cfi_escape handled?

> Fine tune the handling of CFI_escape for SFrame generation by explicitly
> checking for some "harmless" (in context of SFrame generation)
> CFI_escape DWARF expressions:
>    - DW_CFA_expression affecting registers of no significance to SFrame
>      stack trace info
>    - DW_CFA_value_offset affecting registers of no significance to SFrame
>      stack trace info

Thank you for including DW_CFA_value_offset, which may be beneficial
for s390.  I will give it a try.

> Also, add a common test with DWARF reg 12 which is non SP / FP on both
> x86_64 and aarch64.

DWARF register 12 is also non-SP/FP on s390. :-)

> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c

> @@ -1121,7 +1121,8 @@ sframe_xlate_do_offset (struct sframe_xlate_ctx *xlate_ctx,
>   
>   static int
>   sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
> -			    struct cfi_insn_data *cfi_insn)
> +			    struct cfi_insn_data *cfi_insn,
> +			    bool cfi_escape_p)
>   {
>     /* Previous value of register is CFA + offset.  However, if the specified
>        register is not interesting (SP, FP, or RA reg), the current
> @@ -1134,7 +1135,8 @@ sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>         /* Ignore SP reg, if offset matches assumed default rule.  */
>         || (cfi_insn->u.ri.reg == SFRAME_CFA_SP_REG && cfi_insn->u.ri.offset != 0))
>       {
> -      as_warn (_("skipping SFrame FDE; DW_CFA_val_offset with %s register %u"),
> +      as_warn (_("skipping SFrame FDE; %sDW_CFA_val_offset with %s reg %u"),
> +	       cfi_escape_p ? ".cfi_escape " : "",
>   	       sframe_register_name (cfi_insn->u.ri.reg), cfi_insn->u.ri.reg);

Why not drop or adjust your preceding patch and use the following,
as you touch the warning message anyway:

	as_warn (_("skipping SFrame FDE; %s with %s register %u"),
		cfi_escape_p ? ".cfi_escape DW_CFA_val_offset" : ".cfi_val_offset",
		...

>         return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
>       }
> @@ -1310,6 +1312,173 @@ sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
>     return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
>   }

Thanks and regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303)
+49-7031-16-1128 Office
jremus@de.ibm.com

IBM

IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/

Re: [PATCH,V2 2/2] gas: sframe: partially process DWARF expressions in CFI_escape

[Indu Bhagat]

On 2/5/25 8:31 AM, Jens Remus wrote:
> On 05.02.2025 00:10, Indu Bhagat wrote:
> 
>>   - Restructure functionality to accommodate the fact that a valid
>>     unwind info may be split across multiple .cfi_escape. If this is the
>>     case, the current implementation bails out.  Handling this case does
>>     not appear to be worth the effort.
> 
> Is that worth to be mentioned in the commit message?  How is multiple
> unwind info within one .cfi_escape handled?
> 

I remove the stubs between [Changes in VN] and [End of changes in VN] 
from the commit log before pushing. Admittedly this has caused confusion 
previously too...

Multiple unwind info within one .cfi_escape is not handled.  We skip 
generating FDE and warn the user.

>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> 
>> @@ -1121,7 +1121,8 @@ sframe_xlate_do_offset (struct sframe_xlate_ctx 
>> *xlate_ctx,
>>   static int
>>   sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx 
>> ATTRIBUTE_UNUSED,
>> -                struct cfi_insn_data *cfi_insn)
>> +                struct cfi_insn_data *cfi_insn,
>> +                bool cfi_escape_p)
>>   {
>>     /* Previous value of register is CFA + offset.  However, if the 
>> specified
>>        register is not interesting (SP, FP, or RA reg), the current
>> @@ -1134,7 +1135,8 @@ sframe_xlate_do_val_offset (struct 
>> sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>>         /* Ignore SP reg, if offset matches assumed default rule.  */
>>         || (cfi_insn->u.ri.reg == SFRAME_CFA_SP_REG && cfi_insn- 
>> >u.ri.offset != 0))
>>       {
>> -      as_warn (_("skipping SFrame FDE; DW_CFA_val_offset with %s 
>> register %u"),
>> +      as_warn (_("skipping SFrame FDE; %sDW_CFA_val_offset with %s 
>> reg %u"),
>> +           cfi_escape_p ? ".cfi_escape " : "",
>>              sframe_register_name (cfi_insn->u.ri.reg), cfi_insn- 
>> >u.ri.reg);
> 
> Why not drop or adjust your preceding patch and use the following,
> as you touch the warning message anyway:
> 
>      as_warn (_("skipping SFrame FDE; %s with %s register %u"),
>          cfi_escape_p ? ".cfi_escape DW_CFA_val_offset" : 
> ".cfi_val_offset",
>          ...
> 

Ah right! :)
Will do.

>>         return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
>>       }
>> @@ -1310,6 +1312,173 @@ sframe_xlate_do_gnu_window_save (struct 
>> sframe_xlate_ctx *xlate_ctx,
>>     return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
>>   }
> 
> Thanks and regards,
> Jens

Re: [PATCH,V2 2/2] gas: sframe: partially process DWARF expressions in CFI_escape

[Jens Remus]

On 05.02.2025 20:59, Indu Bhagat wrote:
> On 2/5/25 8:31 AM, Jens Remus wrote:
>> On 05.02.2025 00:10, Indu Bhagat wrote:
>>
>>>   - Restructure functionality to accommodate the fact that a valid
>>>     unwind info may be split across multiple .cfi_escape. If this is the
>>>     case, the current implementation bails out.  Handling this case does
>>>     not appear to be worth the effort.
>>
>> Is that worth to be mentioned in the commit message?  How is multiple
>> unwind info within one .cfi_escape handled?
>>
> 
> I remove the stubs between [Changes in VN] and [End of changes in VN] from the commit log before pushing. Admittedly this has caused confusion previously too...

Aww, sorry, I meant it would make sense to mention above and below in
the commit message.  That is that neither CFI instructions split across
multiple .cfi_escape nor multiple CFI instructions encoded in a single
.cfi_escape are handled and the SFrame FDE is skipped.

> Multiple unwind info within one .cfi_escape is not handled.  We skip generating FDE and warn the user.

Regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303)
+49-7031-16-1128 Office
jremus@de.ibm.com

IBM

IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/

Re: [PATCH,V2 2/2] gas: sframe: partially process DWARF expressions in CFI_escape

[Jens Remus]

On 05.02.2025 00:10, Indu Bhagat wrote:

> Fine tune the handling of CFI_escape for SFrame generation by explicitly
> checking for some "harmless" (in context of SFrame generation)
> CFI_escape DWARF expressions:
>    - DW_CFA_expression affecting registers of no significance to SFrame
>      stack trace info
>    - DW_CFA_value_offset affecting registers of no significance to SFrame
>      stack trace info

I successfully tested the DW_CFA_value_offset handling with following
small fix on s390x.  A build of Glibc with SFrame no longer skips FDEs
because of .cfi_escape.

> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c

> +/* Handle DW_CFA_val_offset in .cfi_escape.  */
> +
> +static int
> +sframe_xlate_do_escape_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
> +				   struct cfi_insn_data *cfi_insn)
> +{
> +  const struct cfi_escape_data *e = cfi_insn->u.esc;
> +  unsigned int reg = 0;
> +  offsetT offset = 0;
> +  int err = SFRAME_XLATE_OK;
> +  int i = 0;
> +
> +  if (!e || !e->next)
> +    return SFRAME_XLATE_ERR_INVAL;
> +
> +  /* Check for (DW_CFA_val_offset reg scaled_offset) sequence.  */
> +#define CFI_ESC_NUM_EXP 2
> +  offsetT items[CFI_ESC_NUM_EXP] = {0};
> +  while (e->next)
> +    {
> +      e = e->next;
> +      if (i >= CFI_ESC_NUM_EXP)
> +	return SFRAME_XLATE_ERR_NOTREPRESENTED;
> +      items[i] = e->exp.X_add_number;
> +      i++;
> +    }
> +  if (i <= CFI_ESC_NUM_EXP - 1)
> +    return SFRAME_XLATE_ERR_NOTREPRESENTED;
> +
> +  reg = items[0];
> +  offset = items[1];
> +#undef CFI_ESC_NUM_EXP
> +
> +  /* Invoke sframe_xlate_do_val_offset itself for checking.  */
> +  struct cfi_insn_data *temp = XCNEW (struct cfi_insn_data);
> +  temp->insn = DW_CFA_val_offset;
> +  temp->u.ri.reg = reg;
> +  temp->u.ri.offset = offset;

temp->u.ri.offset = offset * DWARF2_CIE_DATA_ALIGNMENT;

The scaling needs to be undone.  Looking at output_cfi_insn() handling
of DW_CFA_val_offset it makes sense to do it in this assignment instead
of when assigning offset = items[1] a few lines above.  But that is up
to you.

GAS cfi_add_CFA_val_offset() already has asserted that
DWARF2_CIE_DATA_ALIGNMENT is not zero.

> +
> +  err = sframe_xlate_do_val_offset (xlate_ctx, temp, true);
> +  XDELETE (temp);
> +
> +  return err;
> +}

Regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303)
+49-7031-16-1128 Office
jremus@de.ibm.com

IBM

IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/

Re: [PATCH,V2 1/2] gas: sframe: adjust warning text for DW_CFA_val_offset case

[Jan Beulich]

On 05.02.2025 00:10, Indu Bhagat wrote:
> [New in V2]
> 
> Soon we will invoke the same API for handling DW_CFA_val_offset in a
> .cfi_escape directive.  The current substring ".cfi_val_offset" can be
> confusing for the user to see.  Change it.
> 
> gas/
> 	* gen-sframe.c (sframe_xlate_do_val_offset): Adjust the warning
> 	message.

OK.

Jan

Re: [PATCH,V2 2/2] gas: sframe: partially process DWARF expressions in CFI_escape

[Jan Beulich]

On 05.02.2025 00:10, Indu Bhagat wrote:
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -1121,7 +1121,8 @@ sframe_xlate_do_offset (struct sframe_xlate_ctx *xlate_ctx,
>  
>  static int
>  sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
> -			    struct cfi_insn_data *cfi_insn)
> +			    struct cfi_insn_data *cfi_insn,
> +			    bool cfi_escape_p)
>  {
>    /* Previous value of register is CFA + offset.  However, if the specified
>       register is not interesting (SP, FP, or RA reg), the current
> @@ -1134,7 +1135,8 @@ sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>        /* Ignore SP reg, if offset matches assumed default rule.  */
>        || (cfi_insn->u.ri.reg == SFRAME_CFA_SP_REG && cfi_insn->u.ri.offset != 0))
>      {
> -      as_warn (_("skipping SFrame FDE; DW_CFA_val_offset with %s register %u"),
> +      as_warn (_("skipping SFrame FDE; %sDW_CFA_val_offset with %s reg %u"),
> +	       cfi_escape_p ? ".cfi_escape " : "",
>  	       sframe_register_name (cfi_insn->u.ri.reg), cfi_insn->u.ri.reg);

Hmm - I thought patch 1 was added to eliminate the need to alter this
here again.

> @@ -1310,6 +1312,173 @@ sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
>    return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
>  }
>  
> +/* Handle DW_CFA_expression in .cfi_escape.  */
> +
> +static int
> +sframe_xlate_do_escape_expr (struct sframe_xlate_ctx *xlate_ctx,
> +			     struct cfi_insn_data *cfi_insn)
> +{
> +  const struct cfi_escape_data *e = cfi_insn->u.esc;
> +  const struct sframe_row_entry *cur_fre = NULL;
> +  unsigned int reg = 0;
> +  int err = SFRAME_XLATE_OK;
> +  int i = 0;

unsigned int please.

> +  if (!e || !e->next)
> +    return SFRAME_XLATE_ERR_INVAL;

Why the check for e being NULL? The caller already de-referenced it. And
the e->next check is redundant with the while() below.

> +  /* Check roughly for expression of the kind
> +     DW_CFA_expression: r1 (rdx) (DW_OP_bregN (reg): XXX) */
> +#define CFI_ESC_NUM_EXP 4
> +  offsetT items[CFI_ESC_NUM_EXP] = {0};
> +  while (e->next)
> +    {
> +      e = e->next;
> +      if (i >= CFI_ESC_NUM_EXP)
> +	return SFRAME_XLATE_ERR_NOTREPRESENTED;
> +      items[i] = e->exp.X_add_number;
> +      i++;
> +    }
> +
> +  if (i <= CFI_ESC_NUM_EXP - 1)
> +    return SFRAME_XLATE_ERR_NOTREPRESENTED;

See the respective comment on sframe_xlate_do_escape_val_offset() below.

> +  cur_fre = xlate_ctx->cur_fre;
> +  reg = items[0];
> +#undef CFI_ESC_NUM_EXP

You fetch 4 bytes, then use only the first? Shouldn't you at least check
expression length and DW_OP_breg<N> (and hence the 2nd register number)
as well? What about DW_OP_reg<N>, DW_OP_bregx, and DW_OP_regx?

> +/* Handle DW_CFA_val_offset in .cfi_escape.  */
> +
> +static int
> +sframe_xlate_do_escape_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,

Why ATTRIBUTE_UNUSED? The parameter is used at the bottom of the function.

> +				   struct cfi_insn_data *cfi_insn)
> +{
> +  const struct cfi_escape_data *e = cfi_insn->u.esc;
> +  unsigned int reg = 0;
> +  offsetT offset = 0;
> +  int err = SFRAME_XLATE_OK;
> +  int i = 0;
> +
> +  if (!e || !e->next)
> +    return SFRAME_XLATE_ERR_INVAL;
> +
> +  /* Check for (DW_CFA_val_offset reg scaled_offset) sequence.  */
> +#define CFI_ESC_NUM_EXP 2
> +  offsetT items[CFI_ESC_NUM_EXP] = {0};
> +  while (e->next)
> +    {
> +      e = e->next;
> +      if (i >= CFI_ESC_NUM_EXP)
> +	return SFRAME_XLATE_ERR_NOTREPRESENTED;
> +      items[i] = e->exp.X_add_number;
> +      i++;
> +    }
> +  if (i <= CFI_ESC_NUM_EXP - 1)
> +    return SFRAME_XLATE_ERR_NOTREPRESENTED;

Both arguments to DW_CFA_val_offset are ULEB128. Especially with APX (on
x86) we're going to see Dwarf register numbers above 127, for the extended
GPRs. And large enough stack frames would also require multi-byte offset
representation.

> +  reg = items[0];
> +  offset = items[1];
> +#undef CFI_ESC_NUM_EXP
> +
> +  /* Invoke sframe_xlate_do_val_offset itself for checking.  */
> +  struct cfi_insn_data *temp = XCNEW (struct cfi_insn_data);
> +  temp->insn = DW_CFA_val_offset;
> +  temp->u.ri.reg = reg;
> +  temp->u.ri.offset = offset;
> +
> +  err = sframe_xlate_do_val_offset (xlate_ctx, temp, true);
> +  XDELETE (temp);

No real need to involve dynamic allocation?

  struct cfi_insn_data temp = {
    .insn = DW_CFA_val_offset,
    .u = {
      .ri = {
	.reg = reg,
	.offset = offset
      }
    }
  };

  err = sframe_xlate_do_val_offset (xlate_ctx, &temp, true);

?

> +/* Handle CFI_escape in SFrame context.
> +
> +   .cfi_escape CFI directive allows the user to add arbitrary bytes to the
> +   unwind info.  DWARF expressions commonly follow after CFI_escape (fake CFI)
> +   DWARF opcode.  One might also use CFI_escape to add OS-specific CFI opcodes
> +   even.
> +
> +   Complex unwind info added using .cfi_escape directive _may_ be of no
> +   consequence for SFrame when the affected registers are not SP, FP, RA or
> +   CFA.  The challenge in confirming the afore-mentioned is that it needs full
> +   parsing and validation of the bytes presented after .cfi_escape.  Here we
> +   take a case-by-case approach towards skipping _some_ instances of
> +   .cfi_escape: skip those that can be *easily* determined to be harmless in
> +   the context of SFrame stack trace information.
> +
> +   This function partially processes bytes following .cfi_escape and returns
> +   SFRAME_XLATE_OK if OK to skip.  */

See PR gas/32613: It won't be for long that only simple byte sequences can
appear here.

> +static int
> +sframe_xlate_do_cfi_escape (struct sframe_xlate_ctx *xlate_ctx,
> +			    struct cfi_insn_data *cfi_insn)

As you don't mean to alter *cfi_insn, please use pointer-to-const. Possibly
same for xlate_ctx.

> +{
> +  const struct cfi_escape_data *e;
> +  int err = SFRAME_XLATE_OK;
> +  offsetT firstop;
> +
> +  e = cfi_insn->u.esc;
> +
> +  if (!e)
> +    return SFRAME_XLATE_ERR_INVAL;
> +
> +  firstop = e->exp.X_add_number;

What if .X_op isn't O_constant (applies elsewhere as well of course)?

> +  switch (firstop)
> +    {
> +    case DW_CFA_nop:
> +      while (e->next)
> +	{
> +	  e = e->next;
> +	  if (e->exp.X_add_number != DW_CFA_nop)
> +	    {
> +	      err = SFRAME_XLATE_ERR_NOTREPRESENTED;
> +	      break;
> +	    }
> +	}
> +      break;

Hmm, you ignore all-NOPs sequences, but NOP followed by one of the cases
below is still left as "bad". While I'm not going to insist that you go
farther, it striks me as odd.

> +    case DW_CFA_expression:
> +      return sframe_xlate_do_escape_expr (xlate_ctx, cfi_insn);
> +
> +    case DW_CFA_val_offset:
> +      return sframe_xlate_do_escape_val_offset (xlate_ctx, cfi_insn);
> +
> +    /* FIXME - Also add processing for DW_CFA_GNU_args_size in future?  */
> +
> +    default:
> +      err = SFRAME_XLATE_ERR_NOTREPRESENTED;
> +      break;
> +    }
> +
> +  if (err == SFRAME_XLATE_ERR_NOTREPRESENTED)
> +    {
> +      /* In all other cases (e.g., DW_CFA_def_cfa_expression or other
> +	 OS-specific CFI opcodes), skip inspecting the DWARF expression.
> +	 This may impact the asynchronicity due to loss of coverage.
> +	 Continue to warn the user and bail out.  */
> +      as_warn (_("skipping SFrame FDE; .cfi_escape with op (%#lx)"),
> +	       (unsigned long)firstop);

Right now it's supposedly only bytes that you get to see here, so the
truncation (on a 32-bit host with BFD64 in use) hopefully won't matter
much.

> --- /dev/null
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-9.s
> @@ -0,0 +1,17 @@
> +## CFI_escape may be used to encode DWARF expressions among other things.
> +## Depending on the register applicable for the DWARF expression, skipping
> +## SFrame FDE may be OK: SFrame stack trace information is relevant for SP, FP
> +## and RA only.  In this test, CFI_escape is safe to skip (does not affect
> +## correctness of SFrame data).  The register 0xc is non SP / FP on both
> +## aarch64 and x86_64.
> +	.cfi_startproc
> +	.long 0
> +	.cfi_def_cfa_offset 16
> +# DW_CFA_expression for reg 0xc
> +	.cfi_escape 0x10,0xc,0x2,0x76,0x78

The comment only describes part of this expression. Since such hard-coded
byte sequences are hard to decipher, can such comments please fully describe
things?

Jan

Re: [PATCH,V2 2/2] gas: sframe: partially process DWARF expressions in CFI_escape

[Indu Bhagat]

On 2/7/25 3:22 AM, Jan Beulich wrote:
> On 05.02.2025 00:10, Indu Bhagat wrote:
>> --- a/gas/gen-sframe.c
>> +++ b/gas/gen-sframe.c
>> @@ -1121,7 +1121,8 @@ sframe_xlate_do_offset (struct sframe_xlate_ctx *xlate_ctx,
>>   
>>   static int
>>   sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>> -			    struct cfi_insn_data *cfi_insn)
>> +			    struct cfi_insn_data *cfi_insn,
>> +			    bool cfi_escape_p)
>>   {
>>     /* Previous value of register is CFA + offset.  However, if the specified
>>        register is not interesting (SP, FP, or RA reg), the current
>> @@ -1134,7 +1135,8 @@ sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>>         /* Ignore SP reg, if offset matches assumed default rule.  */
>>         || (cfi_insn->u.ri.reg == SFRAME_CFA_SP_REG && cfi_insn->u.ri.offset != 0))
>>       {
>> -      as_warn (_("skipping SFrame FDE; DW_CFA_val_offset with %s register %u"),
>> +      as_warn (_("skipping SFrame FDE; %sDW_CFA_val_offset with %s reg %u"),
>> +	       cfi_escape_p ? ".cfi_escape " : "",
>>   	       sframe_register_name (cfi_insn->u.ri.reg), cfi_insn->u.ri.reg);
> 
> Hmm - I thought patch 1 was added to eliminate the need to alter this
> here again.
> 

I took Jens suggestion.  I have merged the two patches for V3 (not sent 
to the list yet) and now have:

       /* Ignore SP reg, if offset matches assumed default rule.  */
       || (cfi_insn->u.ri.reg == SFRAME_CFA_SP_REG && 
cfi_insn->u.ri.offset != 0))
     {
       as_warn (_("skipping SFrame FDE; %s with %s reg %u"),
                cfi_esc_p ? ".cfi_escape DW_CFA_val_offset" : 
".cfi_val_offset",
                sframe_register_name (cfi_insn->u.ri.reg), 
cfi_insn->u.ri.reg);
       return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
     }

>> @@ -1310,6 +1312,173 @@ sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
>>     return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
>>   }
>>   
>> +/* Handle DW_CFA_expression in .cfi_escape.  */
>> +
>> +static int
>> +sframe_xlate_do_escape_expr (struct sframe_xlate_ctx *xlate_ctx,
>> +			     struct cfi_insn_data *cfi_insn)
>> +{
>> +  const struct cfi_escape_data *e = cfi_insn->u.esc;
>> +  const struct sframe_row_entry *cur_fre = NULL;
>> +  unsigned int reg = 0;
>> +  int err = SFRAME_XLATE_OK;
>> +  int i = 0;
> 
> unsigned int please.
> 

OK.

>> +  if (!e || !e->next)
>> +    return SFRAME_XLATE_ERR_INVAL;
> 
> Why the check for e being NULL? The caller already de-referenced it. And
> the e->next check is redundant with the while() below.
> 

Just wanted to avoid unnecessary bugs in future when caller APIs evolve 
etc.  BTW, the compiled code (-O2) contains most of these functions 
inlined, so its not a concern.

For now, I have removed the complete check though.  We have tests for 
this code path, so my original concern was not totally necessary.

>> +  /* Check roughly for expression of the kind
>> +     DW_CFA_expression: r1 (rdx) (DW_OP_bregN (reg): XXX) */
>> +#define CFI_ESC_NUM_EXP 4
>> +  offsetT items[CFI_ESC_NUM_EXP] = {0};
>> +  while (e->next)
>> +    {
>> +      e = e->next;
>> +      if (i >= CFI_ESC_NUM_EXP)
>> +	return SFRAME_XLATE_ERR_NOTREPRESENTED;
>> +      items[i] = e->exp.X_add_number;
>> +      i++;
>> +    }
>> +
>> +  if (i <= CFI_ESC_NUM_EXP - 1)
>> +    return SFRAME_XLATE_ERR_NOTREPRESENTED;
> 
> See the respective comment on sframe_xlate_do_escape_val_offset() below.
> 
>> +  cur_fre = xlate_ctx->cur_fre;
>> +  reg = items[0];
>> +#undef CFI_ESC_NUM_EXP
> 
> You fetch 4 bytes, then use only the first? Shouldn't you at least check
> expression length and DW_OP_breg<N> (and hence the 2nd register number)
> as well? What about DW_OP_reg<N>, DW_OP_bregx, and DW_OP_regx?
> 

Re:check expression length - We do check for expression length; For 
length >= CFI_ESC_NUM_EXP or length <= CFI_ESC_NUM_EXP - 1, we return 
SFRAME_XLATE_ERR_NOTREPRESENTED.

Once the check for target register is done, what follows after is 
assumed to of no consequence in terms of how it affects SFrame stack 
trace data.  When GAS is in these APIs working out the SFrame 
generation, validating .cfi_escape data is not intended.

This patch is about failing less often: do not skip generating FDE if 
the complex expressions involve a register of no interest.

>> +/* Handle DW_CFA_val_offset in .cfi_escape.  */
>> +
>> +static int
>> +sframe_xlate_do_escape_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
> 
> Why ATTRIBUTE_UNUSED? The parameter is used at the bottom of the function.
> 

Forgot to update from a previous iteration.  I have updated it now.

>> +				   struct cfi_insn_data *cfi_insn)
>> +{
>> +  const struct cfi_escape_data *e = cfi_insn->u.esc;
>> +  unsigned int reg = 0;
>> +  offsetT offset = 0;
>> +  int err = SFRAME_XLATE_OK;
>> +  int i = 0;
>> +
>> +  if (!e || !e->next)
>> +    return SFRAME_XLATE_ERR_INVAL;
>> +
>> +  /* Check for (DW_CFA_val_offset reg scaled_offset) sequence.  */
>> +#define CFI_ESC_NUM_EXP 2
>> +  offsetT items[CFI_ESC_NUM_EXP] = {0};
>> +  while (e->next)
>> +    {
>> +      e = e->next;
>> +      if (i >= CFI_ESC_NUM_EXP)
>> +	return SFRAME_XLATE_ERR_NOTREPRESENTED;
>> +      items[i] = e->exp.X_add_number;
>> +      i++;
>> +    }
>> +  if (i <= CFI_ESC_NUM_EXP - 1)
>> +    return SFRAME_XLATE_ERR_NOTREPRESENTED;
> 
> Both arguments to DW_CFA_val_offset are ULEB128. Especially with APX (on
> x86) we're going to see Dwarf register numbers above 127, for the extended
> GPRs. And large enough stack frames would also require multi-byte offset
> representation.
> 

In this case, e.g., we are specifically checking for an expression with 
opcode = DW_CFA_val_offset and _two_ operands.  Hence, for APX (on x86) 
we will fall into the case of (i >= CFI_ESC_NUM_EXP) and return 
SFRAME_XLATE_ERR_NOTREPRESENTED.

Although not ideal (because ideally we do not want to skip FDE 
generation due to APX register in DW_CFA_val_offset), it is not leading 
to incorrect SFrame FDE data.

The intention is to only partially process some simple expressions, when 
feasible.

>> +  reg = items[0];
>> +  offset = items[1];
>> +#undef CFI_ESC_NUM_EXP
>> +
>> +  /* Invoke sframe_xlate_do_val_offset itself for checking.  */
>> +  struct cfi_insn_data *temp = XCNEW (struct cfi_insn_data);
>> +  temp->insn = DW_CFA_val_offset;
>> +  temp->u.ri.reg = reg;
>> +  temp->u.ri.offset = offset;
>> +
>> +  err = sframe_xlate_do_val_offset (xlate_ctx, temp, true);
>> +  XDELETE (temp);
> 
> No real need to involve dynamic allocation?
> 
>    struct cfi_insn_data temp = {
>      .insn = DW_CFA_val_offset,
>      .u = {
>        .ri = {
> 	.reg = reg,
> 	.offset = offset
>        }
>      }
>    };
> 
>    err = sframe_xlate_do_val_offset (xlate_ctx, &temp, true);
> 
> ?
> 

OK.  Although I prefer the other style, so I now have:

   /* Invoke sframe_xlate_do_val_offset itself for checking.  */
   struct cfi_insn_data temp;
   temp.insn = DW_CFA_val_offset;
   temp.u.ri.reg = reg;
   /* Skip undoing the scaling with DWARF2_CIE_DATA_ALIGNMENT.  offset is
      non-uleb anyway.  */
   temp.u.ri.offset = offset;
   err = sframe_xlate_do_val_offset (xlate_ctx, &temp, true);

>> +/* Handle CFI_escape in SFrame context.
>> +
>> +   .cfi_escape CFI directive allows the user to add arbitrary bytes to the
>> +   unwind info.  DWARF expressions commonly follow after CFI_escape (fake CFI)
>> +   DWARF opcode.  One might also use CFI_escape to add OS-specific CFI opcodes
>> +   even.
>> +
>> +   Complex unwind info added using .cfi_escape directive _may_ be of no
>> +   consequence for SFrame when the affected registers are not SP, FP, RA or
>> +   CFA.  The challenge in confirming the afore-mentioned is that it needs full
>> +   parsing and validation of the bytes presented after .cfi_escape.  Here we
>> +   take a case-by-case approach towards skipping _some_ instances of
>> +   .cfi_escape: skip those that can be *easily* determined to be harmless in
>> +   the context of SFrame stack trace information.
>> +
>> +   This function partially processes bytes following .cfi_escape and returns
>> +   SFRAME_XLATE_OK if OK to skip.  */
> 
> See PR gas/32613: It won't be for long that only simple byte sequences can
> appear here.
> 
>> +static int
>> +sframe_xlate_do_cfi_escape (struct sframe_xlate_ctx *xlate_ctx,
>> +			    struct cfi_insn_data *cfi_insn)
> 
> As you don't mean to alter *cfi_insn, please use pointer-to-const. Possibly
> same for xlate_ctx.
> 

Yes, I would like do that in a separate commit later for all functions 
here.  But prior to that, I need to get some bugfixes addressed which 
are higher in my priority queue.

>> +{
>> +  const struct cfi_escape_data *e;
>> +  int err = SFRAME_XLATE_OK;
>> +  offsetT firstop;
>> +
>> +  e = cfi_insn->u.esc;
>> +
>> +  if (!e)
>> +    return SFRAME_XLATE_ERR_INVAL;
>> +
>> +  firstop = e->exp.X_add_number;
> 
> What if .X_op isn't O_constant (applies elsewhere as well of course)?
> 

Hmm, I should check explicitly and exit early.  I have added that.

>> +  switch (firstop)
>> +    {
>> +    case DW_CFA_nop:
>> +      while (e->next)
>> +	{
>> +	  e = e->next;
>> +	  if (e->exp.X_add_number != DW_CFA_nop)
>> +	    {
>> +	      err = SFRAME_XLATE_ERR_NOTREPRESENTED;
>> +	      break;
>> +	    }
>> +	}
>> +      break;
> 
> Hmm, you ignore all-NOPs sequences, but NOP followed by one of the cases
> below is still left as "bad". While I'm not going to insist that you go
> farther, it striks me as odd.
> 
>> +    case DW_CFA_expression:
>> +      return sframe_xlate_do_escape_expr (xlate_ctx, cfi_insn);
>> +
>> +    case DW_CFA_val_offset:
>> +      return sframe_xlate_do_escape_val_offset (xlate_ctx, cfi_insn);
>> +
>> +    /* FIXME - Also add processing for DW_CFA_GNU_args_size in future?  */
>> +
>> +    default:
>> +      err = SFRAME_XLATE_ERR_NOTREPRESENTED;
>> +      break;
>> +    }
>> +
>> +  if (err == SFRAME_XLATE_ERR_NOTREPRESENTED)
>> +    {
>> +      /* In all other cases (e.g., DW_CFA_def_cfa_expression or other
>> +	 OS-specific CFI opcodes), skip inspecting the DWARF expression.
>> +	 This may impact the asynchronicity due to loss of coverage.
>> +	 Continue to warn the user and bail out.  */
>> +      as_warn (_("skipping SFrame FDE; .cfi_escape with op (%#lx)"),
>> +	       (unsigned long)firstop);
> 
> Right now it's supposedly only bytes that you get to see here, so the
> truncation (on a 32-bit host with BFD64 in use) hopefully won't matter
> much.
> 
>> --- /dev/null
>> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-9.s
>> @@ -0,0 +1,17 @@
>> +## CFI_escape may be used to encode DWARF expressions among other things.
>> +## Depending on the register applicable for the DWARF expression, skipping
>> +## SFrame FDE may be OK: SFrame stack trace information is relevant for SP, FP
>> +## and RA only.  In this test, CFI_escape is safe to skip (does not affect
>> +## correctness of SFrame data).  The register 0xc is non SP / FP on both
>> +## aarch64 and x86_64.
>> +	.cfi_startproc
>> +	.long 0
>> +	.cfi_def_cfa_offset 16
>> +# DW_CFA_expression for reg 0xc
>> +	.cfi_escape 0x10,0xc,0x2,0x76,0x78
> 
> The comment only describes part of this expression. Since such hard-coded
> byte sequences are hard to decipher, can such comments please fully describe
> things?
> 

OK. Added comments.

# DW_CFA_expression,reg 0xc,length 2,DW_OP_breg6 (rbp),ULEB(-8)
         .cfi_escape 0x10,0xc,0x2,0x76,0x78
# DW_CFA_nop
         .cfi_escape 0x0
         .cfi_escape 0x0,0x0,0x0,0x0
# DW_CFA_val_offset,reg 0xc,ULEB scaled offset for 32
         .cfi_escape 0x14,0xc,0x4

Re: [PATCH,V2 2/2] gas: sframe: partially process DWARF expressions in CFI_escape

[Jan Beulich]

On 10.02.2025 07:06, Indu Bhagat wrote:
> On 2/7/25 3:22 AM, Jan Beulich wrote:
>> On 05.02.2025 00:10, Indu Bhagat wrote:
>>> +  /* Check roughly for expression of the kind
>>> +     DW_CFA_expression: r1 (rdx) (DW_OP_bregN (reg): XXX) */
>>> +#define CFI_ESC_NUM_EXP 4
>>> +  offsetT items[CFI_ESC_NUM_EXP] = {0};
>>> +  while (e->next)
>>> +    {
>>> +      e = e->next;
>>> +      if (i >= CFI_ESC_NUM_EXP)
>>> +	return SFRAME_XLATE_ERR_NOTREPRESENTED;
>>> +      items[i] = e->exp.X_add_number;
>>> +      i++;
>>> +    }
>>> +
>>> +  if (i <= CFI_ESC_NUM_EXP - 1)
>>> +    return SFRAME_XLATE_ERR_NOTREPRESENTED;
>>
>> See the respective comment on sframe_xlate_do_escape_val_offset() below.
>>
>>> +  cur_fre = xlate_ctx->cur_fre;
>>> +  reg = items[0];
>>> +#undef CFI_ESC_NUM_EXP
>>
>> You fetch 4 bytes, then use only the first? Shouldn't you at least check
>> expression length and DW_OP_breg<N> (and hence the 2nd register number)
>> as well? What about DW_OP_reg<N>, DW_OP_bregx, and DW_OP_regx?
>>
> 
> Re:check expression length - We do check for expression length; For 
> length >= CFI_ESC_NUM_EXP or length <= CFI_ESC_NUM_EXP - 1, we return 
> SFRAME_XLATE_ERR_NOTREPRESENTED.

That's not what I mean though. The length is encoded in the stream of
bytes (items[1] iirc), and you don't check that at all. Imo you want to
reject anything that isn't valid in the first place, i.e. you probably
shouldn't even look past this encoded length if it is, say, zero.

> Once the check for target register is done, what follows after is 
> assumed to of no consequence in terms of how it affects SFrame stack 
> trace data.  When GAS is in these APIs working out the SFrame 
> generation, validating .cfi_escape data is not intended.

Yet then - why fetch the extra bytes?

>>> +/* Handle DW_CFA_val_offset in .cfi_escape.  */
>>> +
>>> +static int
>>> +sframe_xlate_do_escape_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>>
>> Why ATTRIBUTE_UNUSED? The parameter is used at the bottom of the function.
>>
> 
> Forgot to update from a previous iteration.  I have updated it now.
> 
>>> +				   struct cfi_insn_data *cfi_insn)
>>> +{
>>> +  const struct cfi_escape_data *e = cfi_insn->u.esc;
>>> +  unsigned int reg = 0;
>>> +  offsetT offset = 0;
>>> +  int err = SFRAME_XLATE_OK;
>>> +  int i = 0;
>>> +
>>> +  if (!e || !e->next)
>>> +    return SFRAME_XLATE_ERR_INVAL;
>>> +
>>> +  /* Check for (DW_CFA_val_offset reg scaled_offset) sequence.  */
>>> +#define CFI_ESC_NUM_EXP 2
>>> +  offsetT items[CFI_ESC_NUM_EXP] = {0};
>>> +  while (e->next)
>>> +    {
>>> +      e = e->next;
>>> +      if (i >= CFI_ESC_NUM_EXP)
>>> +	return SFRAME_XLATE_ERR_NOTREPRESENTED;
>>> +      items[i] = e->exp.X_add_number;
>>> +      i++;
>>> +    }
>>> +  if (i <= CFI_ESC_NUM_EXP - 1)
>>> +    return SFRAME_XLATE_ERR_NOTREPRESENTED;
>>
>> Both arguments to DW_CFA_val_offset are ULEB128. Especially with APX (on
>> x86) we're going to see Dwarf register numbers above 127, for the extended
>> GPRs. And large enough stack frames would also require multi-byte offset
>> representation.
>>
> 
> In this case, e.g., we are specifically checking for an expression with 
> opcode = DW_CFA_val_offset and _two_ operands.  Hence, for APX (on x86) 
> we will fall into the case of (i >= CFI_ESC_NUM_EXP) and return 
> SFRAME_XLATE_ERR_NOTREPRESENTED.
> 
> Although not ideal (because ideally we do not want to skip FDE 
> generation due to APX register in DW_CFA_val_offset), it is not leading 
> to incorrect SFrame FDE data.
> 
> The intention is to only partially process some simple expressions, when 
> feasible.

Can comments then please be extended to mention all intentional limitations
(for readers to be able to tell those from behavior that wasn't intended to
be the way it is, and hence may want corrrecting)?

>>> +  reg = items[0];
>>> +  offset = items[1];
>>> +#undef CFI_ESC_NUM_EXP
>>> +
>>> +  /* Invoke sframe_xlate_do_val_offset itself for checking.  */
>>> +  struct cfi_insn_data *temp = XCNEW (struct cfi_insn_data);
>>> +  temp->insn = DW_CFA_val_offset;
>>> +  temp->u.ri.reg = reg;
>>> +  temp->u.ri.offset = offset;
>>> +
>>> +  err = sframe_xlate_do_val_offset (xlate_ctx, temp, true);
>>> +  XDELETE (temp);
>>
>> No real need to involve dynamic allocation?
>>
>>    struct cfi_insn_data temp = {
>>      .insn = DW_CFA_val_offset,
>>      .u = {
>>        .ri = {
>> 	.reg = reg,
>> 	.offset = offset
>>        }
>>      }
>>    };
>>
>>    err = sframe_xlate_do_val_offset (xlate_ctx, &temp, true);
>>
>> ?
>>
> 
> OK.  Although I prefer the other style, so I now have:
> 
>    /* Invoke sframe_xlate_do_val_offset itself for checking.  */
>    struct cfi_insn_data temp;
>    temp.insn = DW_CFA_val_offset;
>    temp.u.ri.reg = reg;
>    /* Skip undoing the scaling with DWARF2_CIE_DATA_ALIGNMENT.  offset is
>       non-uleb anyway.  */
>    temp.u.ri.offset = offset;
>    err = sframe_xlate_do_val_offset (xlate_ctx, &temp, true);

Please have the variable have an initializer. I specifically wrote it that way;
if you don't like the extensive one, use e.g. just

struct cfi_insn_data temp = { .insn = DW_CFA_val_offset };

This way you'll make sure to initialize the full struct, including unused parts
(due to [perhaps future] union members). That'll save us from possibly
surprising compiler diagnostics about accessing uninitialized fields (it was
certainly more than once that newly added warnings were issued in wider a
manner than would have been desirable).

>>> +/* Handle CFI_escape in SFrame context.
>>> +
>>> +   .cfi_escape CFI directive allows the user to add arbitrary bytes to the
>>> +   unwind info.  DWARF expressions commonly follow after CFI_escape (fake CFI)
>>> +   DWARF opcode.  One might also use CFI_escape to add OS-specific CFI opcodes
>>> +   even.
>>> +
>>> +   Complex unwind info added using .cfi_escape directive _may_ be of no
>>> +   consequence for SFrame when the affected registers are not SP, FP, RA or
>>> +   CFA.  The challenge in confirming the afore-mentioned is that it needs full
>>> +   parsing and validation of the bytes presented after .cfi_escape.  Here we
>>> +   take a case-by-case approach towards skipping _some_ instances of
>>> +   .cfi_escape: skip those that can be *easily* determined to be harmless in
>>> +   the context of SFrame stack trace information.
>>> +
>>> +   This function partially processes bytes following .cfi_escape and returns
>>> +   SFRAME_XLATE_OK if OK to skip.  */
>>
>> See PR gas/32613: It won't be for long that only simple byte sequences can
>> appear here.
>>
>>> +static int
>>> +sframe_xlate_do_cfi_escape (struct sframe_xlate_ctx *xlate_ctx,
>>> +			    struct cfi_insn_data *cfi_insn)
>>
>> As you don't mean to alter *cfi_insn, please use pointer-to-const. Possibly
>> same for xlate_ctx.
>>
> 
> Yes, I would like do that in a separate commit later for all functions 
> here.  But prior to that, I need to get some bugfixes addressed which 
> are higher in my priority queue.

What's getting in the way of doing it here right away?

>>> --- /dev/null
>>> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-9.s
>>> @@ -0,0 +1,17 @@
>>> +## CFI_escape may be used to encode DWARF expressions among other things.
>>> +## Depending on the register applicable for the DWARF expression, skipping
>>> +## SFrame FDE may be OK: SFrame stack trace information is relevant for SP, FP
>>> +## and RA only.  In this test, CFI_escape is safe to skip (does not affect
>>> +## correctness of SFrame data).  The register 0xc is non SP / FP on both
>>> +## aarch64 and x86_64.
>>> +	.cfi_startproc
>>> +	.long 0
>>> +	.cfi_def_cfa_offset 16
>>> +# DW_CFA_expression for reg 0xc
>>> +	.cfi_escape 0x10,0xc,0x2,0x76,0x78
>>
>> The comment only describes part of this expression. Since such hard-coded
>> byte sequences are hard to decipher, can such comments please fully describe
>> things?
> 
> OK. Added comments.
> 
> # DW_CFA_expression,reg 0xc,length 2,DW_OP_breg6 (rbp),ULEB(-8)

This being an arch-independent check, may I ask that you don't mention x86
register names here?

What's ULEB(-8)? 'U' standing for "unsigned" doesn't seem to fit with a
negative number.

Jan

>          .cfi_escape 0x10,0xc,0x2,0x76,0x78
> # DW_CFA_nop
>          .cfi_escape 0x0
>          .cfi_escape 0x0,0x0,0x0,0x0
> # DW_CFA_val_offset,reg 0xc,ULEB scaled offset for 32
>          .cfi_escape 0x14,0xc,0x4
> 
>