[PATCH,V3] gas: sframe: partially process DWARF unwind info in CFI_escape

[Jan Beulich]

On 20.02.2025 22:44, Indu Bhagat wrote:
> On 2/19/25 6:56 AM, Jan Beulich wrote:
>> On 13.02.2025 02:14, Indu Bhagat wrote:
>>> @@ -1310,6 +1315,192 @@ sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
>>>     return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
>>>   }
>>>   
>>> +/* Handle DW_CFA_expression in .cfi_escape.
>>> +
>>> +   As with sframe_xlate_do_cfi_escape, the intent of this function is to detect
>>> +   only the simple-to-process but common cases, where skipping over the escape
>>> +   expr data does not affect correctness of the SFrame stack trace data.  */
>>> +
>>> +static int
>>> +sframe_xlate_do_escape_expr (const struct sframe_xlate_ctx *xlate_ctx,
>>> +			     const struct cfi_insn_data *cfi_insn)
>>> +{
>>> +  const struct cfi_escape_data *e = cfi_insn->u.esc;
>>> +  const struct sframe_row_entry *cur_fre = NULL;
>>> +  int err = SFRAME_XLATE_OK;
>>> +  unsigned int reg = 0;
>>> +  unsigned int i = 0;
>>> +
>>> +  /* Check roughly for an expression
>>> +     DW_CFA_expression: r1 (rdx) (DW_OP_bregN (reg): XXX).  */
>>> +#define CFI_ESC_NUM_EXP 4
>>> +  offsetT items[CFI_ESC_NUM_EXP] = {0};
>>> +  while (e->next)
>>> +    {
>>> +      e = e->next;
>>> +      if ((i == 2 && items[1] == 0) /* Zero length in DWARF expr.  */
>>
>> But when you fetch a total of 4 bytes a length of 1 isn't okay either,
>> is it? Considering ...
>>
>>> +	  || i >= CFI_ESC_NUM_EXP || e->exp.X_op != O_constant)
>>> +	return SFRAME_XLATE_ERR_NOTREPRESENTED;
>>> +      items[i] = e->exp.X_add_number;
>>> +      i++;
>>> +    }
>>> +
>>> +  if (i <= CFI_ESC_NUM_EXP - 1)
>>> +    return SFRAME_XLATE_ERR_NOTREPRESENTED;
>>> +
>>> +  cur_fre = xlate_ctx->cur_fre;
>>> +  /* reg operand to DW_CFA_expression is ULEB128.  For the purpose at hand,
>>> +     however, the register value will be less than 127 (CFI_ESC_NUM_EXP set
>>> +     to 4).  */
>>
>> ... this line of thought, shouldn't you check that the expression length
>> is exactly 2? 
> 
> Fair enough, I guess an explicit check for 2 is clearer.  Now I have
> 
>    while (e->next) 
> 
>      {
>        e = e->next;
>        if ((i == 2 && (items[1] != 2)) /* Expected len of 2 in DWARF 
> expr.  */
>         ...
> 
> 
> And then do you really need to check the subsequent
>> expressions for being O_constant? You don't care about the ultimate
>> values, after all. (As you may guess, I'm also trying to consider how
>> this code will need to change when .cfi_escape accepts more than just
>> raw bytes.)
> 
> We dont care about the values items[2] and items[3], no. I thought the 
> check for O_constant did not hurt the patch and its scope negatively.

Maybe. My main concern is with unnecessary checks getting in the way of
future adjustments. Whoever does those may find it hard to decide what
is needed (and hence needs updating) and what isn't needed (and hence
could as well be dropped). Hopefully for the .cfi_escape extension I'm
working on that'll still be fresh in our memories (as it's not clear
yet who's going to need to do the adjustments here, as it depends on
what lands first).

>>> --- /dev/null
>>> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-9.s
>>> @@ -0,0 +1,18 @@
>>> +## CFI_escape may be used to encode DWARF expressions among other things.
>>> +## Depending on the register applicable for the DWARF expression, skipping
>>> +## SFrame FDE may be OK: SFrame stack trace information is relevant for SP, FP
>>> +## and RA only.  In this test, CFI_escape is safe to skip (does not affect
>>> +## correctness of SFrame data).  The register 0xc is non SP / FP on both
>>> +## aarch64 and x86_64.
>>> +	.cfi_startproc
>>> +	.long 0
>>> +	.cfi_def_cfa_offset 16
>>> +# DW_CFA_expression,reg 0xc,length 2,DW_OP_breg6,SLEB(-8)
>>> +	.cfi_escape 0x10,0xc,0x2,0x76,0x78
>>> +# DW_CFA_nop
>>> +	.cfi_escape 0x0
>>> +	.cfi_escape 0x0,0x0,0x0,0x0
>>> +# DW_CFA_val_offset,reg 0xc,ULEB scaled offset(32)
>>
>> The scaling factor of 8 isn't universal. The expectation therefore is
>> that once sframe is to be supported an architecture with a different
>> factor, this test would need adjustment (or wouldn't be "common" anymore)?
> 
> Hmm.  I removed the "(32)" from the comment to avoid this sort of confusion.

Did you at the same time also relax the testcase expectation then?

Jan