Home Original page

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

Indu Bhagat indu.bhagat@oracle.com
Tue Feb 4 14:20:03 GMT 2025 
On 1/29/25 11:40 PM, Jan Beulich wrote:
> 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).
> 

ACK.

Apologies for the oversight in understanding and implementation. You are 
right, the user may write .cfi_escape in various forms (multiple 
expressions and opcodes in a single directive or one 
expressions/operation split between multiple .cfi_escape); The 
cfi_escape_data in cfi_insn will reflect what the user input.

I have fixed this in V2.  Now we roughly check the length of the 
expression / operations after .cfi_escape and skip to warn for some 
harmless cases.  For too short or too long cases, we continue to warn 
and bail out.

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

OK.

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

Thanks for reviewing,
Indu
More information about the Binutils mailing list