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

Jiri Olsa
2024-11-11T09:54:19Z

On Fri, Nov 08, 2024 at 10:05:24AM -0800, Yonghong Song wrote:
> Song Liu reported that a kernel func (perf_event_read()) cannot be traced
> in certain situations since the func is not in vmlinux bTF. This happens
> in kernels 6.4, 6.9 and 6.11 and the kernel is built with pahole 1.27.
>
> The perf_event_read() signature in kernel (kernel/events/core.c):
> static int perf_event_read(struct perf_event *event, bool group)
>
> Adding '-V' to pahole command line, and the following error msg can be found:
> skipping addition of 'perf_event_read'(perf_event_read) due to unexpected register used for parameter
>
> Eventually the error message is attributed to the setting
> (parm->unexpected_reg = 1) in parameter__new() function.
>
> The following is the dwarf representation for perf_event_read():
> 0x0334c034: DW_TAG_subprogram
> DW_AT_low_pc (0xffffffff812c6110)
> DW_AT_high_pc (0xffffffff812c640a)
> DW_AT_frame_base (DW_OP_reg7 RSP)
> DW_AT_GNU_all_call_sites (true)
> DW_AT_name ("perf_event_read")
> DW_AT_decl_file ("/rw/compile/kernel/events/core.c")
> DW_AT_decl_line (4641)
> DW_AT_prototyped (true)
> DW_AT_type (0x03324f6a "int")
> 0x0334c04e: DW_TAG_formal_parameter
> DW_AT_location (0x007de9fd:
> [0xffffffff812c6115, 0xffffffff812c6141): DW_OP_reg5 RDI
> [0xffffffff812c6141, 0xffffffff812c6323): DW_OP_reg14 R14
> [0xffffffff812c6323, 0xffffffff812c63fe): DW_OP_GNU_entry_value(DW_OP_reg5 RDI), DW_OP_stack_value
> [0xffffffff812c63fe, 0xffffffff812c6405): DW_OP_reg14 R14
> [0xffffffff812c6405, 0xffffffff812c640a): DW_OP_GNU_entry_value(DW_OP_reg5 RDI), DW_OP_stack_value)
> DW_AT_name ("event")
> DW_AT_decl_file ("/rw/compile/kernel/events/core.c")
> DW_AT_decl_line (4641)
> DW_AT_type (0x0333aac2 "perf_event *")
> 0x0334c05e: DW_TAG_formal_parameter
> DW_AT_location (0x007dea82:
> [0xffffffff812c6137, 0xffffffff812c63f2): DW_OP_reg12 R12
> [0xffffffff812c63f2, 0xffffffff812c63fe): DW_OP_GNU_entry_value(DW_OP_reg4 RSI), DW_OP_stack_value
> [0xffffffff812c63fe, 0xffffffff812c640a): DW_OP_reg12 R12)
> DW_AT_name ("group")
> DW_AT_decl_file ("/rw/compile/kernel/events/core.c")
> DW_AT_decl_line (4641)
> DW_AT_type (0x03327059 "bool")

hi,
I don't see that on gcc compiled kernel, is that related to clang?

<1><318d475>: Abbrev Number: 74 (DW_TAG_subprogram)
<318d476> DW_AT_name : (indirect string, offset: 0xf5776): perf_event_read
<318d47a> DW_AT_decl_file : 1
<318d47a> DW_AT_decl_line : 4746
<318d47c> DW_AT_decl_column : 12
<318d47d> DW_AT_prototyped : 1
<318d47d> DW_AT_type : <0x3135e35>
<318d481> DW_AT_low_pc : 0xffffffff8135be90
<318d489> DW_AT_high_pc : 0x196
<318d491> DW_AT_frame_base : 1 byte block: 9c (DW_OP_call_frame_cfa)
<318d493> DW_AT_call_all_calls: 1
<318d493> DW_AT_sibling : <0x318d900>
<2><318d497>: Abbrev Number: 30 (DW_TAG_formal_parameter)
<318d498> DW_AT_name : (indirect string, offset: 0x491590): event
<318d49c> DW_AT_decl_file : 1
<318d49c> DW_AT_decl_line : 4746
<318d49e> DW_AT_decl_column : 47
<318d49f> DW_AT_type : <0x313a680>
<318d4a3> DW_AT_location : 0x70c118 (location list)
<318d4a7> DW_AT_GNU_locviews: 0x70c110
<2><318d4ab>: Abbrev Number: 30 (DW_TAG_formal_parameter)
<318d4ac> DW_AT_name : (indirect string, offset: 0x51a865): group
<318d4b0> DW_AT_decl_file : 1
<318d4b0> DW_AT_decl_line : 4746
<318d4b2> DW_AT_decl_column : 59
<318d4b3> DW_AT_type : <0x3136055>
<318d4b7> DW_AT_location : 0x70c144 (location list)
<318d4bb> DW_AT_GNU_locviews: 0x70c13e

