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

[Jan Beulich]

On 29.01.2025 21:23, Indu Bhagat wrote:
> 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.

I'm sorry, but no - the Dwarf standard doesn't matter here. What you
say needs to hold for the eventual section contents. Yet the piecing
together may be done by multiple .cfi_escape directives, each with a
single operand. In that case each one's cfi_insn->u.esc->next will be
NULL afaict.

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

No, you don't. You only do so if every individual .cfi_escape encodes
one single DW_CFA_*. If it encodes multiple, as in ...

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

... this example, to entirely ignore the subsequent one(s).

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

Well, then please consider DW_CFA_nop as the most basic example of something
surely not affecting any state.

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

See Jens' similar request.

Jan