[PATCH v2 08/10] perf dwarf-aux: Skip check_variable for die_find_variable_by_reg

[PATCH v2 08/10] perf dwarf-aux: Skip check_variable for die_find_variable_by_reg

[Zecheng Li]

In die_find_variable_by_reg, match_var_offset already performs
sufficient checking and type matching. The additional check_variable
call is redundant, and its need_pointer logic is only a heuristic. Since
DWARF encodes accurate type information, which match_var_offset
verifies, skipping check_variable improves both coverage and accuracy.

This change implements

- Return type from die_find_variable_by_reg via a new `type` field in
  find_var_data.

- In match_var_offset, use __die_get_real_type instead of
  die_get_real_type to preserve typedefs. Move the (offset == 0) branch
  after the is_pointer check to ensure the correct type is used, fixing
  cases where an incorrect pointer type was chosen when the access
  offset was 0.

- When comparing types from different scopes, first compare their type
  offsets. A larger offset means the field belongs to an outer
  (enclosing) struct. This helps resolve cases where a pointer is found
  in an inner scope, but a struct containing that pointer exists in an
  outer scope. Previously, is_better_type would prefer the pointer type,
  but the struct type is actually more complete and should be chosen.

Signed-off-by: Zecheng Li <zecheng@google.com>
---
 tools/perf/util/annotate-data.c | 12 ++++++++----
 tools/perf/util/dwarf-aux.c     | 30 +++++++++++++++++-------------
 tools/perf/util/dwarf-aux.h     |  2 +-
 3 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index e067e91f2037..b1a4b02afeb1 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -1551,19 +1551,21 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 			if (!die_find_variable_by_addr(&scopes[i], dloc->var_addr,
 						       &var_die, &type_offset))
 				continue;
+			/* Found a variable, see if it's correct */
+			result = check_variable(dloc, &var_die, &mem_die, reg,
+						type_offset, is_fbreg);
 		} else {
 			/* Look up variables/parameters in this scope */
 			if (!die_find_variable_by_reg(&scopes[i], pc, reg,
-						      &type_offset, is_fbreg, &var_die))
+						      &mem_die, &type_offset, is_fbreg, &var_die))
 				continue;
+			result = PERF_TMR_OK;
 		}
 
 		pr_debug_dtp("found \"%s\" (die: %#lx) in scope=%d/%d (die: %#lx) ",
 			     dwarf_diename(&var_die), (long)dwarf_dieoffset(&var_die),
 			     i+1, nr_scopes, (long)dwarf_dieoffset(&scopes[i]));
 