locations:
0070c144 ffffffff8135be90 (base address)
0070c14d v000000000000000 v000000000000000 views at 0070c13e for:
ffffffff8135be90 ffffffff8135bed2 (DW_OP_reg4 (rsi))
0070c152 v000000000000000 v000000000000000 views at 0070c140 for:
ffffffff8135bed2 ffffffff8135bf17 (DW_OP_reg14 (r14))
0070c158 v000000000000000 v000000000000000 views at 0070c142 for:
ffffffff8135bf17 ffffffff8135c026 (DW_OP_entry_value: (DW_OP_reg4 (rsi)); DW_OP_stack_value)
0070c162 <End of list>

other than that lgtm and I like the change Eduard suggested

thanks,
jirka

>
> By inspecting the binary, the second argument ("bool group") is used
> in the function. The following are the disasm code:
> ffffffff812c6110 <perf_event_read>:
> ffffffff812c6110: 0f 1f 44 00 00 nopl (%rax,%rax)
> ffffffff812c6115: 55 pushq %rbp
> ffffffff812c6116: 41 57 pushq %r15
> ffffffff812c6118: 41 56 pushq %r14
> ffffffff812c611a: 41 55 pushq %r13
> ffffffff812c611c: 41 54 pushq %r12
> ffffffff812c611e: 53 pushq %rbx
> ffffffff812c611f: 48 83 ec 18 subq $24, %rsp
> ffffffff812c6123: 41 89 f4 movl %esi, %r12d
> <=========== NOTE that here '%esi' is used and moved to '%r12d'.
> ffffffff812c6126: 49 89 fe movq %rdi, %r14
> ffffffff812c6129: 65 48 8b 04 25 28 00 00 00 movq %gs:40, %rax
> ffffffff812c6132: 48 89 44 24 10 movq %rax, 16(%rsp)
> ffffffff812c6137: 8b af a8 00 00 00 movl 168(%rdi), %ebp
> ffffffff812c613d: 85 ed testl %ebp, %ebp
> ffffffff812c613f: 75 3f jne 0xffffffff812c6180 <perf_event_read+0x70>
> ffffffff812c6141: 66 2e 0f 1f 84 00 00 00 00 00 nopw %cs:(%rax,%rax)
> ffffffff812c614b: 0f 1f 44 00 00 nopl (%rax,%rax)
> ffffffff812c6150: 49 8b 9e 28 02 00 00 movq 552(%r14), %rbx
> ffffffff812c6157: 48 89 df movq %rbx, %rdi
> ffffffff812c615a: e8 c1 a0 d7 00 callq 0xffffffff82040220 <_raw_spin_lock_irqsave>
> ffffffff812c615f: 49 89 c7 movq %rax, %r15
> ffffffff812c6162: 41 8b ae a8 00 00 00 movl 168(%r14), %ebp
> ffffffff812c6169: 85 ed testl %ebp, %ebp
> ffffffff812c616b: 0f 84 9a 00 00 00 je 0xffffffff812c620b <perf_event_read+0xfb>
> ffffffff812c6171: 48 89 df movq %rbx, %rdi
> ffffffff812c6174: 4c 89 fe movq %r15, %rsi
> <=========== NOTE: %rsi is overwritten
> ......
> ffffffff812c63f0: 41 5c popq %r12
> <============ POP r12
> ffffffff812c63f2: 41 5d popq %r13
> ffffffff812c63f4: 41 5e popq %r14
> ffffffff812c63f6: 41 5f popq %r15
> ffffffff812c63f8: 5d popq %rbp
> ffffffff812c63f9: e9 e2 a8 d7 00 jmp 0xffffffff82040ce0 <__x86_return_thunk>
> ffffffff812c63fe: 31 c0 xorl %eax, %eax
> ffffffff812c6400: e9 be fe ff ff jmp 0xffffffff812c62c3 <perf_event_read+0x1b3>
>
> It is not clear why dwarf didn't encode %rsi in locations. But
> DW_OP_GNU_entry_value(DW_OP_reg4 RSI) tells us that RSI is live at
> the entry of perf_event_read(). So this patch tries to search
> DW_OP_GNU_entry_value/DW_OP_entry_value location/expression so if
> the expected parameter register matchs the register in
> DW_OP_GNU_entry_value/DW_OP_entry_value, then the original parameter
> is not optimized.
>
> For one of internal 6.11 kernel, there are 62498 functions in BTF and
> perf_event_read() is not there. With this patch, there are 61552 functions
> in BTF and perf_event_read() is included.
>
> Reported-by: Song Liu <song@kernel.org>
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---
> dwarf_loader.c | 81 +++++++++++++++++++++++++++++++++++---------------
> 1 file changed, 57 insertions(+), 24 deletions(-)
>
> diff --git a/dwarf_loader.c b/dwarf_loader.c
> index e0b8c11..1fe44bc 100644
> --- a/dwarf_loader.c
> +++ b/dwarf_loader.c
> @@ -1169,34 +1169,67 @@ static bool check_dwarf_locations(Dwarf_Attribute *attr, struct parameter *parm,
> return false;
>
> #if _ELFUTILS_PREREQ(0, 157)
> - /* dwarf_getlocations() handles location lists; here we are
> - * only interested in the first expr.
> - */
> - if (dwarf_getlocations(attr, 0, &base, &start, &end,
> - &loc.expr, &loc.exprlen) > 0 &&
> - loc.exprlen != 0) {
> - expr = loc.expr;
> -
> - switch (expr->atom) {
> - case DW_OP_reg0 ... DW_OP_reg31:
> - /* mark parameters that use an unexpected
> - * register to hold a parameter; these will
> - * be problematic for users of BTF as they
> - * violate expectations about register
> - * contents.
> + bool reg_matched = false, reg_unmatched = false, first_expr_reg = false, ret = false;
> + ptrdiff_t offset = 0;
> + int loc_num = -1;
> +
> + while ((offset = dwarf_getlocations(attr, offset, &base, &start, &end, &loc.expr, &loc.exprlen)) > 0 &&
> + loc.exprlen != 0) {
> + ret = true;
> + loc_num++;
> +
> + for (int i = 0; i < loc.exprlen; i++) {
> + Dwarf_Attribute entry_attr;
> + Dwarf_Op *entry_ops;
> + size_t entry_len;
> +
> + expr = &loc.expr[i];
> + switch (expr->atom) {
> + case DW_OP_reg0 ... DW_OP_reg31:
> + /* first location, first expression */
> + if (loc_num == 0 && i == 0) {
> + if (expected_reg >= 0) {
> + if (expected_reg == expr->atom) {
> + reg_matched = true;
> + return true;
> + } else {
> + reg_unmatched = true;
> + }
> + }
> + first_expr_reg = true;
> + }
> + break;
> + /* For the following dwarf entry (arch x86_64) in parameter locations:
> + * DW_OP_GNU_entry_value(DW_OP_reg4 RSI), DW_OP_stack_value
> + * RSI register should be available at the entry of the program.
> */
> - if (expected_reg >= 0 && expected_reg != expr->atom)
> - parm->unexpected_reg = 1;
> - break;
> - default:
> - parm->optimized = 1;
> - break;
> + case DW_OP_entry_value:
> + case DW_OP_GNU_entry_value:
> + if (reg_matched)
> + break;
> + if (dwarf_getlocation_attr (attr, expr, &entry_attr) != 0)
> + break;
> + if (dwarf_getlocation (&entry_attr, &entry_ops, &entry_len) != 0)
> + break;
> + if (entry_len != 1)
> + break;
> + if (expected_reg >= 0 && expected_reg == entry_ops->atom) {
> + reg_matched = true;
> + return true;
> + }
> + break;
> + default:
> + break;
> + }
> }
> -
> - return true;
> }
>
> - return false;
> + if (reg_unmatched)
> + parm->unexpected_reg = 1;
> + else if (ret && !first_expr_reg)
> + parm->optimized = 1;
> +
> + return ret;
> #else
> if (dwarf_getlocation(attr, &loc.expr, &loc.exprlen) == 0 &&
> loc.exprlen != 0) {
> --
> 2.43.5
>
>