[PATCH,V2 2/2] gas: sframe: partially process DWARF expressions in CFI_escape

[Indu Bhagat]

On 2/10/25 1:17 AM, Jan Beulich wrote:
> On 10.02.2025 07:06, Indu Bhagat wrote:
>> On 2/7/25 3:22 AM, Jan Beulich wrote:
>>> On 05.02.2025 00:10, Indu Bhagat wrote:
>>>> +  /* Check roughly for expression of the kind
>>>> +     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 >= CFI_ESC_NUM_EXP)
>>>> +	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;
>>>
>>> See the respective comment on sframe_xlate_do_escape_val_offset() below.
>>>
>>>> +  cur_fre = xlate_ctx->cur_fre;
>>>> +  reg = items[0];
>>>> +#undef CFI_ESC_NUM_EXP
>>>
>>> You fetch 4 bytes, then use only the first? Shouldn't you at least check
>>> expression length and DW_OP_breg<N> (and hence the 2nd register number)
>>> as well? What about DW_OP_reg<N>, DW_OP_bregx, and DW_OP_regx?
>>>
>>
>> Re:check expression length - We do check for expression length; For
>> length >= CFI_ESC_NUM_EXP or length <= CFI_ESC_NUM_EXP - 1, we return
>> SFRAME_XLATE_ERR_NOTREPRESENTED.
> 
> That's not what I mean though. The length is encoded in the stream of
> bytes (items[1] iirc), and you don't check that at all. Imo you want to
> reject anything that isn't valid in the first place, i.e. you probably
> shouldn't even look past this encoded length if it is, say, zero.
> 

OK, makes sense.  I now have :

       if ((i == 2 && items[1] == 0) /* Zero length in DWARF expr.  */
           || i >= CFI_ESC_NUM_EXP || e->exp.X_op != O_constant)
         return SFRAME_XLATE_ERR_NOTREPRESENTED;

inside the while (e->next) loop.

>> Once the check for target register is done, what follows after is
>> assumed to of no consequence in terms of how it affects SFrame stack
>> trace data.  When GAS is in these APIs working out the SFrame
>> generation, validating .cfi_escape data is not intended.
> 
> Yet then - why fetch the extra bytes?
> 

OK. Fixed now as above.


