[PATCH] gas: sframe: partially process DWARF expressions in CFI_escape

[PATCH] gas: sframe: partially process DWARF expressions in CFI_escape

[Indu Bhagat]

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 non SP/FP registers
  - DW_CFA_value_expression affecting non SP/FP registers.

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_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/
        * dw2gencfi.c (struct cfi_escape_data): Move from ...
        * dw2gencfi.h (struct cfi_escape_data): ... to.
        * gen-sframe.c (sframe_xlate_do_expr): New definition.
        (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.
---
Testing notes:
 - Regression tested on a set of x86_64* and aarch64* targets.
 - In a follow up patch, we will target some more of "harmless"
   instances of the currently seen warnings in some packages:
      skipping SFrame FDE; .cfi_escape with op (0x2e)
   (DW_CFA_GNU_args_size = 0x2e).

 gas/dw2gencfi.c                               |  6 --
 gas/dw2gencfi.h                               |  6 ++
 gas/gen-sframe.c                              | 77 ++++++++++++++++++-
 .../gas/cfi-sframe/cfi-sframe-common-9.d      | 22 ++++++
 .../gas/cfi-sframe/cfi-sframe-common-9.s      | 12 +++
 .../cfi-sframe/cfi-sframe-x86_64-empty-1.d    | 17 ++++
 .../cfi-sframe/cfi-sframe-x86_64-empty-1.s    | 10 +++
 gas/testsuite/gas/cfi-sframe/cfi-sframe.exp   |  2 +
 8 files changed, 144 insertions(+), 8 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

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 85d2f03a55c..05ecc60adda 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -1310,6 +1310,77 @@ sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
   return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
 }
 
