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. > > > > [...] > > >