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

[Indu Bhagat]

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.

> Also (nit): "less than 128" or "at most 127".
> 

Thanks.

>> +  reg = items[0];
>> +#undef CFI_ESC_NUM_EXP
>> +
>> +  if (reg == SFRAME_CFA_SP_REG || reg == SFRAME_CFA_FP_REG
>> +#ifdef SFRAME_FRE_RA_TRACKING
>> +      || (sframe_ra_tracking_p () && reg == SFRAME_CFA_RA_REG)
>> +#endif
>> +      || reg == cur_fre->cfa_base_reg)
> 
> This looks to be the only use of "cur_fre". Is the local variable really
> worth having?
> 

Probably not. Removed from V4.

>> +    {
>> +      as_warn (_("skipping SFrame FDE; "
>> +		 ".cfi_escape DW_CFA_expression with %s reg %u"),
>> +	       sframe_register_name (reg), reg);
> 
> Won't the %s result in printing of <null> or whatever the underlying libc
> uses when only the last of the conditions resulted in us getting here?
> That's not a crash, sure, but also not really a good diagnostic.
> 

I too noticed that.  I will see if something can be done about this later.

>> +      err = SFRAME_XLATE_ERR_NOTREPRESENTED;
>> +    }
>> +
>> +  return err;
>> +}
>> +
>> +/* Handle DW_CFA_val_offset 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_val_offset (const struct sframe_xlate_ctx *xlate_ctx,
>> +				   const struct cfi_insn_data *cfi_insn)
>> +{
>> +  const struct cfi_escape_data *e = cfi_insn->u.esc;
>> +  int err = SFRAME_XLATE_OK;
>> +  unsigned int i = 0;
>> +  unsigned int reg;
>> +  offsetT offset;
>> +
>> +  /* 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 || 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;
>> +
>> +  /* 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.  However, since we limit our focus on cases when
>> +     CFI_ESC_NUM_EXP is 2, reading UELB can be skipped.  IOW, although not
> 
> Nit: ULEB
> 

Fixed.

>> +     ideal, SFrame FDE generation in case of an APX register in
>> +     DW_CFA_val_offset is being skipped (PS: this does _not_ mean incorrect
>> +     SFrame stack trace data).
>> +
>> +     Recall that the intent here is to check for simple and prevalent cases,
>> +     when feasible.  */
> 
> This more extensive comment may want referencing from the much less
> informative one in sframe_xlate_do_escape_expr().
> 

OK. Added.

>> +  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 = { .insn = DW_CFA_val_offset };
>> +  temp.u.ri.reg = reg;
>> +  temp.u.ri.offset = offset * DWARF2_CIE_DATA_ALIGNMENT;
> 
> I'm curious: Why partly by initializer and partly by assignments?
> 

I guess because old habits die hard :)
I switched to your original version:

   struct cfi_insn_data temp = {
     .insn = DW_CFA_val_offset,
     .u = {
       .ri = {
         .reg = reg,
         .offset = offset * DWARF2_CIE_DATA_ALIGNMENT
       }
     }
   };

>> +  err = sframe_xlate_do_val_offset (xlate_ctx, &temp, true);
>> +
>> +  return err;
>> +}
>> +
>> +/* Handle CFI_escape in SFrame context.
>> +
>> +   .cfi_escape CFI directive allows the user to add arbitrary bytes to the
> 
> In order for this to not go stale very quickly, may I ask to use "data"
> instead of "bytes"?
> 

OK.

>> +   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
> 
> Again here then.
> 

OK.

>> +   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.  */
>> +
>> +static int
>> +sframe_xlate_do_cfi_escape (const struct sframe_xlate_ctx *xlate_ctx,
>> +			    const struct cfi_insn_data *cfi_insn)
>> +{
>> +  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;
>> +
>> +  if (e->exp.X_op != O_constant)
>> +    return SFRAME_XLATE_ERR_NOTREPRESENTED;
> 
> Shouldn't this also wire to the as_warn() at the bottom of the function?
> 

It should.  Fixed in V4.  It seems I also needed to wire some paths in 
sframe_xlate_do_escape_expr and sframe_xlate_do_escape_val_offset too.

>> +  firstop = e->exp.X_add_number;
>> +  switch (firstop)
>> +    {
>> +    case DW_CFA_nop:
>> +      /* One or more nops together are harmless for SFrame.  */
>> +      while (e->next)
>> +	{
>> +	  e = e->next;
>> +	  if (e->exp.X_op != O_constant || e->exp.X_add_number != DW_CFA_nop)
>> +	    {
>> +	      err = SFRAME_XLATE_ERR_NOTREPRESENTED;
>> +	      break;
>> +	    }
>> +	}
>> +      break;
>> +
>> +    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);
> 
> Similarly for the two calls: Shouldn't SFRAME_XLATE_ERR_NOTREPRESENTED
> coming back also cause the warning to be issued?
> 

Right.  Fixed in V4. Thanks.

I also added a testcase where multiple valid DWARF expr follow one 
.cfi_escape to help keep some of this tested.

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

Thanks
Indu