[PATCH] gas: sframe: partially process DWARF expressions in CFI_escape

[Indu Bhagat]

On 1/28/25 11:22 PM, 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)
>> +{
>> +  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 ...
> 

We can know that e is non-NULL because we are doing this only for 
DW_CFA_expression and DW_CFA_val_expression.

The DWARF standard says:

"The DW_CFA_expression instruction takes two operands: an unsigned
LEB128 value representing a register number, and a DW_FORM_block value
representing a DWARF expression."

Similar specification for DW_CFA_val_expression.

So the code is assuming that after the op (DW_CFA_expression or 
DW_CFA_val_expression), the next one up is the register.  I think 
assuming next one is the register is also OK to do because the DWARF 
expression evaluation is based on a stack machine model.

>> +	  /* 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.
> 

We have checked that the op is DW_CFA_expression or 
DW_CFA_val_expression at this point...

For all other DW_CFA_* in the .cfi_escape, we do not decipher them, and 
simply warn and error out (and not generate any SFrame FDE for that 
function).

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

advance_loc, remember/restore state in .cfi_escape are important for 
SFrame stack trace data generation.  GAS will continue to warn and error 
out with:
   skipping SFrame FDE; .cfi_escape with op (XXX)

But to your point, surely, there may be more "harmless" cases when it is 
better to scan the .cfi_escape data and if it doesnt affect SFrame stack 
trace data, we do not warn.  My intention is to take them on a 
case-by-case basis: target the easy-to-parse/process and commonly 
occurring ones first.