[PATCH,V3] gas: sframe: partially process DWARF unwind info in CFI_escape

[Jan Beulich]

On 13.02.2025 02:14, Indu Bhagat wrote:
> @@ -1310,6 +1315,192 @@ 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.
> +
> +   As with sframe_xlate_do_cfi_escape, the intent of this function is to detect
> +   only the simple-to-process but common cases, where skipping over the escape
> +   expr data does not affect correctness of the SFrame stack trace data.  */
> +
> +static int
> +sframe_xlate_do_escape_expr (const struct sframe_xlate_ctx *xlate_ctx,
> +			     const struct cfi_insn_data *cfi_insn)
> +{
> +  const struct cfi_escape_data *e = cfi_insn->u.esc;
> +  const struct sframe_row_entry *cur_fre = NULL;
> +  int err = SFRAME_XLATE_OK;
> +  unsigned int reg = 0;
> +  unsigned int i = 0;
> +
> +  /* Check roughly for an expression
> +     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 == 2 && items[1] == 0) /* Zero length in DWARF expr.  */

But when you fetch a total of 4 bytes a length of 1 isn't okay either,
is it? Considering ...

> +	  || i >= CFI_ESC_NUM_EXP || e->exp.X_op != O_constant)
> +	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 operand to DW_CFA_expression is ULEB128.  For the purpose at hand,
> +     however, the register value will be less than 127 (CFI_ESC_NUM_EXP set
> +     to 4).  */

... this line of thought, shouldn't you check that the expression length
is exactly 2? And then do you really need to check the subsequent
expressions for being O_constant? You don't care about the ultimate
values, after all. (As you may guess, I'm also trying to consider how
this code will need to change when .cfi_escape accepts more than just
raw bytes.)

Also (nit): "less than 128" or "at most 127".

> +  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)

This looks to be the only use of "cur_fre". Is the local variable really
worth having?

> +    {
> +      as_warn (_("skipping SFrame FDE; "
> +		 ".cfi_escape DW_CFA_expression with %s reg %u"),
> +	       sframe_register_name (reg), reg);

Won't the %s result in printing of <null> or whatever the underlying libc
uses when only the last of the conditions resulted in us getting here?
That's not a crash, sure, but also not really a good diagnostic.

> +      err = SFRAME_XLATE_ERR_NOTREPRESENTED;
> +    }
> +
> +  return err;
> +}
> +
> +/* Handle DW_CFA_val_offset in .cfi_escape.
> +
> +   As with sframe_xlate_do_cfi_escape, the intent of this function is to detect
> +   only the simple-to-process but common cases, where skipping over the escape
> +   expr data does not affect correctness of the SFrame stack trace data.  */
> +
> +static int
> +sframe_xlate_do_escape_val_offset (const struct sframe_xlate_ctx *xlate_ctx,
> +				   const struct cfi_insn_data *cfi_insn)
> +{
> +  const struct cfi_escape_data *e = cfi_insn->u.esc;
> +  int err = SFRAME_XLATE_OK;
> +  unsigned int i = 0;
> +  unsigned int reg;
> +  offsetT offset;
> +
> +  /* 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 || e->exp.X_op != O_constant)
> +	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.  However, since we limit our focus on cases when
> +     CFI_ESC_NUM_EXP is 2, reading UELB can be skipped.  IOW, although not

Nit: ULEB

> +     ideal, SFrame FDE generation in case of an APX register in
> +     DW_CFA_val_offset is being skipped (PS: this does _not_ mean incorrect
> +     SFrame stack trace data).
> +
> +     Recall that the intent here is to check for simple and prevalent cases,
> +     when feasible.  */

This more extensive comment may want referencing from the much less
informative one in sframe_xlate_do_escape_expr().

> +  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 = { .insn = DW_CFA_val_offset };
> +  temp.u.ri.reg = reg;
> +  temp.u.ri.offset = offset * DWARF2_CIE_DATA_ALIGNMENT;

I'm curious: Why partly by initializer and partly by assignments?

> +  err = sframe_xlate_do_val_offset (xlate_ctx, &temp, true);
> +
> +  return err;
> +}
> +
> +/* Handle CFI_escape in SFrame context.
> +
> +   .cfi_escape CFI directive allows the user to add arbitrary bytes to the

In order for this to not go stale very quickly, may I ask to use "data"
instead of "bytes"?

> +   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

Again here then.

> +   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 (const struct sframe_xlate_ctx *xlate_ctx,
> +			    const 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;
> +
> +  if (e->exp.X_op != O_constant)
> +    return SFRAME_XLATE_ERR_NOTREPRESENTED;

Shouldn't this also wire to the as_warn() at the bottom of the function?

> +  firstop = e->exp.X_add_number;
> +  switch (firstop)
> +    {
> +    case DW_CFA_nop:
> +      /* One or more nops together are harmless for SFrame.  */
> +      while (e->next)
> +	{
> +	  e = e->next;
> +	  if (e->exp.X_op != O_constant || 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);

Similarly for the two calls: Shouldn't SFRAME_XLATE_ERR_NOTREPRESENTED
coming back also cause the warning to be issued?

> --- /dev/null
> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-9.s
> @@ -0,0 +1,18 @@
> +## 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,reg 0xc,length 2,DW_OP_breg6,SLEB(-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(32)

The scaling factor of 8 isn't universal. The expectation therefore is
that once sframe is to be supported an architecture with a different
factor, this test would need adjustment (or wouldn't be "common" anymore)?

Jan