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

[Namhyung Kim <namhyung@kernel.org>]

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