+/* Translate CFI_escape into 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.
+
+   In SFrame stack trace format, complex unwind info cannot be represented.  In
+   such cases, SFrame FDE generation is skipped and the user is warned.  Recall,
+   however, that SFrame stack trace information is meant to convey information
+   about SP, FP and RA only.  Hence, some DWARF expressions, are indeed safe to
+   skip.
+
+   This function partially processes some DWARF expresssions and returns
+   SFRAME_XLATE_OK if OK to skip.  */
+
+static int
+sframe_xlate_do_expr (struct sframe_xlate_ctx *xlate_ctx,
+		      struct cfi_insn_data *cfi_insn)
+{
+  int op;
+  struct cfi_escape_data *e;
+  unsigned int reg = 0;
+  int err = SFRAME_XLATE_OK;
+  struct sframe_row_entry *cur_fre = NULL;
+
+  e = cfi_insn->u.esc;
+
+  if (e)
+    {
+      op = e->exp.X_add_number;
+      switch (op)
+	{
+	  /* Of all the possible opcodes expected here, it is safe to
+	     ignore DW_CFA_expression and DW_CFA_val_expression, provided they
+	     do not impact the SP / FP register.  */
+	case DW_CFA_expression:
+	case DW_CFA_val_expression:
+	  /* Both DW_CFA_expression and DW_CFA_val_expression instructions take
+	     two operands: an unsigned LEB128 value representing a register
+	     number, and a DW_FORM_block value representing a DWARF expression.
+	     For the current purpose, we simply need to know the register
+	     number.  */
+	  e = e->next;
+	  /* Keep in sync with the behaviour of cfi_parse_reg ().  */
+	  gas_assert (e->exp.X_op == O_register || e->exp.X_op == O_constant);
+	  reg = e->exp.X_add_number;
+	  /* Get the scratchpad FRE.  */
+	  cur_fre = xlate_ctx->cur_fre;
+	  if (reg == SFRAME_CFA_SP_REG || reg == SFRAME_CFA_FP_REG
+	      || reg == cur_fre->cfa_base_reg)
+	    {
+	      as_warn (_("skipping SFrame FDE; .cfi_escape with reg (%#x)"), reg);
+	      err = SFRAME_XLATE_ERR_NOTREPRESENTED;
+	    }
+	  break;
+	/* FIXME - Also add processing for DW_CFA_GNU_args_size in future?  */
+	default:
+	  /* 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 (%#x)"), op);
+	  err = SFRAME_XLATE_ERR_NOTREPRESENTED;
+	  break;
+	}
+    }
+
+  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.  */
 
@@ -1406,6 +1477,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_expr (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 +1487,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..743e774a2b2
--- /dev/null
+++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-9.s
@@ -0,0 +1,12 @@
+## 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
+	.cfi_escape 0x10,0xc,0x2,0x76,0x78
+	.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..aa21d52a970
--- /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 with reg \(0x7\)
+#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.exp b/gas/testsuite/gas/cfi-sframe/cfi-sframe.exp
index b119b9da73d..803ae1c6cf0 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,7 @@ 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"
 	set ASFLAGS "$old_ASFLAGS"
     }
 }
-- 
2.43.0

Re: [PATCH] gas: sframe: partially process DWARF expressions in CFI_escape

[Jan Beulich]

On 28.01.2025 01:57, Indu Bhagat wrote:
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -1310,6 +1310,77 @@ sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
>    return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
>  }
>  
> +/* Translate CFI_escape into 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.
> +
> +   In SFrame stack trace format, complex unwind info cannot be represented.  In
> +   such cases, SFrame FDE generation is skipped and the user is warned.  Recall,
> +   however, that SFrame stack trace information is meant to convey information
> +   about SP, FP and RA only.  Hence, some DWARF expressions, are indeed safe to
> +   skip.
> +
> +   This function partially processes some DWARF expresssions and returns
> +   SFRAME_XLATE_OK if OK to skip.  */
> +
> +static int
> +sframe_xlate_do_expr (struct sframe_xlate_ctx *xlate_ctx,
> +		      struct cfi_insn_data *cfi_insn)

Both pointer-to-const?

> +{
> +  int op;
> +  struct cfi_escape_data *e;

Again.

> +  unsigned int reg = 0;
> +  int err = SFRAME_XLATE_OK;
> +  struct sframe_row_entry *cur_fre = NULL;

And yet gain.

Some of the variables may also better go into the more narrow scope, ...

> +  e = cfi_insn->u.esc;
> +
> +  if (e)

... or you may want to invert the condition here:

  if (!e)
    return ...;

reducing indentation for the bulk of the function (helping readability).

> +    {
> +      op = e->exp.X_add_number;

You're truncating the value here, and ...

> +      switch (op)
> +	{
> +	  /* Of all the possible opcodes expected here, it is safe to
> +	     ignore DW_CFA_expression and DW_CFA_val_expression, provided they
> +	     do not impact the SP / FP register.  */
> +	case DW_CFA_expression:
> +	case DW_CFA_val_expression:
> +	  /* Both DW_CFA_expression and DW_CFA_val_expression instructions take
> +	     two operands: an unsigned LEB128 value representing a register
> +	     number, and a DW_FORM_block value representing a DWARF expression.
> +	     For the current purpose, we simply need to know the register
> +	     number.  */
> +	  e = e->next;
> +	  /* Keep in sync with the behaviour of cfi_parse_reg ().  */
> +	  gas_assert (e->exp.X_op == O_register || e->exp.X_op == O_constant);

... you're asserting on user input here. Afaics neither is in any way
enforced by dot_cfi_escape() / do_parse_cons_expression(). As to the
comment - how does cfi_parse_reg() come into play for .cfi_escape?

> +	  reg = e->exp.X_add_number;
> +	  /* Get the scratchpad FRE.  */
> +	  cur_fre = xlate_ctx->cur_fre;
> +	  if (reg == SFRAME_CFA_SP_REG || reg == SFRAME_CFA_FP_REG
> +	      || reg == cur_fre->cfa_base_reg)
> +	    {
> +	      as_warn (_("skipping SFrame FDE; .cfi_escape with reg (%#x)"), reg);
> +	      err = SFRAME_XLATE_ERR_NOTREPRESENTED;
> +	    }
> +	  break;
> +	/* FIXME - Also add processing for DW_CFA_GNU_args_size in future?  */
> +	default:
> +	  /* 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 (%#x)"), op);
> +	  err = SFRAME_XLATE_ERR_NOTREPRESENTED;
> +	  break;
> +	}
> +    }

Doesn't all of this need to go in a loop, as a single .cfi_escape might
specify multiple successive DW_CFA_*?

Jan

Re: [PATCH] gas: sframe: partially process DWARF expressions in CFI_escape

[Indu Bhagat]

On 1/28/25 12:03 AM, Jan Beulich wrote:
> On 28.01.2025 01:57, Indu Bhagat wrote:
>> --- a/gas/gen-sframe.c
>> +++ b/gas/gen-sframe.c
>> @@ -1310,6 +1310,77 @@ sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
>>     return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
>>   }
>>   
>> +/* Translate CFI_escape into 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.
>> +
>> +   In SFrame stack trace format, complex unwind info cannot be represented.  In
>> +   such cases, SFrame FDE generation is skipped and the user is warned.  Recall,
>> +   however, that SFrame stack trace information is meant to convey information
>> +   about SP, FP and RA only.  Hence, some DWARF expressions, are indeed safe to
>> +   skip.
>> +
>> +   This function partially processes some DWARF expresssions and returns
>> +   SFRAME_XLATE_OK if OK to skip.  */
>> +
>> +static int
>> +sframe_xlate_do_expr (struct sframe_xlate_ctx *xlate_ctx,
>> +		      struct cfi_insn_data *cfi_insn)
> 
> Both pointer-to-const?
> 

Hmm, the second argument "struct cfi_insn_data *cfi_insn" in all APIs in 
this file can be made const I think. I should do that in a separate 
commit though.  Thanks.

For the first argument, I am not sure yet: I do plan to get to handling 
DW_CFA_GNU_args_size in near future.

>> +{
>> +  int op;
>> +  struct cfi_escape_data *e;
> 
> Again.
> 

OK. Fixed in V2.

>> +  unsigned int reg = 0;
>> +  int err = SFRAME_XLATE_OK;
>> +  struct sframe_row_entry *cur_fre = NULL;
> 
> And yet gain.
> 
> Some of the variables may also better go into the more narrow scope, ...
> 

Sure.

>> +  e = cfi_insn->u.esc;
>> +
>> +  if (e)
> 
> ... or you may want to invert the condition here:
> 
>    if (!e)
>      return ...;
> 
> reducing indentation for the bulk of the function (helping readability).
> 

Fixed in V2.

>> +    {
>> +      op = e->exp.X_add_number;
> 
> You're truncating the value here, and ...
> 

Ah, Thanks. Changed the int op to offsetT op.

>> +      switch (op)
>> +	{
>> +	  /* Of all the possible opcodes expected here, it is safe to
>> +	     ignore DW_CFA_expression and DW_CFA_val_expression, provided they
>> +	     do not impact the SP / FP register.  */
>> +	case DW_CFA_expression:
>> +	case DW_CFA_val_expression:
>> +	  /* Both DW_CFA_expression and DW_CFA_val_expression instructions take
>> +	     two operands: an unsigned LEB128 value representing a register
>> +	     number, and a DW_FORM_block value representing a DWARF expression.
>> +	     For the current purpose, we simply need to know the register
>> +	     number.  */
>> +	  e = e->next;
>> +	  /* Keep in sync with the behaviour of cfi_parse_reg ().  */
>> +	  gas_assert (e->exp.X_op == O_register || e->exp.X_op == O_constant);
> 
> ... you're asserting on user input here. Afaics neither is in any way
> enforced by dot_cfi_escape() / do_parse_cons_expression(). As to the
> comment - how does cfi_parse_reg() come into play for .cfi_escape?
> 

IIUC, the first operand of both of these opcodes (DW_CFA_expression, 
DW_CFA_val_expression) will be a register.  Hence, cfi_parse_reg () 
behaviour is being cross-checked against.

This function only aims to process some "simple DWARF expressions", and 
if they are benign (OK to skip), we do not warn (nor error out with 
SFRAME_XLATE_ERR_NOTREPRESENTED).  E.g. for,

DW_CFA_expression: r1 (rdx) (DW_OP_breg7 (rsp): 136)
DW_CFA_expression: r0 (rax) (DW_OP_breg7 (rsp): 144)

>> +	  reg = e->exp.X_add_number;
>> +	  /* Get the scratchpad FRE.  */
>> +	  cur_fre = xlate_ctx->cur_fre;
>> +	  if (reg == SFRAME_CFA_SP_REG || reg == SFRAME_CFA_FP_REG
>> +	      || reg == cur_fre->cfa_base_reg)
>> +	    {
>> +	      as_warn (_("skipping SFrame FDE; .cfi_escape with reg (%#x)"), reg);
>> +	      err = SFRAME_XLATE_ERR_NOTREPRESENTED;
>> +	    }
>> +	  break;
>> +	/* FIXME - Also add processing for DW_CFA_GNU_args_size in future?  */
>> +	default:
>> +	  /* 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 (%#x)"), op);
>> +	  err = SFRAME_XLATE_ERR_NOTREPRESENTED;
>> +	  break;
>> +	}
>> +    }
> 
> Doesn't all of this need to go in a loop, as a single .cfi_escape might
> specify multiple successive DW_CFA_*?
> 

The function intends to process only some DWARF expressions (basically 
some commonly-occurring, but safe to skip from SFrame perspective). 
Going into a loop may be necessary for opcodes other than 
DW_CFA_expression or DW_CFA_val_expression, but at the moment, since we 
prefer to cater to only the simple expressions, parsing the complete 
expression is not required.

Thanks

Re: [PATCH] gas: sframe: partially process DWARF expressions in CFI_escape

[Jan Beulich]

On 28.01.2025 16:24, Indu Bhagat wrote:
> On 1/28/25 12:03 AM, Jan Beulich wrote:
>> On 28.01.2025 01:57, Indu Bhagat wrote:
>>> +      switch (op)
>>> +	{
>>> +	  /* Of all the possible opcodes expected here, it is safe to
>>> +	     ignore DW_CFA_expression and DW_CFA_val_expression, provided they
>>> +	     do not impact the SP / FP register.  */
>>> +	case DW_CFA_expression:
>>> +	case DW_CFA_val_expression:
>>> +	  /* Both DW_CFA_expression and DW_CFA_val_expression instructions take
>>> +	     two operands: an unsigned LEB128 value representing a register
>>> +	     number, and a DW_FORM_block value representing a DWARF expression.
>>> +	     For the current purpose, we simply need to know the register
>>> +	     number.  */
>>> +	  e = e->next;
>>> +	  /* Keep in sync with the behaviour of cfi_parse_reg ().  */
>>> +	  gas_assert (e->exp.X_op == O_register || e->exp.X_op == O_constant);
>>
>> ... you're asserting on user input here. Afaics neither is in any way
>> enforced by dot_cfi_escape() / do_parse_cons_expression(). As to the
>> comment - how does cfi_parse_reg() come into play for .cfi_escape?
> 
> IIUC, the first operand of both of these opcodes (DW_CFA_expression, 
> DW_CFA_val_expression) will be a register.  Hence, cfi_parse_reg () 
> behaviour is being cross-checked against.
> 
> This function only aims to process some "simple DWARF expressions", and 
> if they are benign (OK to skip), we do not warn (nor error out with 
> SFRAME_XLATE_ERR_NOTREPRESENTED).  E.g. for,
> 
> DW_CFA_expression: r1 (rdx) (DW_OP_breg7 (rsp): 136)
> DW_CFA_expression: r0 (rax) (DW_OP_breg7 (rsp): 144)

Yet .cfi_escape accepts all sorts of expressions. It's okay to tell the
user that for SFrame this isn't supported. But it's not okay to hide
this information behind an internal error (that the failed assertion
will result in).

>>> +	  reg = e->exp.X_add_number;
>>> +	  /* Get the scratchpad FRE.  */
>>> +	  cur_fre = xlate_ctx->cur_fre;
>>> +	  if (reg == SFRAME_CFA_SP_REG || reg == SFRAME_CFA_FP_REG
>>> +	      || reg == cur_fre->cfa_base_reg)
>>> +	    {
>>> +	      as_warn (_("skipping SFrame FDE; .cfi_escape with reg (%#x)"), reg);
>>> +	      err = SFRAME_XLATE_ERR_NOTREPRESENTED;
>>> +	    }
>>> +	  break;
>>> +	/* FIXME - Also add processing for DW_CFA_GNU_args_size in future?  */
>>> +	default:
>>> +	  /* 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 (%#x)"), op);
>>> +	  err = SFRAME_XLATE_ERR_NOTREPRESENTED;
>>> +	  break;
>>> +	}
>>> +    }
>>
>> Doesn't all of this need to go in a loop, as a single .cfi_escape might
>> specify multiple successive DW_CFA_*?
> 
> The function intends to process only some DWARF expressions (basically 
> some commonly-occurring, but safe to skip from SFrame perspective). 
> Going into a loop may be necessary for opcodes other than 
> DW_CFA_expression or DW_CFA_val_expression, but at the moment, since we 
> prefer to cater to only the simple expressions, parsing the complete 
> expression is not required.

No, wait. Simple expressions or not isn't the point here. The point is
that with a single .cfi_escape you can specify any number of things,
i.e. also multiple operations involving multiple expressions. There's
no limit to the number of operands to .cfi_escape afaics - it could
literally be hundreds.

Jan

Re: [PATCH] gas: sframe: partially process DWARF expressions in CFI_escape

[Indu Bhagat]

On 1/28/25 7:31 AM, Jan Beulich wrote:
> On 28.01.2025 16:24, Indu Bhagat wrote:
>> On 1/28/25 12:03 AM, Jan Beulich wrote:
>>> On 28.01.2025 01:57, Indu Bhagat wrote:
>>>> +      switch (op)
>>>> +	{
>>>> +	  /* Of all the possible opcodes expected here, it is safe to
>>>> +	     ignore DW_CFA_expression and DW_CFA_val_expression, provided they
>>>> +	     do not impact the SP / FP register.  */
>>>> +	case DW_CFA_expression:
>>>> +	case DW_CFA_val_expression:
>>>> +	  /* Both DW_CFA_expression and DW_CFA_val_expression instructions take
>>>> +	     two operands: an unsigned LEB128 value representing a register
>>>> +	     number, and a DW_FORM_block value representing a DWARF expression.
>>>> +	     For the current purpose, we simply need to know the register
>>>> +	     number.  */
>>>> +	  e = e->next;
>>>> +	  /* Keep in sync with the behaviour of cfi_parse_reg ().  */
>>>> +	  gas_assert (e->exp.X_op == O_register || e->exp.X_op == O_constant);
>>>
>>> ... you're asserting on user input here. Afaics neither is in any way
>>> enforced by dot_cfi_escape() / do_parse_cons_expression(). As to the
>>> comment - how does cfi_parse_reg() come into play for .cfi_escape?
>>
>> IIUC, the first operand of both of these opcodes (DW_CFA_expression,
>> DW_CFA_val_expression) will be a register.  Hence, cfi_parse_reg ()
>> behaviour is being cross-checked against.
>>
>> This function only aims to process some "simple DWARF expressions", and
>> if they are benign (OK to skip), we do not warn (nor error out with
>> SFRAME_XLATE_ERR_NOTREPRESENTED).  E.g. for,
>>
>> DW_CFA_expression: r1 (rdx) (DW_OP_breg7 (rsp): 136)
>> DW_CFA_expression: r0 (rax) (DW_OP_breg7 (rsp): 144)
> 
> Yet .cfi_escape accepts all sorts of expressions. It's okay to tell the
> user that for SFrame this isn't supported. But it's not okay to hide
> this information behind an internal error (that the failed assertion
> will result in).
> 

FWIW, cfi_parse_reg () will issue an as_bad () already.  I can change 
the assert to if () and continue to warn and return 
SFRAME_XLATE_ERR_NOTREPRESENTED.

>>>> +	  reg = e->exp.X_add_number;
>>>> +	  /* Get the scratchpad FRE.  */
>>>> +	  cur_fre = xlate_ctx->cur_fre;
>>>> +	  if (reg == SFRAME_CFA_SP_REG || reg == SFRAME_CFA_FP_REG
>>>> +	      || reg == cur_fre->cfa_base_reg)
>>>> +	    {
>>>> +	      as_warn (_("skipping SFrame FDE; .cfi_escape with reg (%#x)"), reg);
>>>> +	      err = SFRAME_XLATE_ERR_NOTREPRESENTED;
>>>> +	    }
>>>> +	  break;
>>>> +	/* FIXME - Also add processing for DW_CFA_GNU_args_size in future?  */
>>>> +	default:
>>>> +	  /* 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 (%#x)"), op);
>>>> +	  err = SFRAME_XLATE_ERR_NOTREPRESENTED;
>>>> +	  break;
>>>> +	}
>>>> +    }
>>>
>>> Doesn't all of this need to go in a loop, as a single .cfi_escape might
>>> specify multiple successive DW_CFA_*?
>>
>> The function intends to process only some DWARF expressions (basically
>> some commonly-occurring, but safe to skip from SFrame perspective).
>> Going into a loop may be necessary for opcodes other than
>> DW_CFA_expression or DW_CFA_val_expression, but at the moment, since we
>> prefer to cater to only the simple expressions, parsing the complete
>> expression is not required.
> 
> No, wait. Simple expressions or not isn't the point here. The point is
> that with a single .cfi_escape you can specify any number of things,
> i.e. also multiple operations involving multiple expressions. There's
> no limit to the number of operands to .cfi_escape afaics - it could
> literally be hundreds.
> 

(Sorry I am still missing the point)

Yes, potentially large number of operands may follow a .cfi_escape. And 
for all of those, we will hit the "default:" case of the thw switch and 
warn and bail out (like we are doing currently on master).

The only "safe to skip" cases are possible when the first opcode is 
DW_CFA_expression or DW_CFA_val_expression (for now).

Re: [PATCH] gas: sframe: partially process DWARF expressions in CFI_escape

[Jan Beulich]

On 28.01.2025 19:09, Indu Bhagat wrote:
> On 1/28/25 7:31 AM, Jan Beulich wrote:
>> On 28.01.2025 16:24, Indu Bhagat wrote:
>>> On 1/28/25 12:03 AM, Jan Beulich wrote:
>>>> On 28.01.2025 01:57, Indu Bhagat wrote:
>>>>> +      switch (op)
>>>>> +	{
>>>>> +	  /* Of all the possible opcodes expected here, it is safe to
>>>>> +	     ignore DW_CFA_expression and DW_CFA_val_expression, provided they
>>>>> +	     do not impact the SP / FP register.  */
>>>>> +	case DW_CFA_expression:
>>>>> +	case DW_CFA_val_expression:
>>>>> +	  /* Both DW_CFA_expression and DW_CFA_val_expression instructions take
>>>>> +	     two operands: an unsigned LEB128 value representing a register
>>>>> +	     number, and a DW_FORM_block value representing a DWARF expression.
>>>>> +	     For the current purpose, we simply need to know the register
>>>>> +	     number.  */
>>>>> +	  e = e->next;
>>>>> +	  /* Keep in sync with the behaviour of cfi_parse_reg ().  */
>>>>> +	  gas_assert (e->exp.X_op == O_register || e->exp.X_op == O_constant);
>>>>
>>>> ... you're asserting on user input here. Afaics neither is in any way
>>>> enforced by dot_cfi_escape() / do_parse_cons_expression(). As to the
>>>> comment - how does cfi_parse_reg() come into play for .cfi_escape?
>>>
>>> IIUC, the first operand of both of these opcodes (DW_CFA_expression,
>>> DW_CFA_val_expression) will be a register.  Hence, cfi_parse_reg ()
>>> behaviour is being cross-checked against.
>>>
>>> This function only aims to process some "simple DWARF expressions", and
>>> if they are benign (OK to skip), we do not warn (nor error out with
>>> SFRAME_XLATE_ERR_NOTREPRESENTED).  E.g. for,
>>>
>>> DW_CFA_expression: r1 (rdx) (DW_OP_breg7 (rsp): 136)
>>> DW_CFA_expression: r0 (rax) (DW_OP_breg7 (rsp): 144)
>>
>> Yet .cfi_escape accepts all sorts of expressions. It's okay to tell the
>> user that for SFrame this isn't supported. But it's not okay to hide
>> this information behind an internal error (that the failed assertion
>> will result in).
> 
> FWIW, cfi_parse_reg () will issue an as_bad () already.  I can change 
> the assert to if () and continue to warn and return 
> SFRAME_XLATE_ERR_NOTREPRESENTED.

Yes please.

>>>>> +	  reg = e->exp.X_add_number;
>>>>> +	  /* Get the scratchpad FRE.  */
>>>>> +	  cur_fre = xlate_ctx->cur_fre;
>>>>> +	  if (reg == SFRAME_CFA_SP_REG || reg == SFRAME_CFA_FP_REG
>>>>> +	      || reg == cur_fre->cfa_base_reg)
>>>>> +	    {
>>>>> +	      as_warn (_("skipping SFrame FDE; .cfi_escape with reg (%#x)"), reg);
>>>>> +	      err = SFRAME_XLATE_ERR_NOTREPRESENTED;
>>>>> +	    }
>>>>> +	  break;
>>>>> +	/* FIXME - Also add processing for DW_CFA_GNU_args_size in future?  */
>>>>> +	default:
>>>>> +	  /* 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 (%#x)"), op);
>>>>> +	  err = SFRAME_XLATE_ERR_NOTREPRESENTED;
>>>>> +	  break;
>>>>> +	}
>>>>> +    }
>>>>
>>>> Doesn't all of this need to go in a loop, as a single .cfi_escape might
>>>> specify multiple successive DW_CFA_*?
>>>
>>> The function intends to process only some DWARF expressions (basically
>>> some commonly-occurring, but safe to skip from SFrame perspective).
>>> Going into a loop may be necessary for opcodes other than
>>> DW_CFA_expression or DW_CFA_val_expression, but at the moment, since we
>>> prefer to cater to only the simple expressions, parsing the complete
>>> expression is not required.
>>
>> No, wait. Simple expressions or not isn't the point here. The point is
>> that with a single .cfi_escape you can specify any number of things,
>> i.e. also multiple operations involving multiple expressions. There's
>> no limit to the number of operands to .cfi_escape afaics - it could
>> literally be hundreds.
>>
> 
> (Sorry I am still missing the point)
> 
> Yes, potentially large number of operands may follow a .cfi_escape. And 
> for all of those, we will hit the "default:" case of the thw switch and 
> warn and bail out (like we are doing currently on master).
> 
> The only "safe to skip" cases are possible when the first opcode is 
> DW_CFA_expression or DW_CFA_val_expression (for now).

Even when others follow? You look at only the first two elements of the
chain starting from cfi_insn->u.esc. That chain, as said, can in principle
be hundreds of elements long, and can - aiui - contain any sequence of
DW_CFA_* (including their operands).

Jan

Re: [PATCH] gas: sframe: partially process DWARF expressions in CFI_escape

[Jan Beulich]

On 28.01.2025 01:57, Indu Bhagat wrote:
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -1310,6 +1310,77 @@ sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
>    return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
>  }
>  
> +/* Translate CFI_escape into 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.
> +
> +   In SFrame stack trace format, complex unwind info cannot be represented.  In
> +   such cases, SFrame FDE generation is skipped and the user is warned.  Recall,
> +   however, that SFrame stack trace information is meant to convey information
> +   about SP, FP and RA only.  Hence, some DWARF expressions, are indeed safe to
> +   skip.
> +
> +   This function partially processes some DWARF expresssions and returns
> +   SFRAME_XLATE_OK if OK to skip.  */
> +
> +static int
> +sframe_xlate_do_expr (struct sframe_xlate_ctx *xlate_ctx,
> +		      struct cfi_insn_data *cfi_insn)
> +{
> +  int op;
> +  struct cfi_escape_data *e;
> +  unsigned int reg = 0;
> +  int err = SFRAME_XLATE_OK;
> +  struct sframe_row_entry *cur_fre = NULL;
> +
> +  e = cfi_insn->u.esc;
> +
> +  if (e)
> +    {
> +      op = e->exp.X_add_number;
> +      switch (op)
> +	{
> +	  /* Of all the possible opcodes expected here, it is safe to
> +	     ignore DW_CFA_expression and DW_CFA_val_expression, provided they
> +	     do not impact the SP / FP register.  */
> +	case DW_CFA_expression:
> +	case DW_CFA_val_expression:
> +	  /* Both DW_CFA_expression and DW_CFA_val_expression instructions take
> +	     two operands: an unsigned LEB128 value representing a register
> +	     number, and a DW_FORM_block value representing a DWARF expression.
> +	     For the current purpose, we simply need to know the register
> +	     number.  */
> +	  e = e->next;

Another thing (quite the opposite of my concern regarding the chain being
quite long): How do you know e is non-NULL at this point, i.e. that it is
safe to de-reference ...

> +	  /* Keep in sync with the behaviour of cfi_parse_reg ().  */
> +	  gas_assert (e->exp.X_op == O_register || e->exp.X_op == O_constant);
> +	  reg = e->exp.X_add_number;

... without checking? Much like a single .cfi_escape can comprise many
DW_CFA_*, a single DW_CFA_* can also be split across multiple
.cfi_escape, aiui.

Consider this example covering both of the named cases (without involving
any DW_CFA_*expression, just to demonstrate the possible uses of the
directive):

	.text
func:
	.cfi_startproc
	.cfi_escape 0x0a
	nop
	.cfi_escape 0x02, 0x00, 0x02, 0x00
	nop
	.cfi_escape 0x03
	.cfi_escape 0x00
	.cfi_escape 0x00
	nop
	.cfi_escape 0x0b
	ret
	.cfi_endproc

Also what about in particular operations that don't affect any registers
(other than perhaps PC)? DW_CFA_nop being the most prominent example, but
also any purely advance-loc ones (and others, like remember/restore state).
Wouldn't you better skip those before deciding whether to warn?

Jan

Re: [PATCH] gas: sframe: partially process DWARF expressions in CFI_escape

[Indu Bhagat]

On 1/28/25 11:22 PM, Jan Beulich wrote:
> On 28.01.2025 01:57, Indu Bhagat wrote:
>> --- a/gas/gen-sframe.c
>> +++ b/gas/gen-sframe.c
>> @@ -1310,6 +1310,77 @@ sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
>>     return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
>>   }
>>   
>> +/* Translate CFI_escape into 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.
>> +
>> +   In SFrame stack trace format, complex unwind info cannot be represented.  In
>> +   such cases, SFrame FDE generation is skipped and the user is warned.  Recall,
>> +   however, that SFrame stack trace information is meant to convey information
>> +   about SP, FP and RA only.  Hence, some DWARF expressions, are indeed safe to
>> +   skip.
>> +
>> +   This function partially processes some DWARF expresssions and returns
>> +   SFRAME_XLATE_OK if OK to skip.  */
>> +
>> +static int
>> +sframe_xlate_do_expr (struct sframe_xlate_ctx *xlate_ctx,
>> +		      struct cfi_insn_data *cfi_insn)
>> +{
>> +  int op;
>> +  struct cfi_escape_data *e;
>> +  unsigned int reg = 0;
>> +  int err = SFRAME_XLATE_OK;
>> +  struct sframe_row_entry *cur_fre = NULL;
>> +
>> +  e = cfi_insn->u.esc;
>> +
>> +  if (e)
>> +    {
>> +      op = e->exp.X_add_number;
>> +      switch (op)
>> +	{
>> +	  /* Of all the possible opcodes expected here, it is safe to
>> +	     ignore DW_CFA_expression and DW_CFA_val_expression, provided they
>> +	     do not impact the SP / FP register.  */
>> +	case DW_CFA_expression:
>> +	case DW_CFA_val_expression:
>> +	  /* Both DW_CFA_expression and DW_CFA_val_expression instructions take
>> +	     two operands: an unsigned LEB128 value representing a register
>> +	     number, and a DW_FORM_block value representing a DWARF expression.
>> +	     For the current purpose, we simply need to know the register
>> +	     number.  */
>> +	  e = e->next;
> 
> Another thing (quite the opposite of my concern regarding the chain being
> quite long): How do you know e is non-NULL at this point, i.e. that it is
> safe to de-reference ...
> 

We can know that e is non-NULL because we are doing this only for 
DW_CFA_expression and DW_CFA_val_expression.

The DWARF standard says:

"The DW_CFA_expression instruction takes two operands: an unsigned
LEB128 value representing a register number, and a DW_FORM_block value
representing a DWARF expression."

Similar specification for DW_CFA_val_expression.

So the code is assuming that after the op (DW_CFA_expression or 
DW_CFA_val_expression), the next one up is the register.  I think 
assuming next one is the register is also OK to do because the DWARF 
expression evaluation is based on a stack machine model.

>> +	  /* Keep in sync with the behaviour of cfi_parse_reg ().  */
>> +	  gas_assert (e->exp.X_op == O_register || e->exp.X_op == O_constant);
>> +	  reg = e->exp.X_add_number;
> 
> ... without checking? Much like a single .cfi_escape can comprise many
> DW_CFA_*, a single DW_CFA_* can also be split across multiple
> .cfi_escape, aiui.
> 

We have checked that the op is DW_CFA_expression or 
DW_CFA_val_expression at this point...

For all other DW_CFA_* in the .cfi_escape, we do not decipher them, and 
simply warn and error out (and not generate any SFrame FDE for that 
function).

> Consider this example covering both of the named cases (without involving
> any DW_CFA_*expression, just to demonstrate the possible uses of the
> directive):
> 
> 	.text
> func:
> 	.cfi_startproc
> 	.cfi_escape 0x0a
> 	nop
> 	.cfi_escape 0x02, 0x00, 0x02, 0x00
> 	nop
> 	.cfi_escape 0x03
> 	.cfi_escape 0x00
> 	.cfi_escape 0x00
> 	nop
> 	.cfi_escape 0x0b
> 	ret
> 	.cfi_endproc
> 
> Also what about in particular operations that don't affect any registers
> (other than perhaps PC)? DW_CFA_nop being the most prominent example, but
> also any purely advance-loc ones (and others, like remember/restore state).
> Wouldn't you better skip those before deciding whether to warn?
> 

advance_loc, remember/restore state in .cfi_escape are important for 
SFrame stack trace data generation.  GAS will continue to warn and error 
out with:
   skipping SFrame FDE; .cfi_escape with op (XXX)

But to your point, surely, there may be more "harmless" cases when it is 
better to scan the .cfi_escape data and if it doesnt affect SFrame stack 
trace data, we do not warn.  My intention is to take them on a 
case-by-case basis: target the easy-to-parse/process and commonly 
occurring ones first.

Re: [PATCH] gas: sframe: partially process DWARF expressions in CFI_escape

[Jan Beulich]

On 29.01.2025 21:23, Indu Bhagat wrote:
> On 1/28/25 11:22 PM, Jan Beulich wrote:
>> On 28.01.2025 01:57, Indu Bhagat wrote:
>>> --- a/gas/gen-sframe.c
>>> +++ b/gas/gen-sframe.c
>>> @@ -1310,6 +1310,77 @@ sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
>>>     return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
>>>   }
>>>   
>>> +/* Translate CFI_escape into 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.
>>> +
>>> +   In SFrame stack trace format, complex unwind info cannot be represented.  In
>>> +   such cases, SFrame FDE generation is skipped and the user is warned.  Recall,
>>> +   however, that SFrame stack trace information is meant to convey information
>>> +   about SP, FP and RA only.  Hence, some DWARF expressions, are indeed safe to
>>> +   skip.
>>> +
>>> +   This function partially processes some DWARF expresssions and returns
>>> +   SFRAME_XLATE_OK if OK to skip.  */
>>> +
>>> +static int
>>> +sframe_xlate_do_expr (struct sframe_xlate_ctx *xlate_ctx,
>>> +		      struct cfi_insn_data *cfi_insn)
>>> +{
>>> +  int op;
>>> +  struct cfi_escape_data *e;
>>> +  unsigned int reg = 0;
>>> +  int err = SFRAME_XLATE_OK;
>>> +  struct sframe_row_entry *cur_fre = NULL;
>>> +
>>> +  e = cfi_insn->u.esc;
>>> +
>>> +  if (e)
>>> +    {
>>> +      op = e->exp.X_add_number;
>>> +      switch (op)
>>> +	{
>>> +	  /* Of all the possible opcodes expected here, it is safe to
>>> +	     ignore DW_CFA_expression and DW_CFA_val_expression, provided they
>>> +	     do not impact the SP / FP register.  */
>>> +	case DW_CFA_expression:
>>> +	case DW_CFA_val_expression:
>>> +	  /* Both DW_CFA_expression and DW_CFA_val_expression instructions take
>>> +	     two operands: an unsigned LEB128 value representing a register
>>> +	     number, and a DW_FORM_block value representing a DWARF expression.
>>> +	     For the current purpose, we simply need to know the register
>>> +	     number.  */
>>> +	  e = e->next;
>>
>> Another thing (quite the opposite of my concern regarding the chain being
>> quite long): How do you know e is non-NULL at this point, i.e. that it is
>> safe to de-reference ...
>>
> 
> We can know that e is non-NULL because we are doing this only for 
> DW_CFA_expression and DW_CFA_val_expression.
> 
> The DWARF standard says:
> 
> "The DW_CFA_expression instruction takes two operands: an unsigned
> LEB128 value representing a register number, and a DW_FORM_block value
> representing a DWARF expression."
> 
> Similar specification for DW_CFA_val_expression.
> 
> So the code is assuming that after the op (DW_CFA_expression or 
> DW_CFA_val_expression), the next one up is the register.  I think 
> assuming next one is the register is also OK to do because the DWARF 
> expression evaluation is based on a stack machine model.

I'm sorry, but no - the Dwarf standard doesn't matter here. What you
say needs to hold for the eventual section contents. Yet the piecing
together may be done by multiple .cfi_escape directives, each with a
single operand. In that case each one's cfi_insn->u.esc->next will be
NULL afaict.

>>> +	  /* Keep in sync with the behaviour of cfi_parse_reg ().  */
>>> +	  gas_assert (e->exp.X_op == O_register || e->exp.X_op == O_constant);
>>> +	  reg = e->exp.X_add_number;
>>
>> ... without checking? Much like a single .cfi_escape can comprise many
>> DW_CFA_*, a single DW_CFA_* can also be split across multiple
>> .cfi_escape, aiui.
>>
> 
> We have checked that the op is DW_CFA_expression or 
> DW_CFA_val_expression at this point...
> 
> For all other DW_CFA_* in the .cfi_escape, we do not decipher them, and 
> simply warn and error out (and not generate any SFrame FDE for that 
> function).

No, you don't. You only do so if every individual .cfi_escape encodes
one single DW_CFA_*. If it encodes multiple, as in ...

>> Consider this example covering both of the named cases (without involving
>> any DW_CFA_*expression, just to demonstrate the possible uses of the
>> directive):
>>
>> 	.text
>> func:
>> 	.cfi_startproc
>> 	.cfi_escape 0x0a
>> 	nop
>> 	.cfi_escape 0x02, 0x00, 0x02, 0x00

... this example, to entirely ignore the subsequent one(s).

>> 	nop
>> 	.cfi_escape 0x03
>> 	.cfi_escape 0x00
>> 	.cfi_escape 0x00
>> 	nop
>> 	.cfi_escape 0x0b
>> 	ret
>> 	.cfi_endproc
>>
>> Also what about in particular operations that don't affect any registers
>> (other than perhaps PC)? DW_CFA_nop being the most prominent example, but
>> also any purely advance-loc ones (and others, like remember/restore state).
>> Wouldn't you better skip those before deciding whether to warn?
> 
> advance_loc, remember/restore state in .cfi_escape are important for 
> SFrame stack trace data generation.  GAS will continue to warn and error 
> out with:
>    skipping SFrame FDE; .cfi_escape with op (XXX)

Well, then please consider DW_CFA_nop as the most basic example of something
surely not affecting any state.

> But to your point, surely, there may be more "harmless" cases when it is 
> better to scan the .cfi_escape data and if it doesnt affect SFrame stack 
> trace data, we do not warn.  My intention is to take them on a 
> case-by-case basis: target the easy-to-parse/process and commonly 
> occurring ones first.

See Jens' similar request.

Jan

Re: [PATCH] gas: sframe: partially process DWARF expressions in CFI_escape

[Indu Bhagat]

On 1/29/25 11:40 PM, Jan Beulich wrote:
> On 29.01.2025 21:23, Indu Bhagat wrote:
>> On 1/28/25 11:22 PM, Jan Beulich wrote:
>>> On 28.01.2025 01:57, Indu Bhagat wrote:
>>>> --- a/gas/gen-sframe.c
>>>> +++ b/gas/gen-sframe.c
>>>> @@ -1310,6 +1310,77 @@ sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
>>>>      return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
>>>>    }
>>>>    
>>>> +/* Translate CFI_escape into 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.
>>>> +
>>>> +   In SFrame stack trace format, complex unwind info cannot be represented.  In
>>>> +   such cases, SFrame FDE generation is skipped and the user is warned.  Recall,
>>>> +   however, that SFrame stack trace information is meant to convey information
>>>> +   about SP, FP and RA only.  Hence, some DWARF expressions, are indeed safe to
>>>> +   skip.
>>>> +
>>>> +   This function partially processes some DWARF expresssions and returns
>>>> +   SFRAME_XLATE_OK if OK to skip.  */
>>>> +
>>>> +static int
>>>> +sframe_xlate_do_expr (struct sframe_xlate_ctx *xlate_ctx,
>>>> +		      struct cfi_insn_data *cfi_insn)
>>>> +{
>>>> +  int op;
>>>> +  struct cfi_escape_data *e;
>>>> +  unsigned int reg = 0;
>>>> +  int err = SFRAME_XLATE_OK;
>>>> +  struct sframe_row_entry *cur_fre = NULL;
>>>> +
>>>> +  e = cfi_insn->u.esc;
>>>> +
>>>> +  if (e)
>>>> +    {
>>>> +      op = e->exp.X_add_number;
>>>> +      switch (op)
>>>> +	{
>>>> +	  /* Of all the possible opcodes expected here, it is safe to
>>>> +	     ignore DW_CFA_expression and DW_CFA_val_expression, provided they
>>>> +	     do not impact the SP / FP register.  */
>>>> +	case DW_CFA_expression:
>>>> +	case DW_CFA_val_expression:
>>>> +	  /* Both DW_CFA_expression and DW_CFA_val_expression instructions take
>>>> +	     two operands: an unsigned LEB128 value representing a register
>>>> +	     number, and a DW_FORM_block value representing a DWARF expression.
>>>> +	     For the current purpose, we simply need to know the register
>>>> +	     number.  */
>>>> +	  e = e->next;
>>>
>>> Another thing (quite the opposite of my concern regarding the chain being
>>> quite long): How do you know e is non-NULL at this point, i.e. that it is
>>> safe to de-reference ...
>>>
>>
>> We can know that e is non-NULL because we are doing this only for
>> DW_CFA_expression and DW_CFA_val_expression.
>>
>> The DWARF standard says:
>>
>> "The DW_CFA_expression instruction takes two operands: an unsigned
>> LEB128 value representing a register number, and a DW_FORM_block value
>> representing a DWARF expression."
>>
>> Similar specification for DW_CFA_val_expression.
>>
>> So the code is assuming that after the op (DW_CFA_expression or
>> DW_CFA_val_expression), the next one up is the register.  I think
>> assuming next one is the register is also OK to do because the DWARF
>> expression evaluation is based on a stack machine model.
> 
> I'm sorry, but no - the Dwarf standard doesn't matter here. What you
> say needs to hold for the eventual section contents. Yet the piecing
> together may be done by multiple .cfi_escape directives, each with a
> single operand. In that case each one's cfi_insn->u.esc->next will be
> NULL afaict.
> 
>>>> +	  /* Keep in sync with the behaviour of cfi_parse_reg ().  */
>>>> +	  gas_assert (e->exp.X_op == O_register || e->exp.X_op == O_constant);
>>>> +	  reg = e->exp.X_add_number;
>>>
>>> ... without checking? Much like a single .cfi_escape can comprise many
>>> DW_CFA_*, a single DW_CFA_* can also be split across multiple
>>> .cfi_escape, aiui.
>>>
>>
>> We have checked that the op is DW_CFA_expression or
>> DW_CFA_val_expression at this point...
>>
>> For all other DW_CFA_* in the .cfi_escape, we do not decipher them, and
>> simply warn and error out (and not generate any SFrame FDE for that
>> function).
> 
> No, you don't. You only do so if every individual .cfi_escape encodes
> one single DW_CFA_*. If it encodes multiple, as in ...
> 
>>> Consider this example covering both of the named cases (without involving
>>> any DW_CFA_*expression, just to demonstrate the possible uses of the
>>> directive):
>>>
>>> 	.text
>>> func:
>>> 	.cfi_startproc
>>> 	.cfi_escape 0x0a
>>> 	nop
>>> 	.cfi_escape 0x02, 0x00, 0x02, 0x00
> 
> ... this example, to entirely ignore the subsequent one(s).
> 

ACK.

Apologies for the oversight in understanding and implementation. You are 
right, the user may write .cfi_escape in various forms (multiple 
expressions and opcodes in a single directive or one 
expressions/operation split between multiple .cfi_escape); The 
cfi_escape_data in cfi_insn will reflect what the user input.

I have fixed this in V2.  Now we roughly check the length of the 
expression / operations after .cfi_escape and skip to warn for some 
harmless cases.  For too short or too long cases, we continue to warn 
and bail out.

>>> 	nop
>>> 	.cfi_escape 0x03
>>> 	.cfi_escape 0x00
>>> 	.cfi_escape 0x00
>>> 	nop
>>> 	.cfi_escape 0x0b
>>> 	ret
>>> 	.cfi_endproc
>>>
>>> Also what about in particular operations that don't affect any registers
>>> (other than perhaps PC)? DW_CFA_nop being the most prominent example, but
>>> also any purely advance-loc ones (and others, like remember/restore state).
>>> Wouldn't you better skip those before deciding whether to warn?
>>
>> advance_loc, remember/restore state in .cfi_escape are important for
>> SFrame stack trace data generation.  GAS will continue to warn and error
>> out with:
>>     skipping SFrame FDE; .cfi_escape with op (XXX)
> 
> Well, then please consider DW_CFA_nop as the most basic example of something
> surely not affecting any state.
> 

OK.

>> But to your point, surely, there may be more "harmless" cases when it is
>> better to scan the .cfi_escape data and if it doesnt affect SFrame stack
>> trace data, we do not warn.  My intention is to take them on a
>> case-by-case basis: target the easy-to-parse/process and commonly
>> occurring ones first.
> 
> See Jens' similar request.
> 

Thanks for reviewing,
Indu

Re: [PATCH] gas: sframe: partially process DWARF expressions in CFI_escape

[Jens Remus]

On 28.01.2025 01:57, Indu Bhagat wrote:
> 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 non SP/FP registers
>    - DW_CFA_value_expression affecting non SP/FP registers.

Could that be extended to also process the CFI instructions already
supported by SFrame, such as e.g. DW_CFA_val_offset through
sframe_xlate_do_val_offset()?

On s390 Glibc for instance uses .cfi_escape to piece together
.cfi_val_offset due to minimum binutils release requirements.

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] gas: sframe: partially process DWARF expressions in CFI_escape

[Indu Bhagat]

On 1/29/25 9:11 AM, Jens Remus wrote:
> On 28.01.2025 01:57, Indu Bhagat wrote:
>> 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 non SP/FP registers
>>    - DW_CFA_value_expression affecting non SP/FP registers.
> 
> Could that be extended to also process the CFI instructions already
> supported by SFrame, such as e.g. DW_CFA_val_offset through
> sframe_xlate_do_val_offset()?
> 

We could invoke sframe_xlate_do_val_offset (), we will just have to 
craft a new struct cfi_insn_data object just for checking the register...

What is the opinion on copying the necessary conditional into 
sframe_xlate_do_expr () for DW_CFA_val_offset ?

> On s390 Glibc for instance uses .cfi_escape to piece together
> .cfi_val_offset due to minimum binutils release requirements.
> 

Re: [PATCH] gas: sframe: partially process DWARF expressions in CFI_escape

[Jens Remus]

On 01.02.2025 02:15, Indu Bhagat wrote:
> On 1/29/25 9:11 AM, Jens Remus wrote:
>> On 28.01.2025 01:57, Indu Bhagat wrote:
>>> 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 non SP/FP registers
>>>    - DW_CFA_value_expression affecting non SP/FP registers.
>>
>> Could that be extended to also process the CFI instructions already
>> supported by SFrame, such as e.g. DW_CFA_val_offset through
>> sframe_xlate_do_val_offset()?
>>
> 
> We could invoke sframe_xlate_do_val_offset (), we will just have to craft a new struct cfi_insn_data object just for checking the register...
> 
> What is the opinion on copying the necessary conditional into sframe_xlate_do_expr () for DW_CFA_val_offset ?

You mean duplicating the sframe_xlate_do_val_offset() logic into
sframe_xlate_do_expr()?  I would not object.  Though I thought maybe
all existing sframe_xlate_do_*() could be covered, although it probably
only makes sense to take care of those where there are known use cases,
such as sframe_xlate_do_val_offset().

>> On s390 Glibc for instance uses .cfi_escape to piece together
>> .cfi_val_offset due to minimum binutils release requirements.

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/