[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