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

[Jan Beulich]

On 22.02.2025 17:58, Indu Bhagat wrote:
> @@ -1310,6 +1315,223 @@ 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.
> +
> +   Sets CALLER_WARN_P for skipped cases (and returns SFRAME_XLATE_OK) where the
> +   caller must warn.  The caller then must also set
> +   SFRAME_XLATE_ERR_NOTREPRESENTED for their callers.  */
> +
> +static int
> +sframe_xlate_do_escape_expr (const struct sframe_xlate_ctx *xlate_ctx,
> +			     const struct cfi_insn_data *cfi_insn,
> +			     bool *caller_warn_p)
> +{
> +  const struct cfi_escape_data *e = cfi_insn->u.esc;
> +  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): OFFSET).  */
> +#define CFI_ESC_NUM_EXP 4
> +  offsetT items[CFI_ESC_NUM_EXP] = {0};
> +  while (e->next)
> +    {
> +      e = e->next;
> +      if ((i == 2 && (items[1] != 2)) /* Expected len of 2 in DWARF expr.  */
> +	  /* We do not care for the exact values of items[2] and items[3],
> +	     so an explicit check for O_constant isnt necessary either.  */
> +	  || i >= CFI_ESC_NUM_EXP || (i < 2 && e->exp.X_op != O_constant))
> +	goto warn_and_exit;
> +      items[i] = e->exp.X_add_number;
> +      i++;
> +    }
> +
> +  if (i <= CFI_ESC_NUM_EXP - 1)
> +    goto warn_and_exit;
> +
> +  /* reg operand to DW_CFA_expression is ULEB128.  For the purpose at hand,
> +     however, the register value will be less than 128 (CFI_ESC_NUM_EXP set
> +     to 4).  See an extended comment in sframe_xlate_do_escape_expr for why
> +     reading ULEB is okay to skip without sacrificing correctness.  */
> +  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 == xlate_ctx->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;
> +    }
> +  /* else safe to skip, so continue to return SFRAME_XLATE_OK.  */
> +
> +  *caller_warn_p = false;

I don't think this and ...

> +  return err;
> +
> +warn_and_exit:
> +  *caller_warn_p = true;
> +  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.
> +
> +   Sets CALLER_WARN_P for skipped cases (and returns SFRAME_XLATE_OK) where the
> +   caller must warn.  The caller then must also set
> +   SFRAME_XLATE_ERR_NOTREPRESENTED for their callers.  */
> +
> +static int
> +sframe_xlate_do_escape_val_offset (const struct sframe_xlate_ctx *xlate_ctx,
> +				   const struct cfi_insn_data *cfi_insn,
> +				   bool *caller_warn_p)
> +{
> +  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)
> +	goto warn_and_exit;
> +      items[i] = e->exp.X_add_number;
> +      i++;
> +    }
> +  if (i <= CFI_ESC_NUM_EXP - 1)
> +    goto warn_and_exit;
> +
> +  /* 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 ULEB can be skipped.  IOW, although not
> +     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.  */
> +
> +  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,
> +    .u = {
> +      .ri = {
> +	.reg = reg,
> +	.offset = offset * DWARF2_CIE_DATA_ALIGNMENT
> +      }
> +    }
> +  };
> +  err = sframe_xlate_do_val_offset (xlate_ctx, &temp, true);
> +  *caller_warn_p = false;

... this are needed; the caller ...

> +  return err;
> +
> +warn_and_exit:
> +  *caller_warn_p = true;
> +  return err;
> +}
> +
> +/* Handle CFI_escape in SFrame context.
> +
> +   .cfi_escape CFI directive allows the user to add arbitrary data 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 data 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 data 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;
> +  bool warn_p = false;

... takes care of it starting out false.

Okay preferably with the redundant assignments dropped.

I've just posted the series including the .cfi_escape enhancements, and I've
Cc-ed you on the relevant patches. Assuming your change is going to go in
soon-ish, I'll aim at re-basing mine, to adjust code being added here. I
won't get to committing mine anyway (even in the event that there would be
no need for a v2) in the next week or so anyway, as mid-day tomorrow I'll
leave for a short vacation.

Jan