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

[Jan Beulich]

On 28.01.2025 19:09, Indu Bhagat wrote:
> On 1/28/25 7:31 AM, Jan Beulich wrote:
>> 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).
> 
> FWIW, cfi_parse_reg () will issue an as_bad () already.  I can change 
> the assert to if () and continue to warn and return 
> SFRAME_XLATE_ERR_NOTREPRESENTED.

Yes please.

>>>>> +	  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.
>>
> 
> (Sorry I am still missing the point)
> 
> Yes, potentially large number of operands may follow a .cfi_escape. And 
> for all of those, we will hit the "default:" case of the thw switch and 
> warn and bail out (like we are doing currently on master).
> 
> The only "safe to skip" cases are possible when the first opcode is 
> DW_CFA_expression or DW_CFA_val_expression (for now).

Even when others follow? You look at only the first two elements of the
chain starting from cfi_insn->u.esc. That chain, as said, can in principle
be hundreds of elements long, and can - aiui - contain any sequence of
DW_CFA_* (including their operands).

Jan