-		/* Found a variable, see if it's correct */
-		result = check_variable(dloc, &var_die, &mem_die, reg, type_offset, is_fbreg);
 		if (result == PERF_TMR_OK) {
 			if (reg == DWARF_REG_PC) {
 				pr_debug_dtp("addr=%#"PRIx64" type_offset=%#x\n",
@@ -1575,7 +1577,9 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 				pr_debug_dtp("type_offset=%#x\n", type_offset);
 			}
 
-			if (!found || is_better_type(type_die, &mem_die)) {
+			if (!found || dloc->type_offset < type_offset ||
+				(dloc->type_offset == type_offset &&
+				 is_better_type(type_die, &mem_die))) {
 				*type_die = mem_die;
 				dloc->type_offset = type_offset;
 				found = true;
diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index 6e8877ff2172..56e1b5690dc4 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -1378,6 +1378,8 @@ struct find_var_data {
 	Dwarf_Addr addr;
 	/* Target register */
 	unsigned reg;
+	/* Access data type */
+	Dwarf_Die type;
 	/* Access offset, set for global data */
 	int offset;
 	/* True if the current register is the frame base */
@@ -1390,29 +1392,28 @@ struct find_var_data {
 static bool match_var_offset(Dwarf_Die *die_mem, struct find_var_data *data,
 			     s64 addr_offset, s64 addr_type, bool is_pointer)
 {
-	Dwarf_Die type_die;
 	Dwarf_Word size;
 	s64 offset = addr_offset - addr_type;
 
-	if (offset == 0) {
-		/* Update offset relative to the start of the variable */
-		data->offset = 0;
-		return true;
-	}
-
 	if (offset < 0)
 		return false;
 
-	if (die_get_real_type(die_mem, &type_die) == NULL)
+	if (__die_get_real_type(die_mem, &data->type) == NULL)
 		return false;
 
-	if (is_pointer && dwarf_tag(&type_die) == DW_TAG_pointer_type) {
+	if (is_pointer && dwarf_tag(&data->type) == DW_TAG_pointer_type) {
 		/* Get the target type of the pointer */
-		if (die_get_real_type(&type_die, &type_die) == NULL)
+		if (__die_get_real_type(&data->type, &data->type) == NULL)
 			return false;
 	}
 
-	if (dwarf_aggregate_size(&type_die, &size) < 0)
+	if (offset == 0) {
+		/* Update offset relative to the start of the variable */
+		data->offset = 0;
+		return true;
+	}
+
+	if (dwarf_aggregate_size(&data->type, &size) < 0)
 		return false;
 
 	if ((u64)offset >= size)
@@ -1529,7 +1530,7 @@ static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
  * when the variable is in the stack.
  */
 Dwarf_Die *die_find_variable_by_reg(Dwarf_Die *sc_die, Dwarf_Addr pc, int reg,
-				    int *poffset, bool is_fbreg,
+				    Dwarf_Die *type_die, int *poffset, bool is_fbreg,
 				    Dwarf_Die *die_mem)
 {
 	struct find_var_data data = {
@@ -1541,8 +1542,11 @@ Dwarf_Die *die_find_variable_by_reg(Dwarf_Die *sc_die, Dwarf_Addr pc, int reg,
 	Dwarf_Die *result;
 
 	result = die_find_child(sc_die, __die_find_var_reg_cb, &data, die_mem);
-	if (result)
+	if (result) {
 		*poffset = data.offset;
+		*type_die = data.type;
+	}
+
 	return result;
 }
 
diff --git a/tools/perf/util/dwarf-aux.h b/tools/perf/util/dwarf-aux.h
index f20319eb97a9..beb6926aaa71 100644
--- a/tools/perf/util/dwarf-aux.h
+++ b/tools/perf/util/dwarf-aux.h
@@ -167,7 +167,7 @@ int die_get_var_range(Dwarf_Die *sp_die, Dwarf_Die *vr_die, struct strbuf *buf);
 
 /* Find a variable saved in the 'reg' at given address */
 Dwarf_Die *die_find_variable_by_reg(Dwarf_Die *sc_die, Dwarf_Addr pc, int reg,
-				    int *poffset, bool is_fbreg,
+				    Dwarf_Die *type_die, int *poffset, bool is_fbreg,
 				    Dwarf_Die *die_mem);
 
 /* Find a (global) variable located in the 'addr' */
-- 
2.51.0.261.g7ce5a0a67e-goog

Re: [PATCH v2 08/10] perf dwarf-aux: Skip check_variable for die_find_variable_by_reg

[Namhyung Kim]

On Mon, Aug 25, 2025 at 07:58:06PM +0000, Zecheng Li wrote:
> In die_find_variable_by_reg, match_var_offset already performs
> sufficient checking and type matching. The additional check_variable
> call is redundant, and its need_pointer logic is only a heuristic. Since
> DWARF encodes accurate type information, which match_var_offset
> verifies, skipping check_variable improves both coverage and accuracy.
> 
> This change implements

Looks like you can split these into separate commits.

> 
> - Return type from die_find_variable_by_reg via a new `type` field in
>   find_var_data.

Sounds ok.

> 
> - In match_var_offset, use __die_get_real_type instead of
>   die_get_real_type to preserve typedefs. Move the (offset == 0) branch

Why do you want to preserve typedefs?  I think a variable type can be
typedef to a pointer then now it won't resolve that target type anymore.


>   after the is_pointer check to ensure the correct type is used, fixing
>   cases where an incorrect pointer type was chosen when the access
>   offset was 0.
> 
> - When comparing types from different scopes, first compare their type
>   offsets. A larger offset means the field belongs to an outer
>   (enclosing) struct. This helps resolve cases where a pointer is found
>   in an inner scope, but a struct containing that pointer exists in an
>   outer scope. Previously, is_better_type would prefer the pointer type,
>   but the struct type is actually more complete and should be chosen.

Can we improve is_better_type() then?

Thanks,
Namhyung

> 
> Signed-off-by: Zecheng Li <zecheng@google.com>
> ---
>  tools/perf/util/annotate-data.c | 12 ++++++++----
>  tools/perf/util/dwarf-aux.c     | 30 +++++++++++++++++-------------
>  tools/perf/util/dwarf-aux.h     |  2 +-
>  3 files changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> index e067e91f2037..b1a4b02afeb1 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c
> @@ -1551,19 +1551,21 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
>  			if (!die_find_variable_by_addr(&scopes[i], dloc->var_addr,
>  						       &var_die, &type_offset))
>  				continue;
> +			/* Found a variable, see if it's correct */
> +			result = check_variable(dloc, &var_die, &mem_die, reg,
> +						type_offset, is_fbreg);
>  		} else {
>  			/* Look up variables/parameters in this scope */
>  			if (!die_find_variable_by_reg(&scopes[i], pc, reg,
> -						      &type_offset, is_fbreg, &var_die))
> +						      &mem_die, &type_offset, is_fbreg, &var_die))
>  				continue;
> +			result = PERF_TMR_OK;
>  		}
>  
>  		pr_debug_dtp("found \"%s\" (die: %#lx) in scope=%d/%d (die: %#lx) ",
>  			     dwarf_diename(&var_die), (long)dwarf_dieoffset(&var_die),
>  			     i+1, nr_scopes, (long)dwarf_dieoffset(&scopes[i]));
>  
> -		/* Found a variable, see if it's correct */
> -		result = check_variable(dloc, &var_die, &mem_die, reg, type_offset, is_fbreg);
>  		if (result == PERF_TMR_OK) {
>  			if (reg == DWARF_REG_PC) {
>  				pr_debug_dtp("addr=%#"PRIx64" type_offset=%#x\n",
> @@ -1575,7 +1577,9 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
>  				pr_debug_dtp("type_offset=%#x\n", type_offset);
>  			}
>  
> -			if (!found || is_better_type(type_die, &mem_die)) {
> +			if (!found || dloc->type_offset < type_offset ||
> +				(dloc->type_offset == type_offset &&
> +				 is_better_type(type_die, &mem_die))) {
>  				*type_die = mem_die;
>  				dloc->type_offset = type_offset;
>  				found = true;
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index 6e8877ff2172..56e1b5690dc4 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
> @@ -1378,6 +1378,8 @@ struct find_var_data {
>  	Dwarf_Addr addr;
>  	/* Target register */
>  	unsigned reg;
> +	/* Access data type */
> +	Dwarf_Die type;
>  	/* Access offset, set for global data */
>  	int offset;
>  	/* True if the current register is the frame base */
> @@ -1390,29 +1392,28 @@ struct find_var_data {
>  static bool match_var_offset(Dwarf_Die *die_mem, struct find_var_data *data,
>  			     s64 addr_offset, s64 addr_type, bool is_pointer)
>  {
> -	Dwarf_Die type_die;
>  	Dwarf_Word size;
>  	s64 offset = addr_offset - addr_type;
>  
> -	if (offset == 0) {
> -		/* Update offset relative to the start of the variable */
> -		data->offset = 0;
> -		return true;
> -	}
> -
>  	if (offset < 0)
>  		return false;
>  
> -	if (die_get_real_type(die_mem, &type_die) == NULL)
> +	if (__die_get_real_type(die_mem, &data->type) == NULL)
>  		return false;
>  
> -	if (is_pointer && dwarf_tag(&type_die) == DW_TAG_pointer_type) {
> +	if (is_pointer && dwarf_tag(&data->type) == DW_TAG_pointer_type) {
>  		/* Get the target type of the pointer */
> -		if (die_get_real_type(&type_die, &type_die) == NULL)
> +		if (__die_get_real_type(&data->type, &data->type) == NULL)
>  			return false;
>  	}
>  
> -	if (dwarf_aggregate_size(&type_die, &size) < 0)
> +	if (offset == 0) {
> +		/* Update offset relative to the start of the variable */
> +		data->offset = 0;
> +		return true;
> +	}
> +
> +	if (dwarf_aggregate_size(&data->type, &size) < 0)
>  		return false;
>  
>  	if ((u64)offset >= size)
> @@ -1529,7 +1530,7 @@ static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
>   * when the variable is in the stack.
>   */
>  Dwarf_Die *die_find_variable_by_reg(Dwarf_Die *sc_die, Dwarf_Addr pc, int reg,
> -				    int *poffset, bool is_fbreg,
> +				    Dwarf_Die *type_die, int *poffset, bool is_fbreg,
>  				    Dwarf_Die *die_mem)
>  {
>  	struct find_var_data data = {
> @@ -1541,8 +1542,11 @@ Dwarf_Die *die_find_variable_by_reg(Dwarf_Die *sc_die, Dwarf_Addr pc, int reg,
>  	Dwarf_Die *result;
>  
>  	result = die_find_child(sc_die, __die_find_var_reg_cb, &data, die_mem);
> -	if (result)
> +	if (result) {
>  		*poffset = data.offset;
> +		*type_die = data.type;
> +	}
> +
>  	return result;
>  }
>  
> diff --git a/tools/perf/util/dwarf-aux.h b/tools/perf/util/dwarf-aux.h
> index f20319eb97a9..beb6926aaa71 100644
> --- a/tools/perf/util/dwarf-aux.h
> +++ b/tools/perf/util/dwarf-aux.h
> @@ -167,7 +167,7 @@ int die_get_var_range(Dwarf_Die *sp_die, Dwarf_Die *vr_die, struct strbuf *buf);
>  
>  /* Find a variable saved in the 'reg' at given address */
>  Dwarf_Die *die_find_variable_by_reg(Dwarf_Die *sc_die, Dwarf_Addr pc, int reg,
> -				    int *poffset, bool is_fbreg,
> +				    Dwarf_Die *type_die, int *poffset, bool is_fbreg,
>  				    Dwarf_Die *die_mem);
>  
>  /* Find a (global) variable located in the 'addr' */
> -- 
> 2.51.0.261.g7ce5a0a67e-goog
> 

Re: [PATCH v2 08/10] perf dwarf-aux: Skip check_variable for die_find_variable_by_reg

[Zecheng Li]

On Sat, Aug 30, 2025 at 3:31 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> > - In match_var_offset, use __die_get_real_type instead of
> >   die_get_real_type to preserve typedefs. Move the (offset == 0) branch
>
> Why do you want to preserve typedefs?  I think a variable type can be
> typedef to a pointer then now it won't resolve that target type anymore.

check_variable preserves the typedefs. It would sometimes resolve to
an unnamed struct if we remove the typedefs. Let me test if it will
affect the dwarf_tag(&data->type) == DW_TAG_pointer_type check. Also I
found calling dwarf_aggregate_size on typedef'd types gives the
correct result, so maybe we don't need the sized_type in
check_variable?

> > - When comparing types from different scopes, first compare their type
> >   offsets. A larger offset means the field belongs to an outer
> >   (enclosing) struct. This helps resolve cases where a pointer is found
> >   in an inner scope, but a struct containing that pointer exists in an
> >   outer scope. Previously, is_better_type would prefer the pointer type,
> >   but the struct type is actually more complete and should be chosen.
>
> Can we improve is_better_type() then?

Here we are comparing two types with the extra access offset
information. In other contexts, the calls to is_better_type compares
two types only, so I think we don't need to add two new parameters to
is_better_type?

> > -                     if (!found || is_better_type(type_die, &mem_die)) {
> > +                     if (!found || dloc->type_offset < type_offset ||
> > +                             (dloc->type_offset == type_offset &&
> > +                              is_better_type(type_die, &mem_die))) {
> >                               *type_die = mem_die;
> >                               dloc->type_offset = type_offset;
> >                               found = true;

I find changing the is_better_type call to !is_better_type(&mem_die,
type_die) would yield better results: prefer types from outer scope if
the current one is not "better" than the new one.

Re: [PATCH v2 08/10] perf dwarf-aux: Skip check_variable for die_find_variable_by_reg

[Namhyung Kim]

On Wed, Sep 03, 2025 at 05:23:01PM -0400, Zecheng Li wrote:
> On Sat, Aug 30, 2025 at 3:31 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > > - In match_var_offset, use __die_get_real_type instead of
> > >   die_get_real_type to preserve typedefs. Move the (offset == 0) branch
> >
> > Why do you want to preserve typedefs?  I think a variable type can be
> > typedef to a pointer then now it won't resolve that target type anymore.
> 
> check_variable preserves the typedefs. It would sometimes resolve to
> an unnamed struct if we remove the typedefs. 

Ah, that's unfortunate.

> Let me test if it will
> affect the dwarf_tag(&data->type) == DW_TAG_pointer_type check. Also I
> found calling dwarf_aggregate_size on typedef'd types gives the
> correct result, so maybe we don't need the sized_type in
> check_variable?

You're right.

> 
> > > - When comparing types from different scopes, first compare their type
> > >   offsets. A larger offset means the field belongs to an outer
> > >   (enclosing) struct. This helps resolve cases where a pointer is found
> > >   in an inner scope, but a struct containing that pointer exists in an
> > >   outer scope. Previously, is_better_type would prefer the pointer type,
> > >   but the struct type is actually more complete and should be chosen.
> >
> > Can we improve is_better_type() then?
> 
> Here we are comparing two types with the extra access offset
> information. In other contexts, the calls to is_better_type compares
> two types only, so I think we don't need to add two new parameters to
> is_better_type?

Right, I meant just about pointer type and struct type.  It compares two
types take the same location so I didn't expect they can be a pointer
and a struct.  My intention was about a pointer and a base type.

Also you may consider typedef and struct.  I think we prefer struct
since it can access the member field.  But as you said we should use
typedef if it's an unnamed struct.  It'd be great if we can get members
even for typedefs (for structs).

> 
> > > -                     if (!found || is_better_type(type_die, &mem_die)) {
> > > +                     if (!found || dloc->type_offset < type_offset ||
> > > +                             (dloc->type_offset == type_offset &&
> > > +                              is_better_type(type_die, &mem_die))) {
> > >                               *type_die = mem_die;
> > >                               dloc->type_offset = type_offset;
> > >                               found = true;
> 
> I find changing the is_better_type call to !is_better_type(&mem_die,
> type_die) would yield better results: prefer types from outer scope if
> the current one is not "better" than the new one.

Ok, sounds good.

Thanks,
Namhyung