>>>> +				   struct cfi_insn_data *cfi_insn)
>>>> +{
>>>> +  const struct cfi_escape_data *e = cfi_insn->u.esc;
>>>> +  unsigned int reg = 0;
>>>> +  offsetT offset = 0;
>>>> +  int err = SFRAME_XLATE_OK;
>>>> +  int i = 0;
>>>> +
>>>> +  if (!e || !e->next)
>>>> +    return SFRAME_XLATE_ERR_INVAL;
>>>> +
>>>> +  /* Check for (DW_CFA_val_offset reg scaled_offset) sequence.  */
>>>> +#define CFI_ESC_NUM_EXP 2
>>>> +  offsetT items[CFI_ESC_NUM_EXP] = {0};
>>>> +  while (e->next)
>>>> +    {
>>>> +      e = e->next;
>>>> +      if (i >= CFI_ESC_NUM_EXP)
>>>> +	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;
>>>
>>> Both arguments to DW_CFA_val_offset are ULEB128. Especially with APX (on
>>> x86) we're going to see Dwarf register numbers above 127, for the extended
>>> GPRs. And large enough stack frames would also require multi-byte offset
>>> representation.
>>>
>>
>> In this case, e.g., we are specifically checking for an expression with
>> opcode = DW_CFA_val_offset and _two_ operands.  Hence, for APX (on x86)
>> we will fall into the case of (i >= CFI_ESC_NUM_EXP) and return
>> SFRAME_XLATE_ERR_NOTREPRESENTED.
>>
>> Although not ideal (because ideally we do not want to skip FDE
>> generation due to APX register in DW_CFA_val_offset), it is not leading
>> to incorrect SFrame FDE data.
>>
>> The intention is to only partially process some simple expressions, when
>> feasible.
> 
> Can comments then please be extended to mention all intentional limitations
> (for readers to be able to tell those from behavior that wasn't intended to
> be the way it is, and hence may want corrrecting)?
> 

Yes, I have been adding comments in the code reflecting this discussion.

>>>> +  reg = items[0];
>>>> +  offset = items[1];
>>>> +#undef CFI_ESC_NUM_EXP
>>>> +
>>>> +  /* Invoke sframe_xlate_do_val_offset itself for checking.  */
>>>> +  struct cfi_insn_data *temp = XCNEW (struct cfi_insn_data);
>>>> +  temp->insn = DW_CFA_val_offset;
>>>> +  temp->u.ri.reg = reg;
>>>> +  temp->u.ri.offset = offset;
>>>> +
>>>> +  err = sframe_xlate_do_val_offset (xlate_ctx, temp, true);
>>>> +  XDELETE (temp);
>>>
>>> No real need to involve dynamic allocation?
>>>
>>>     struct cfi_insn_data temp = {
>>>       .insn = DW_CFA_val_offset,
>>>       .u = {
>>>         .ri = {
>>> 	.reg = reg,
>>> 	.offset = offset
>>>         }
>>>       }
>>>     };
>>>
>>>     err = sframe_xlate_do_val_offset (xlate_ctx, &temp, true);
>>>
>>> ?
>>>
>>
>> OK.  Although I prefer the other style, so I now have:
>>
>>     /* Invoke sframe_xlate_do_val_offset itself for checking.  */
>>     struct cfi_insn_data temp;
>>     temp.insn = DW_CFA_val_offset;
>>     temp.u.ri.reg = reg;
>>     /* Skip undoing the scaling with DWARF2_CIE_DATA_ALIGNMENT.  offset is
>>        non-uleb anyway.  */
>>     temp.u.ri.offset = offset;
>>     err = sframe_xlate_do_val_offset (xlate_ctx, &temp, true);
> 
> Please have the variable have an initializer. I specifically wrote it that way;
> if you don't like the extensive one, use e.g. just
> 
> struct cfi_insn_data temp = { .insn = DW_CFA_val_offset };
> 
> This way you'll make sure to initialize the full struct, including unused parts
> (due to [perhaps future] union members). That'll save us from possibly
> surprising compiler diagnostics about accessing uninitialized fields (it was
> certainly more than once that newly added warnings were issued in wider a
> manner than would have been desirable).
> 

OK.

>>>> +/* Handle CFI_escape in 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.
>>>> +
>>>> +   Complex unwind info added using .cfi_escape directive _may_ be of no
>>>> +   consequence for SFrame when the affected registers are not SP, FP, RA or
>>>> +   CFA.  The challenge in confirming the afore-mentioned is that it needs full
>>>> +   parsing and validation of the bytes presented after .cfi_escape.  Here we
>>>> +   take a case-by-case approach towards skipping _some_ instances of
>>>> +   .cfi_escape: skip those that can be *easily* determined to be harmless in
>>>> +   the context of SFrame stack trace information.
>>>> +
>>>> +   This function partially processes bytes following .cfi_escape and returns
>>>> +   SFRAME_XLATE_OK if OK to skip.  */
>>>
>>> See PR gas/32613: It won't be for long that only simple byte sequences can
>>> appear here.
>>>
>>>> +static int
>>>> +sframe_xlate_do_cfi_escape (struct sframe_xlate_ctx *xlate_ctx,
>>>> +			    struct cfi_insn_data *cfi_insn)
>>>
>>> As you don't mean to alter *cfi_insn, please use pointer-to-const. Possibly
>>> same for xlate_ctx.
>>>
>>
>> Yes, I would like do that in a separate commit later for all functions
>> here.  But prior to that, I need to get some bugfixes addressed which
>> are higher in my priority queue.
> 
> What's getting in the way of doing it here right away?
> 

Ah yes, for this new function, it can be done now.  I was referring to 
the same fix being applied to all the other (similar) APIs in this file.

>>>> --- /dev/null
>>>> +++ b/gas/testsuite/gas/cfi-sframe/cfi-sframe-common-9.s
>>>> @@ -0,0 +1,17 @@
>>>> +## 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 for reg 0xc
>>>> +	.cfi_escape 0x10,0xc,0x2,0x76,0x78
>>>
>>> The comment only describes part of this expression. Since such hard-coded
>>> byte sequences are hard to decipher, can such comments please fully describe
>>> things?
>>
>> OK. Added comments.
>>
>> # DW_CFA_expression,reg 0xc,length 2,DW_OP_breg6 (rbp),ULEB(-8)
> 
> This being an arch-independent check, may I ask that you don't mention x86
> register names here?
> 

Yes, I intend this test to be arch independent.  I have removed the rbp 
mention. Thanks.

> What's ULEB(-8)? 'U' standing for "unsigned" doesn't seem to fit with a
> negative number.
> 

My bad. I have changed this to SLEB.

> 
>>           .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 for 32
>>           .cfi_escape 0x14,0xc,0x4
>>
>>
>