[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