Home Original page

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

Jan Beulich jbeulich@suse.com
Mon Feb 10 09:17:37 GMT 2025 
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.

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

>>> +/* Handle DW_CFA_val_offset in .cfi_escape.  */
>>> +
>>> +static int
>>> +sframe_xlate_do_escape_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>>
>> Why ATTRIBUTE_UNUSED? The parameter is used at the bottom of the function.
>>
> 
> Forgot to update from a previous iteration.  I have updated it now.
> 
>>> +				   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)?

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

>>> +/* 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?

>>> --- /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?

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

Jan

>          .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
> 
>
More information about the Binutils mailing list