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

[Jan Beulich <jbeulich@suse.com>]

On 28.01.2025 16:24, Indu Bhagat wrote:
> On 1/28/25 12:03 AM, Jan Beulich wrote:
>> On 28.01.2025 01:57, Indu Bhagat wrote:
>>> +      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)

Yet .cfi_escape accepts all sorts of expressions. It's okay to tell the
user that for SFrame this isn't supported. But it's not okay to hide
this information behind an internal error (that the failed assertion
will result in).

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

No, wait. Simple expressions or not isn't the point here. The point is
that with a single .cfi_escape you can specify any number of things,
i.e. also multiple operations involving multiple expressions. There's
no limit to the number of operands to .cfi_escape afaics - it could
literally be hundreds.

Jan