Re: [PATCH v2 02/10] perf dwarf-aux: More accurate variable type match for breg

[Namhyung Kim <namhyung@kernel.org>]

On Mon, Aug 25, 2025 at 07:54:04PM +0000, Zecheng Li wrote:
> Introduces the function is_breg_access_indirect to determine whether a
> memory access involving a DW_OP_breg* operation refers to the variable's
> value directly or requires dereferencing the variable's type as a
> pointer based on the DWARF expression. Previously, all breg based
> accesses were assumed to directly access the variable's value
> (is_pointer = false).
> 
> The is_breg_access_indirect function handles three cases:
> 
> 1. Base register + offset only: (e.g., DW_OP_breg7 RSP+88) The
>    calculated address is the location of the variable. The access is
>    direct, so no type dereference is needed. Returns false.

I'm afraid there may be cases that the base register doesn't point to
the stack.  In that case it may return true, right?

I think struct find_var_data already has 'is_fbreg' field.  Maybe you
can add 'is_stack' or 'is_stack_reg' field if the target.  Currently we
hardcoded X86_REG_SP but it should be arch-dependent.

> 
> 2. Base register + offset, followed by other operations ending in
>    DW_OP_stack_value, including DW_OP_deref: (e.g., DW_OP_breg*,
>    DW_OP_deref, DW_OP_stack_value) The DWARF expression computes the
>    variable's value, but that value requires a dereference. The memory
>    access is fetching that value, so no type dereference is needed.
>    Returns false.
> 
> 3. Base register + offset, followed only by DW_OP_stack_value: (e.g.,
>    DW_OP_breg13 R13+256, DW_OP_stack_value) This indicates the value at
>    the base + offset is the variable's value. Since this value is being
>    used as an address in the memory access, the variable's type is
>    treated as a pointer and requires a type dereference. Returns true.
> 
> The is_pointer argument passed to match_var_offset is now set by
> is_breg_access_indirect for breg accesses.
> 
> There are more complex expressions that includes multiple operations and
> may require additional handling, such as DW_OP_deref without a
> DW_OP_stack_value, or including multiple base registers. They are less
> common in the Linux kernel dwarf and are skipped in check_allowed_ops.
> 
> Signed-off-by: Zecheng Li <zecheng@google.com>
> ---
>  tools/perf/util/dwarf-aux.c | 38 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index 920054425578..449bc9ad7aff 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
> @@ -1423,6 +1423,34 @@ static bool match_var_offset(Dwarf_Die *die_mem, struct find_var_data *data,
>  	return true;
>  }
>  
> +/**
> + * is_breg_access_indirect - Check if breg based access implies type
> + * dereference
> + * @ops: DWARF operations array
> + * @nops: Number of operations in @ops
> + *
> + * Returns true if the DWARF expression evaluates to the variable's
> + * value, so the memory access on that register needs type dereference.
> + * Returns false if the expression evaluates to the variable's address.
> + * This is called after check_allowed_ops.
> + */
> +static bool is_breg_access_indirect(Dwarf_Op *ops, size_t nops)
> +{
> +	/* only the base register */
> +	if (nops == 1)
> +		return false;

Then it could be like below:

	if (nops == 1) {
		int reg = reg_from_dwarf_op(ops);
		return !(reg == DWARF_REG_FB || data->is_fbreg || reg == data->is_stack);
	}

Thanks,
Namhyung

> +
> +	if (nops == 2 && ops[1].atom == DW_OP_stack_value)
> +		return true;
> +
> +	if (nops == 3 && (ops[1].atom == DW_OP_deref ||
> +		ops[1].atom == DW_OP_deref_size) &&
> +		ops[2].atom == DW_OP_stack_value)
> +		return false;
> +	/* unreachable, OP not supported */
> +	return false;
> +}
> +
>  /* Only checks direct child DIEs in the given scope. */
>  static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
>  {
> @@ -1451,7 +1479,7 @@ static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
>  		if (data->is_fbreg && ops->atom == DW_OP_fbreg &&
>  		    check_allowed_ops(ops, nops) &&
>  		    match_var_offset(die_mem, data, data->offset, ops->number,
> -				     /*is_pointer=*/false))
> +				     is_breg_access_indirect(ops, nops)))
>  			return DIE_FIND_CB_END;
>  
>  		/* Only match with a simple case */
> @@ -1463,11 +1491,11 @@ static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
>  					     /*is_pointer=*/true))
>  				return DIE_FIND_CB_END;
>  
> -			/* Local variables accessed by a register + offset */
> +			/* variables accessed by a register + offset */
>  			if (ops->atom == (DW_OP_breg0 + data->reg) &&
>  			    check_allowed_ops(ops, nops) &&
>  			    match_var_offset(die_mem, data, data->offset, ops->number,
> -					     /*is_pointer=*/false))
> +					     is_breg_access_indirect(ops, nops)))
>  				return DIE_FIND_CB_END;
>  		} else {
>  			/* pointer variables saved in a register 32 or above */
> @@ -1477,11 +1505,11 @@ static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
>  					     /*is_pointer=*/true))
>  				return DIE_FIND_CB_END;
>  
> -			/* Local variables accessed by a register + offset */
> +			/* variables accessed by a register + offset */
>  			if (ops->atom == DW_OP_bregx && data->reg == ops->number &&
>  			    check_allowed_ops(ops, nops) &&
>  			    match_var_offset(die_mem, data, data->offset, ops->number2,
> -					     /*is_poitner=*/false))
> +					     is_breg_access_indirect(ops, nops)))
>  				return DIE_FIND_CB_END;
>  		}
>  	}
> -- 
> 2.51.0.261.g7ce5a0a67e-goog
>