Re: [PATCH v2 dwarves 1/2] dwarf_loader: Check DW_OP_[GNU_]entry_value for possible parameter matching

[Jiri Olsa <olsajiri@gmail.com>]

On Thu, Nov 14, 2024 at 12:04:24PM -0800, Yonghong Song wrote:
> 
> 
> 
> On 11/14/24 10:21 AM, Eduard Zingerman wrote:
> > On Thu, 2024-11-14 at 08:51 -0800, Yonghong Song wrote:
> > 
> > [...]
> > 
> > > > +		/* match DW_OP_entry_value(DW_OP_regXX) at any location */
> > > > +		case DW_OP_entry_value:
> > > > +		case DW_OP_GNU_entry_value:
> > > > +			if (dwarf_getlocation_attr(attr, expr, &entry_attr) == 0 &&
> > > > +			    dwarf_getlocation(&entry_attr, &entry_ops, &entry_len) == 0 &&
> > > > +			    entry_len == 1) {
> > > > +				ret = entry_ops->atom;
> > > Could we have more than one DW_OP_entry_value? What if the second one
> > > matches execpted_reg? From dwarf5 documentation, there is no say about
> > > whether we could have more than one DW_OP_entry_value or not.
> > > 
> > > If we have evidence that only one DW_OP_entry_value will appear in parameter
> > > locations, a comment will be needed in the above.
> > > 
> > > Otherwise, let us not do 'goto out' here. Rather, let us compare
> > > entry_ops->atom with expected_reg. Do 'ret = entry_ops->atom' and
> > > 'goto out' only if entry_ops->atom == expected_reg. Otherwise,
> > > the original 'ret' value is preserved.
> > Basing on this description in lldb source:
> > https://github.com/llvm/llvm-project/blob/1cd981a5f3c89058edd61cdeb1efa3232b1f71e6/lldb/source/Expression/DWARFExpression.cpp#L538
> > It would be surprising if DW_OP_entry_value records had different expressions.
> > However, there are 50 instances of such behaviour in my clang 18.1.8 built kernel., e.g.:
> > 
> > 0x01f75d14:   DW_TAG_subprogram
> >                  DW_AT_low_pc    (0xffffffff818c43a0)
> >                  DW_AT_high_pc   (0xffffffff818c43c9)
> >                  DW_AT_frame_base        (DW_OP_reg7 RSP)
> >                  DW_AT_call_all_calls    (true)
> >                  DW_AT_name      ("hwcache_align_show")
> >                  DW_AT_decl_file ("/home/eddy/work/bpf-next/mm/slub.c")
> >                  DW_AT_decl_line (6621)
> >                  DW_AT_prototyped        (true)
> >                  DW_AT_type      (0x01f51a9b "ssize_t")
> > 
> > 0x01f75d26:     DW_TAG_formal_parameter
> >                    DW_AT_location        (indexed (0xa0f) loclist = 0x0062c64f:
> >                       [0xffffffff818c43a9, 0xffffffff818c43b5): DW_OP_reg5 RDI
> >                       [0xffffffff818c43b5, 0xffffffff818c43c1): DW_OP_entry_value(DW_OP_reg5 RDI), DW_OP_stack_value
> >                       [0xffffffff818c43c1, 0xffffffff818c43c9): DW_OP_entry_value(DW_OP_reg4 RSI), DW_OP_stack_value)
> >                    DW_AT_name    ("s")
> >                    DW_AT_decl_file       ("/home/eddy/work/bpf-next/mm/slub.c")
> >                    DW_AT_decl_line       (6621)
> >                    DW_AT_type    (0x01f4f449 "kmem_cache *")
> > 
> > The following change seem not to affect pahole execution time:
> > 
> > @@ -1234,7 +1234,8 @@ static int parameter__reg(Dwarf_Attribute *attr, int expected_reg)
> >                              dwarf_getlocation(&entry_attr, &entry_ops, &entry_len) == 0 &&
> >                              entry_len == 1) {
> >                                  ret = entry_ops->atom;
> > -                               goto out;
> > +                               if (expr->atom == expected_reg)
> > +                                       goto out;
> >                          }
> >                          break;
> >                  }
> 
> Should we do
> 			...
> 			dwarf_getlocation(&entry_attr, &entry_ops, &entry_len) == 0 &&
> 			entry_len == 1 && expr->atom == expected_reg) {
> 				ret = entry_ops->atom;
> 				goto out;
> 		}
> 		...
> ?

+1 

jirka

> 
> > 
> > This question aside, I think the changes fine.
> > 
> > [...]
> > 
>