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

[Indu Bhagat]

On 2/7/25 3:22 AM, Jan Beulich wrote:
> On 05.02.2025 00:10, Indu Bhagat wrote:
>> --- a/gas/gen-sframe.c
>> +++ b/gas/gen-sframe.c
>> @@ -1121,7 +1121,8 @@ sframe_xlate_do_offset (struct sframe_xlate_ctx *xlate_ctx,
>>   
>>   static int
>>   sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>> -			    struct cfi_insn_data *cfi_insn)
>> +			    struct cfi_insn_data *cfi_insn,
>> +			    bool cfi_escape_p)
>>   {
>>     /* Previous value of register is CFA + offset.  However, if the specified
>>        register is not interesting (SP, FP, or RA reg), the current
>> @@ -1134,7 +1135,8 @@ sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>>         /* Ignore SP reg, if offset matches assumed default rule.  */
>>         || (cfi_insn->u.ri.reg == SFRAME_CFA_SP_REG && cfi_insn->u.ri.offset != 0))
>>       {
>> -      as_warn (_("skipping SFrame FDE; DW_CFA_val_offset with %s register %u"),
>> +      as_warn (_("skipping SFrame FDE; %sDW_CFA_val_offset with %s reg %u"),
>> +	       cfi_escape_p ? ".cfi_escape " : "",
>>   	       sframe_register_name (cfi_insn->u.ri.reg), cfi_insn->u.ri.reg);
> 
> Hmm - I thought patch 1 was added to eliminate the need to alter this
> here again.
> 

I took Jens suggestion.  I have merged the two patches for V3 (not sent 
to the list yet) and now have:

       /* Ignore SP reg, if offset matches assumed default rule.  */
       || (cfi_insn->u.ri.reg == SFRAME_CFA_SP_REG && 
cfi_insn->u.ri.offset != 0))
     {
       as_warn (_("skipping SFrame FDE; %s with %s reg %u"),
                cfi_esc_p ? ".cfi_escape DW_CFA_val_offset" : 
".cfi_val_offset",
                sframe_register_name (cfi_insn->u.ri.reg), 
cfi_insn->u.ri.reg);
       return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
     }

>> @@ -1310,6 +1312,173 @@ 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.  */
>> +
>> +static int
>> +sframe_xlate_do_escape_expr (struct sframe_xlate_ctx *xlate_ctx,
>> +			     struct cfi_insn_data *cfi_insn)
>> +{
>> +  const struct cfi_escape_data *e = cfi_insn->u.esc;
>> +  const struct sframe_row_entry *cur_fre = NULL;
>> +  unsigned int reg = 0;
>> +  int err = SFRAME_XLATE_OK;
>> +  int i = 0;
> 
> unsigned int please.
> 

OK.

>> +  if (!e || !e->next)
>> +    return SFRAME_XLATE_ERR_INVAL;
> 
> Why the check for e being NULL? The caller already de-referenced it. And
> the e->next check is redundant with the while() below.
> 

Just wanted to avoid unnecessary bugs in future when caller APIs evolve 
etc.  BTW, the compiled code (-O2) contains most of these functions 
inlined, so its not a concern.

For now, I have removed the complete check though.  We have tests for 
this code path, so my original concern was not totally necessary.

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

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.

This patch is about failing less often: do not skip generating FDE if 
the complex expressions involve a register of no interest.

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

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

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

>> +{
>> +  const struct cfi_escape_data *e;
>> +  int err = SFRAME_XLATE_OK;
>> +  offsetT firstop;
>> +
>> +  e = cfi_insn->u.esc;
>> +
>> +  if (!e)
>> +    return SFRAME_XLATE_ERR_INVAL;
>> +
>> +  firstop = e->exp.X_add_number;
> 
> What if .X_op isn't O_constant (applies elsewhere as well of course)?
> 

Hmm, I should check explicitly and exit early.  I have added that.

>> +  switch (firstop)
>> +    {
>> +    case DW_CFA_nop:
>> +      while (e->next)
>> +	{
>> +	  e = e->next;
>> +	  if (e->exp.X_add_number != DW_CFA_nop)
>> +	    {
>> +	      err = SFRAME_XLATE_ERR_NOTREPRESENTED;
>> +	      break;
>> +	    }
>> +	}
>> +      break;
> 
> Hmm, you ignore all-NOPs sequences, but NOP followed by one of the cases
> below is still left as "bad". While I'm not going to insist that you go
> farther, it striks me as odd.
> 
>> +    case DW_CFA_expression:
>> +      return sframe_xlate_do_escape_expr (xlate_ctx, cfi_insn);
>> +
>> +    case DW_CFA_val_offset:
>> +      return sframe_xlate_do_escape_val_offset (xlate_ctx, cfi_insn);
>> +
>> +    /* FIXME - Also add processing for DW_CFA_GNU_args_size in future?  */
>> +
>> +    default:
>> +      err = SFRAME_XLATE_ERR_NOTREPRESENTED;
>> +      break;
>> +    }
>> +
>> +  if (err == SFRAME_XLATE_ERR_NOTREPRESENTED)
>> +    {
>> +      /* 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 (%#lx)"),
>> +	       (unsigned long)firstop);
> 
> Right now it's supposedly only bytes that you get to see here, so the
> truncation (on a 32-bit host with BFD64 in use) hopefully won't matter
> much.
> 
>> --- /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)
         .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