[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