[PATCH 1/2] perf probe: Reset old content before processing the next event string

[PATCH 1/2] perf probe: Reset old content before processing the next event string

[Li Huafei]

I added two probe events:

  # perf probe -f -a schedule+8
  Added new event:
    probe:schedule       (on schedule+8)

  You can now use it in all perf tools, such as:

          perf record -e probe:schedule -aR sleep 1

  # perf probe -f -a schedule+20
  Added new event:
    probe:schedule_1     (on schedule+20)

  You can now use it in all perf tools, such as:

          perf record -e probe:schedule_1 -aR sleep 1

However, 'perf probe -l' shows the same offset:

  # perf probe -l
    probe:schedule       (on schedule+8@kernel/sched/core.c)
    probe:schedule_1     (on schedule+8@kernel/sched/core.c)

__show_perf_probe_events() does not clean up the 'pev' content when
parsing the rawlist. If the 'pev->offset' is not set while processing
the next probe event string, the offset value of the previous event will
be used.  After adding debug information, it was found that indeed there
was line number information when processing 'probe:schedule_1', so the
offset was not set and used the offset from 'probe:schedule'.

To fix this, in the loop that parses the rawlist, reset the contents of
'tev' and 'pev' to ensure it does not affect the next parsing.

After the modification,  'perf probe -l' shows the following:

  # perf probe -l
    probe:schedule       (on schedule+8@kernel/sched/core.c)
    probe:schedule_1     (on schedule:-6751@kernel/sched/core.c)

Note that 'probe:schedule_1' is displayed with line number, but the line
number seem to be incorrect. This issue is independent of the problem
fixed by the current patch and will be addressed in the next patch.

Fixes: d8f9da240495 ("perf tools: Use zfree() where applicable")
Signed-off-by: Li Huafei <lihuafei1@huawei.com>
---
 tools/perf/util/probe-event.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index a17c9b8a7a79..ec0b11f8d881 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2695,6 +2695,8 @@ static int __show_perf_probe_events(int fd, bool is_kprobe,
 next:
 		clear_perf_probe_event(&pev);
 		clear_probe_trace_event(&tev);
+		memset(&tev, 0, sizeof(tev));
+		memset(&pev, 0, sizeof(pev));
 		if (ret < 0)
 			break;
 	}
-- 
2.25.1

[PATCH 2/2] perf probe: Fix the incorrect line number display in 'perf probe -l'

[Li Huafei]

I found an issue with the line number display in perf probe -l:

  # perf probe -l
    probe:schedule       (on schedule:-6751@kernel/sched/core.c)

I founded that in debuginfo__find_probe_point(), the fname obtained by
cu_find_lineinfo() is different from the result returned by
die_get_decl_file(). The 'baseline' and 'lineno' do not correspond to
the same file, resulting in an incorrect calculation of the line number
(lineno - baseline).

The DWARF dump information shows that the probed address
0xffff800080e55bc4 (i.e., schedule+20) has two source file information:

  # readelf --debug-dump=decodedline vmlinux | grep ffff800080e55bc4 -C 2
  ./arch/arm64/include/asm/current.h:
  current.h                                     19  0xffff800080e55bc0
  current.h                                     21  0xffff800080e55bc4               x
  current.h                                     21  0xffff800080e55bc4       1

  kernel/sched/core.c:
  core.c                                      6777  0xffff800080e55bc4       2       x
  core.c                                      6777  0xffff800080e55bc4       3       x
  core.c                                      6777  0xffff800080e55bc4       4       x
  core.c                                      6780  0xffff800080e55bc4       5       x

The first location corresponds to the inline function get_current(), and
cu_find_lineinfo() should have found this entry. However, the probed
instruction is actually in the schedule() function, which is
disassembled as follows:

  crash> disassemble/s schedule
  Dump of assembler code for function schedule:
  ./arch/arm64/include/asm/current.h:
  15      static __always_inline struct task_struct *get_current(void)
  16      {
  17              unsigned long sp_el0;
  18
  19              asm ("mrs %0, sp_el0" : "=r" (sp_el0));
     0xffff800080e55bb0 <+0>:     paciasp
  Dump of assembler code for function schedule:
  ./arch/arm64/include/asm/current.h:
  15      static __always_inline struct task_struct *get_current(void)
  16      {
  17              unsigned long sp_el0;
  18
  19              asm ("mrs %0, sp_el0" : "=r" (sp_el0));
     0xffff800080e55bb0 <+0>:     paciasp
     0xffff800080e55bb4 <+4>:     stp     x29, x30, [sp, #-32]!
     0xffff800080e55bb8 <+8>:     mov     x29, sp
     0xffff800080e55bbc <+12>:    stp     x19, x20, [sp, #16]
     0xffff800080e55bc0 <+16>:    mrs     x19, sp_el0

  kernel/sched/core.c:
  6780            if (!task_is_running(tsk))
     0xffff800080e55bc4 <+20>:    ldr     w0, [x19, #24]
     0xffff800080e55bc8 <+24>:    cbnz    w0, 0xffff800080e55bf8 <schedule+72>

And the DWARF function dump information:

  <1><11eae66>: Abbrev Number: 88 (DW_TAG_subprogram)
     <11eae67>   DW_AT_external    : 1
     <11eae67>   DW_AT_name        : (indirect string, offset: 0x233efb): schedule
     <11eae6b>   DW_AT_decl_file   : 18
     <11eae6c>   DW_AT_decl_line   : 6772
     <11eae6e>   DW_AT_decl_column : 35
     <11eae6f>   DW_AT_prototyped  : 1
     <11eae6f>   DW_AT_low_pc      : 0xffff800080e55bb0
     <11eae77>   DW_AT_high_pc     : 0xb8
     <11eae7f>   DW_AT_frame_base  : 1 byte block: 9c    (DW_OP_call_frame_cfa)
     <11eae81>   DW_AT_GNU_all_call_sites: 1
     <11eae81>   DW_AT_sibling     : <0x11eb12d>
  <2><11eae85>: Abbrev Number: 50 (DW_TAG_variable)
     <11eae86>   DW_AT_name        : tsk
     <11eae8a>   DW_AT_decl_file   : 18
     <11eae8b>   DW_AT_decl_line   : 6774
     <11eae8d>   DW_AT_decl_column : 22
     <11eae8e>   DW_AT_type        : <0x11b2b34>
     <11eae92>   DW_AT_location    : 0x5be6f0 (location list)
     <11eae96>   DW_AT_GNU_locviews: 0x5be6ec
  <2><11eae9a>: Abbrev Number: 78 (DW_TAG_lexical_block)
     <11eae9b>   DW_AT_low_pc      : 0xffff800080e55bc4
     <11eaea3>   DW_AT_high_pc     : 0x0
     <11eaeab>   DW_AT_sibling     : <0x11eaeb9>

Therefore, here we should use the result of die_find_realfunc() +
die_get_decl_file(). However, regardless, we should verify if the fname
obtained from both is the same. If they are different, we should use the
latter to avoid inconsistencies between lineno, fname, and basefunc.

After the modification, the output is as follows:

  # perf probe -l
    probe:schedule       (on schedule+20@kernel/sched/core.c)

Fixes: 57f95bf5f882 ("perf probe: Show correct statement line number by perf probe -l")
Signed-off-by: Li Huafei <lihuafei1@huawei.com>
---
 tools/perf/util/probe-finder.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index 630e16c54ed5..106c9d6bd728 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -1592,7 +1592,16 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
 			goto post;
 		}
 
-		fname = die_get_decl_file(&spdie);
+		tmp = die_get_decl_file(&spdie);
+		/*
+		 * cu_find_lineinfo() uses cu_getsrc_die() to get fname, which
+		 * may differ from the result of die_get_decl_file(). Add a check
+		 * to ensure that lineno and baseline are in the same file.
+		 */
+		if (!tmp || (fname && strcmp(tmp, fname) != 0))
+			lineno = 0;
+		fname = tmp;
+
 		if (addr == baseaddr) {
 			/* Function entry - Relative line number is 0 */
 			lineno = baseline;
-- 
2.25.1

Re: [PATCH 2/2] perf probe: Fix the incorrect line number display in 'perf probe -l'

[Masami Hiramatsu]

On Sat, 9 Nov 2024 02:19:09 +0800
Li Huafei <lihuafei1@huawei.com> wrote:

> I found an issue with the line number display in perf probe -l:
> 
>   # perf probe -l
>     probe:schedule       (on schedule:-6751@kernel/sched/core.c)
> 
> I founded that in debuginfo__find_probe_point(), the fname obtained by
> cu_find_lineinfo() is different from the result returned by
> die_get_decl_file(). The 'baseline' and 'lineno' do not correspond to
> the same file, resulting in an incorrect calculation of the line number
> (lineno - baseline).

Good catch!

> 
> The DWARF dump information shows that the probed address
> 0xffff800080e55bc4 (i.e., schedule+20) has two source file information:
> 
>   # readelf --debug-dump=decodedline vmlinux | grep ffff800080e55bc4 -C 2
>   ./arch/arm64/include/asm/current.h:
>   current.h                                     19  0xffff800080e55bc0
>   current.h                                     21  0xffff800080e55bc4               x
>   current.h                                     21  0xffff800080e55bc4       1
> 
>   kernel/sched/core.c:
>   core.c                                      6777  0xffff800080e55bc4       2       x
>   core.c                                      6777  0xffff800080e55bc4       3       x
>   core.c                                      6777  0xffff800080e55bc4       4       x
>   core.c                                      6780  0xffff800080e55bc4       5       x
> 
> The first location corresponds to the inline function get_current(), and
> cu_find_lineinfo() should have found this entry. However, the probed
> instruction is actually in the schedule() function, which is
> disassembled as follows:
> 
>   crash> disassemble/s schedule
>   Dump of assembler code for function schedule:
>   ./arch/arm64/include/asm/current.h:
>   15      static __always_inline struct task_struct *get_current(void)
>   16      {
>   17              unsigned long sp_el0;
>   18
>   19              asm ("mrs %0, sp_el0" : "=r" (sp_el0));
>      0xffff800080e55bb0 <+0>:     paciasp
>   Dump of assembler code for function schedule:
>   ./arch/arm64/include/asm/current.h:
>   15      static __always_inline struct task_struct *get_current(void)
>   16      {
>   17              unsigned long sp_el0;
>   18
>   19              asm ("mrs %0, sp_el0" : "=r" (sp_el0));
>      0xffff800080e55bb0 <+0>:     paciasp
>      0xffff800080e55bb4 <+4>:     stp     x29, x30, [sp, #-32]!
>      0xffff800080e55bb8 <+8>:     mov     x29, sp
>      0xffff800080e55bbc <+12>:    stp     x19, x20, [sp, #16]
>      0xffff800080e55bc0 <+16>:    mrs     x19, sp_el0


It looks like get_current() is embedded at `0xffff800080e55bc0`, and

> 
>   kernel/sched/core.c:
>   6780            if (!task_is_running(tsk))
>      0xffff800080e55bc4 <+20>:    ldr     w0, [x19, #24]
>      0xffff800080e55bc8 <+24>:    cbnz    w0, 0xffff800080e55bf8 <schedule+72>

`0xffff800080e55bc4` is in schedule(). So line info looks strange.

> 
> And the DWARF function dump information:
> 
>   <1><11eae66>: Abbrev Number: 88 (DW_TAG_subprogram)
>      <11eae67>   DW_AT_external    : 1
>      <11eae67>   DW_AT_name        : (indirect string, offset: 0x233efb): schedule
>      <11eae6b>   DW_AT_decl_file   : 18
>      <11eae6c>   DW_AT_decl_line   : 6772
>      <11eae6e>   DW_AT_decl_column : 35
>      <11eae6f>   DW_AT_prototyped  : 1
>      <11eae6f>   DW_AT_low_pc      : 0xffff800080e55bb0
>      <11eae77>   DW_AT_high_pc     : 0xb8
>      <11eae7f>   DW_AT_frame_base  : 1 byte block: 9c    (DW_OP_call_frame_cfa)
>      <11eae81>   DW_AT_GNU_all_call_sites: 1
>      <11eae81>   DW_AT_sibling     : <0x11eb12d>
>   <2><11eae85>: Abbrev Number: 50 (DW_TAG_variable)
>      <11eae86>   DW_AT_name        : tsk
>      <11eae8a>   DW_AT_decl_file   : 18
>      <11eae8b>   DW_AT_decl_line   : 6774
>      <11eae8d>   DW_AT_decl_column : 22
>      <11eae8e>   DW_AT_type        : <0x11b2b34>
>      <11eae92>   DW_AT_location    : 0x5be6f0 (location list)
>      <11eae96>   DW_AT_GNU_locviews: 0x5be6ec
>   <2><11eae9a>: Abbrev Number: 78 (DW_TAG_lexical_block)
>      <11eae9b>   DW_AT_low_pc      : 0xffff800080e55bc4
>      <11eaea3>   DW_AT_high_pc     : 0x0
>      <11eaeab>   DW_AT_sibling     : <0x11eaeb9>
> 
> Therefore, here we should use the result of die_find_realfunc() +
> die_get_decl_file(). However, regardless, we should verify if the fname
> obtained from both is the same.

I think this is a corner case, but to fix this issue, we have to
change the process pipeline instead of adding a check.

> If they are different, we should use the
> latter to avoid inconsistencies between lineno, fname, and basefunc.

I think filename comparison may not be enough because the inlined
function definition can be done in the same file.

<source.h>
20 static inline void foo()
21 {...}

<source.c>
1000 void bar()
1001 {
1002	foo();
	...

In this case, we can check whether the function names are different
("source.h" != "source.c"), but usually inlined function defined in
the same file.

<source.c>
50 static inline void foo()
51 {...}
...
1000 void bar()
1001 {
1002	foo();
	...

In this case, both has the same file name.

Currently debuginfo__find_probe_point() does

 (1) Get the line and file from CU's lineinfo
 (2) Get the real function(function instance) of the address
     (use this function's decl_line/decl_file as basement)
 (2-1) Search the inlined function scope in the real function
     for the given address.
 (2-2) if there is inlined function, update basement line/file.
 (2-3) verify the filename is same as basement filename.
 (3) calculate the relative line number from basement.

The problem is in (1). Since we have no basement file/line info,
we can not verify that the file/line info from CU's lineinfo.
As Li shown above, the lineinfo may have several different lines
for one address. We need to find most appropriate one based on
the basement file/line.

Thus what we need are

 - Introduce cu_find_lineinfo_at() which gives basement file/line
   information so that it can choose correct one. (hopefully)
 - Swap the order of (1) and (2*) so that we can pass the basement
   file/line when searching lineinfo. (Also, (2-3) should be right
   before (3))

Thank you,

-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH 2/2] perf probe: Fix the incorrect line number display in 'perf probe -l'

[Masami Hiramatsu]

Hi Li,

On Mon, 11 Nov 2024 17:05:49 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> Currently debuginfo__find_probe_point() does
> 
>  (1) Get the line and file from CU's lineinfo
>  (2) Get the real function(function instance) of the address
>      (use this function's decl_line/decl_file as basement)
>  (2-1) Search the inlined function scope in the real function
>      for the given address.
>  (2-2) if there is inlined function, update basement line/file.
>  (2-3) verify the filename is same as basement filename.
>  (3) calculate the relative line number from basement.
> 
> The problem is in (1). Since we have no basement file/line info,
> we can not verify that the file/line info from CU's lineinfo.
> As Li shown above, the lineinfo may have several different lines
> for one address. We need to find most appropriate one based on
> the basement file/line.
> 
> Thus what we need are
> 
>  - Introduce cu_find_lineinfo_at() which gives basement file/line
>    information so that it can choose correct one. (hopefully)
>  - Swap the order of (1) and (2*) so that we can pass the basement
>    file/line when searching lineinfo. (Also, (2-3) should be right
>    before (3))
> 

Can you check below change fixes your issue?

Thank you,

From 3027c4d3c8be874a7c84a98e73fe0837fd135129 Mon Sep 17 00:00:00 2001
From: "Masami Hiramatsu (Google)" <mhiramat@kernel.org>
Date: Mon, 11 Nov 2024 20:56:00 +0900
Subject: [PATCH] perf-probe: Fix --line option to show correct offset line
 number from function

Fix --line option to show correct offset if the DWARF line info of the
probe address has the statement lines in differnt functions.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 tools/perf/util/dwarf-aux.c    | 78 ++++++++++++++++++++++++++++------
 tools/perf/util/dwarf-aux.h    |  7 +++
 tools/perf/util/probe-finder.c | 44 +++++++++++++------
 3 files changed, 105 insertions(+), 24 deletions(-)

diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index 92eb9c8dc3e5..0bffec6962b8 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -60,14 +60,33 @@ const char *cu_get_comp_dir(Dwarf_Die *cu_die)
 	return dwarf_formstring(&attr);
 }
 
+static Dwarf_Line *get_next_statement_line(Dwarf_Lines *lines, size_t *idx, Dwarf_Addr addr)
+{
+	bool is_statement = false;
+	Dwarf_Line *line;
+	Dwarf_Addr laddr;
+	size_t l = *idx;
+
+	while (!is_statement) {
+		line = dwarf_onesrcline(lines, ++l);
+		if (!line || dwarf_lineaddr(line, &laddr) != 0 ||
+			dwarf_linebeginstatement(line, &is_statement) != 0)
+				return NULL;
+		if (laddr > addr)
+			return NULL;
+	}
+	*idx = l;
+	return line;
+}
+
 /* Unlike dwarf_getsrc_die(), cu_getsrc_die() only returns statement line */
-static Dwarf_Line *cu_getsrc_die(Dwarf_Die *cu_die, Dwarf_Addr addr)
+static Dwarf_Line *cu_getsrc_die(Dwarf_Die *cu_die, Dwarf_Addr addr,
+								 Dwarf_Lines **lines_p, size_t *idx)
 {
 	Dwarf_Addr laddr;
 	Dwarf_Lines *lines;
 	Dwarf_Line *line;
 	size_t nlines, l, u, n;
-	bool flag;
 
 	if (dwarf_getsrclines(cu_die, &lines, &nlines) != 0 ||
 	    nlines == 0)
@@ -91,16 +110,14 @@ static Dwarf_Line *cu_getsrc_die(Dwarf_Die *cu_die, Dwarf_Addr addr)
 		if (!line || dwarf_lineaddr(line, &laddr) != 0)
 			return NULL;
 	} while (laddr == addr);
-	l++;
 	/* Going forward to find the statement line */
-	do {
-		line = dwarf_onesrcline(lines, l++);
-		if (!line || dwarf_lineaddr(line, &laddr) != 0 ||
-		    dwarf_linebeginstatement(line, &flag) != 0)
-			return NULL;
-		if (laddr > addr)
-			return NULL;
-	} while (!flag);
+	line = get_next_statement_line(lines, &l, addr);
+	if (!line)
+		return NULL;
+	if (lines_p)
+		*lines_p = lines;
+	if (idx)
+		*idx = l;
 
 	return line;
 }
@@ -129,7 +146,7 @@ int cu_find_lineinfo(Dwarf_Die *cu_die, Dwarf_Addr addr,
 		goto out;
 	}
 
-	line = cu_getsrc_die(cu_die, addr);
+	line = cu_getsrc_die(cu_die, addr, NULL, NULL);
 	if (line && dwarf_lineno(line, lineno) == 0) {
 		*fname = dwarf_linesrc(line, NULL, NULL);
 		if (!*fname)
@@ -141,6 +158,43 @@ int cu_find_lineinfo(Dwarf_Die *cu_die, Dwarf_Addr addr,
 	return (*lineno && *fname) ? *lineno : -ENOENT;
 }
 
+/**
+ * cu_find_lineinfo_after - Get a line number after file:line for given address
+ * @cu_die: a CU DIE
+ * @addr: An address
+ * @lineno: a pointer which returns the line number
+ * @fname: the filename where searching the line
+ * @baseline: the basement line number
+ *
+ * Find a line number after @baseline in @fname for @addr in @cu_die.
+ * Return the found line number, or -ENOENT if not found.
+ */
+int cu_find_lineinfo_after(Dwarf_Die *cu_die, Dwarf_Addr addr,
+					int *lineno, const char *fname, int baseline)
+{
+	const char *line_fname;
+	Dwarf_Lines *lines;
+	Dwarf_Line *line;
+	size_t idx = 0;
+
+	if (cu_find_lineinfo(cu_die, addr, &line_fname, lineno) < 0)
+		return -ENOENT;
+
+	if (!strcmp(line_fname, fname) && baseline <= *lineno)
+		return *lineno;
+
+	line = cu_getsrc_die(cu_die, addr, &lines, &idx);
+
+	while (line && dwarf_lineno(line, lineno) == 0) {
+		line_fname = dwarf_linesrc(line, NULL, NULL);
+		if (line_fname && !strcmp(line_fname, fname) && baseline <= *lineno)
+			return *lineno;
+		line = get_next_statement_line(lines, &idx, addr);
+	}
+
+	return -ENOENT;
+}
+
 static int __die_find_inline_cb(Dwarf_Die *die_mem, void *data);
 
 /**
diff --git a/tools/perf/util/dwarf-aux.h b/tools/perf/util/dwarf-aux.h
index bd7505812569..19edf21e2f78 100644
--- a/tools/perf/util/dwarf-aux.h
+++ b/tools/perf/util/dwarf-aux.h
@@ -23,6 +23,13 @@ const char *cu_get_comp_dir(Dwarf_Die *cu_die);
 int cu_find_lineinfo(Dwarf_Die *cudie, Dwarf_Addr addr,
 		     const char **fname, int *lineno);
 
+/*
+ * Get the most likely line number for given address in given filename
+ * and basement line number.
+ */
+int cu_find_lineinfo_after(Dwarf_Die *cudie, Dwarf_Addr addr,
+					int *lineno, const char *fname, int baseline);
+
 /* Walk on functions at given address */
 int cu_walk_functions_at(Dwarf_Die *cu_die, Dwarf_Addr addr,
 			 int (*callback)(Dwarf_Die *, void *), void *data);
diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index 13ff45d3d6a4..efcacb5568e5 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -1578,7 +1578,8 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
 {
 	Dwarf_Die cudie, spdie, indie;
 	Dwarf_Addr _addr = 0, baseaddr = 0;
-	const char *fname = NULL, *func = NULL, *basefunc = NULL, *tmp;
+	const char *fname = NULL, *func = NULL, *basefunc = NULL;
+	const char *basefname = NULL, *tmp;
 	int baseline = 0, lineno = 0, ret = 0;
 
 	/* We always need to relocate the address for aranges */
@@ -1592,11 +1593,7 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
 		goto end;
 	}
 
-	/* Find a corresponding line (filename and lineno) */
-	cu_find_lineinfo(&cudie, (Dwarf_Addr)addr, &fname, &lineno);
-	/* Don't care whether it failed or not */
-
-	/* Find a corresponding function (name, baseline and baseaddr) */
+	/* Find the basement function (name, baseline and baseaddr) */
 	if (die_find_realfunc(&cudie, (Dwarf_Addr)addr, &spdie)) {
 		/* Get function entry information */
 		func = basefunc = dwarf_diename(&spdie);
@@ -1607,10 +1604,16 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
 			goto post;
 		}
 
-		fname = die_get_decl_file(&spdie);
+		basefname = die_get_decl_file(&spdie);
+		if (!basefname) {
+			lineno = 0;
+			goto post;
+		}
+
 		if (addr == baseaddr) {
 			/* Function entry - Relative line number is 0 */
 			lineno = baseline;
+			fname = basefname;
 			goto post;
 		}
 
@@ -1627,7 +1630,9 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
 				 */
 				lineno = die_get_call_lineno(&indie);
 				fname = die_get_call_file(&indie);
-				break;
+				if (!fname || strcmp(fname, basefname))
+					lineno = 0;
+				goto post;
 			} else {
 				/*
 				 * addr is in an inline function body.
@@ -1636,20 +1641,35 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
 				 * be the entry line of the inline function.
 				 */
 				tmp = dwarf_diename(&indie);
-				if (!tmp ||
-				    dwarf_decl_line(&indie, &baseline) != 0)
-					break;
+				basefname = die_get_decl_file(&indie);
+				if (!tmp || !basefname ||
+				    dwarf_decl_line(&indie, &baseline) != 0) {
+					lineno = 0;
+					goto post;
+				}
 				func = tmp;
 				spdie = indie;
 			}
 		}
+	}
+
+	if (!lineno) {
+		/* Find a corresponding line (filename and lineno) */
+		if (cu_find_lineinfo_after(&cudie, (Dwarf_Addr)addr, &lineno,
+								   basefname, baseline) < 0)
+			lineno = 0;
+		else
+			fname = basefname;
+	}
+
+post:
+	if (lineno) {
 		/* Verify the lineno and baseline are in a same file */
 		tmp = die_get_decl_file(&spdie);
 		if (!tmp || (fname && strcmp(tmp, fname) != 0))
 			lineno = 0;
 	}
 
-post:
 	/* Make a relative line number or an offset */
 	if (lineno)
 		ppt->line = lineno - baseline;
-- 
2.47.0.277.g8800431eea-goog


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH 2/2] perf probe: Fix the incorrect line number display in 'perf probe -l'

[Li Huafei]

Hi Masami,

On 2024/11/11 20:05, Masami Hiramatsu (Google) wrote:
> Hi Li,
> 
> On Mon, 11 Nov 2024 17:05:49 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
>> Currently debuginfo__find_probe_point() does
>>
>>  (1) Get the line and file from CU's lineinfo
>>  (2) Get the real function(function instance) of the address
>>      (use this function's decl_line/decl_file as basement)
>>  (2-1) Search the inlined function scope in the real function
>>      for the given address.
>>  (2-2) if there is inlined function, update basement line/file.
>>  (2-3) verify the filename is same as basement filename.
>>  (3) calculate the relative line number from basement.
>>
>> The problem is in (1). Since we have no basement file/line info,
>> we can not verify that the file/line info from CU's lineinfo.
>> As Li shown above, the lineinfo may have several different lines
>> for one address. We need to find most appropriate one based on
>> the basement file/line.
>>
>> Thus what we need are
>>
>>  - Introduce cu_find_lineinfo_at() which gives basement file/line
>>    information so that it can choose correct one. (hopefully)
>>  - Swap the order of (1) and (2*) so that we can pass the basement
>>    file/line when searching lineinfo. (Also, (2-3) should be right
>>    before (3))
>>
> 
> Can you check below change fixes your issue?
> 

Thank you for the detailed explanation in your previous email. I tested
your patch, and the results are as follows:

  # perf probe -l
    probe:schedule       (on schedule:5@kernel/sched/core.c)

Is this not as expected? Should our expectation be:

  probe:schedule       (on schedule:6780@kernel/sched/core.c)

Thanks,
Huafei

> Thank you,
> 
>>From 3027c4d3c8be874a7c84a98e73fe0837fd135129 Mon Sep 17 00:00:00 2001
> From: "Masami Hiramatsu (Google)" <mhiramat@kernel.org>
> Date: Mon, 11 Nov 2024 20:56:00 +0900
> Subject: [PATCH] perf-probe: Fix --line option to show correct offset line
>  number from function
> 
> Fix --line option to show correct offset if the DWARF line info of the
> probe address has the statement lines in differnt functions.
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  tools/perf/util/dwarf-aux.c    | 78 ++++++++++++++++++++++++++++------
>  tools/perf/util/dwarf-aux.h    |  7 +++
>  tools/perf/util/probe-finder.c | 44 +++++++++++++------
>  3 files changed, 105 insertions(+), 24 deletions(-)
> 
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index 92eb9c8dc3e5..0bffec6962b8 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
> @@ -60,14 +60,33 @@ const char *cu_get_comp_dir(Dwarf_Die *cu_die)
>  	return dwarf_formstring(&attr);
>  }
>  
> +static Dwarf_Line *get_next_statement_line(Dwarf_Lines *lines, size_t *idx, Dwarf_Addr addr)
> +{
> +	bool is_statement = false;
> +	Dwarf_Line *line;
> +	Dwarf_Addr laddr;
> +	size_t l = *idx;
> +
> +	while (!is_statement) {
> +		line = dwarf_onesrcline(lines, ++l);
> +		if (!line || dwarf_lineaddr(line, &laddr) != 0 ||
> +			dwarf_linebeginstatement(line, &is_statement) != 0)
> +				return NULL;
> +		if (laddr > addr)
> +			return NULL;
> +	}
> +	*idx = l;
> +	return line;
> +}
> +
>  /* Unlike dwarf_getsrc_die(), cu_getsrc_die() only returns statement line */
> -static Dwarf_Line *cu_getsrc_die(Dwarf_Die *cu_die, Dwarf_Addr addr)
> +static Dwarf_Line *cu_getsrc_die(Dwarf_Die *cu_die, Dwarf_Addr addr,
> +								 Dwarf_Lines **lines_p, size_t *idx)
>  {
>  	Dwarf_Addr laddr;
>  	Dwarf_Lines *lines;
>  	Dwarf_Line *line;
>  	size_t nlines, l, u, n;
> -	bool flag;
>  
>  	if (dwarf_getsrclines(cu_die, &lines, &nlines) != 0 ||
>  	    nlines == 0)
> @@ -91,16 +110,14 @@ static Dwarf_Line *cu_getsrc_die(Dwarf_Die *cu_die, Dwarf_Addr addr)
>  		if (!line || dwarf_lineaddr(line, &laddr) != 0)
>  			return NULL;
>  	} while (laddr == addr);
> -	l++;
>  	/* Going forward to find the statement line */
> -	do {
> -		line = dwarf_onesrcline(lines, l++);
> -		if (!line || dwarf_lineaddr(line, &laddr) != 0 ||
> -		    dwarf_linebeginstatement(line, &flag) != 0)
> -			return NULL;
> -		if (laddr > addr)
> -			return NULL;
> -	} while (!flag);
> +	line = get_next_statement_line(lines, &l, addr);
> +	if (!line)
> +		return NULL;
> +	if (lines_p)
> +		*lines_p = lines;
> +	if (idx)
> +		*idx = l;
>  
>  	return line;
>  }
> @@ -129,7 +146,7 @@ int cu_find_lineinfo(Dwarf_Die *cu_die, Dwarf_Addr addr,
>  		goto out;
>  	}
>  
> -	line = cu_getsrc_die(cu_die, addr);
> +	line = cu_getsrc_die(cu_die, addr, NULL, NULL);
>  	if (line && dwarf_lineno(line, lineno) == 0) {
>  		*fname = dwarf_linesrc(line, NULL, NULL);
>  		if (!*fname)
> @@ -141,6 +158,43 @@ int cu_find_lineinfo(Dwarf_Die *cu_die, Dwarf_Addr addr,
>  	return (*lineno && *fname) ? *lineno : -ENOENT;
>  }
>  
> +/**
> + * cu_find_lineinfo_after - Get a line number after file:line for given address
> + * @cu_die: a CU DIE
> + * @addr: An address
> + * @lineno: a pointer which returns the line number
> + * @fname: the filename where searching the line
> + * @baseline: the basement line number
> + *
> + * Find a line number after @baseline in @fname for @addr in @cu_die.
> + * Return the found line number, or -ENOENT if not found.
> + */
> +int cu_find_lineinfo_after(Dwarf_Die *cu_die, Dwarf_Addr addr,
> +					int *lineno, const char *fname, int baseline)
> +{
> +	const char *line_fname;
> +	Dwarf_Lines *lines;
> +	Dwarf_Line *line;
> +	size_t idx = 0;
> +
> +	if (cu_find_lineinfo(cu_die, addr, &line_fname, lineno) < 0)
> +		return -ENOENT;
> +
> +	if (!strcmp(line_fname, fname) && baseline <= *lineno)
> +		return *lineno;
> +
> +	line = cu_getsrc_die(cu_die, addr, &lines, &idx);
> +
> +	while (line && dwarf_lineno(line, lineno) == 0) {
> +		line_fname = dwarf_linesrc(line, NULL, NULL);
> +		if (line_fname && !strcmp(line_fname, fname) && baseline <= *lineno)
> +			return *lineno;
> +		line = get_next_statement_line(lines, &idx, addr);
> +	}
> +
> +	return -ENOENT;
> +}
> +
>  static int __die_find_inline_cb(Dwarf_Die *die_mem, void *data);
>  
>  /**
> diff --git a/tools/perf/util/dwarf-aux.h b/tools/perf/util/dwarf-aux.h
> index bd7505812569..19edf21e2f78 100644
> --- a/tools/perf/util/dwarf-aux.h
> +++ b/tools/perf/util/dwarf-aux.h
> @@ -23,6 +23,13 @@ const char *cu_get_comp_dir(Dwarf_Die *cu_die);
>  int cu_find_lineinfo(Dwarf_Die *cudie, Dwarf_Addr addr,
>  		     const char **fname, int *lineno);
>  
> +/*
> + * Get the most likely line number for given address in given filename
> + * and basement line number.
> + */
> +int cu_find_lineinfo_after(Dwarf_Die *cudie, Dwarf_Addr addr,
> +					int *lineno, const char *fname, int baseline);
> +
>  /* Walk on functions at given address */
>  int cu_walk_functions_at(Dwarf_Die *cu_die, Dwarf_Addr addr,
>  			 int (*callback)(Dwarf_Die *, void *), void *data);
> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> index 13ff45d3d6a4..efcacb5568e5 100644
> --- a/tools/perf/util/probe-finder.c
> +++ b/tools/perf/util/probe-finder.c
> @@ -1578,7 +1578,8 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
>  {
>  	Dwarf_Die cudie, spdie, indie;
>  	Dwarf_Addr _addr = 0, baseaddr = 0;
> -	const char *fname = NULL, *func = NULL, *basefunc = NULL, *tmp;
> +	const char *fname = NULL, *func = NULL, *basefunc = NULL;
> +	const char *basefname = NULL, *tmp;
>  	int baseline = 0, lineno = 0, ret = 0;
>  
>  	/* We always need to relocate the address for aranges */
> @@ -1592,11 +1593,7 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
>  		goto end;
>  	}
>  
> -	/* Find a corresponding line (filename and lineno) */
> -	cu_find_lineinfo(&cudie, (Dwarf_Addr)addr, &fname, &lineno);
> -	/* Don't care whether it failed or not */
> -
> -	/* Find a corresponding function (name, baseline and baseaddr) */
> +	/* Find the basement function (name, baseline and baseaddr) */
>  	if (die_find_realfunc(&cudie, (Dwarf_Addr)addr, &spdie)) {
>  		/* Get function entry information */
>  		func = basefunc = dwarf_diename(&spdie);
> @@ -1607,10 +1604,16 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
>  			goto post;
>  		}
>  
> -		fname = die_get_decl_file(&spdie);
> +		basefname = die_get_decl_file(&spdie);
> +		if (!basefname) {
> +			lineno = 0;
> +			goto post;
> +		}
> +
>  		if (addr == baseaddr) {
>  			/* Function entry - Relative line number is 0 */
>  			lineno = baseline;
> +			fname = basefname;
>  			goto post;
>  		}
>  
> @@ -1627,7 +1630,9 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
>  				 */
>  				lineno = die_get_call_lineno(&indie);
>  				fname = die_get_call_file(&indie);
> -				break;
> +				if (!fname || strcmp(fname, basefname))
> +					lineno = 0;
> +				goto post;
>  			} else {
>  				/*
>  				 * addr is in an inline function body.
> @@ -1636,20 +1641,35 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
>  				 * be the entry line of the inline function.
>  				 */
>  				tmp = dwarf_diename(&indie);
> -				if (!tmp ||
> -				    dwarf_decl_line(&indie, &baseline) != 0)
> -					break;
> +				basefname = die_get_decl_file(&indie);
> +				if (!tmp || !basefname ||
> +				    dwarf_decl_line(&indie, &baseline) != 0) {
> +					lineno = 0;
> +					goto post;
> +				}
>  				func = tmp;
>  				spdie = indie;
>  			}
>  		}
> +	}
> +
> +	if (!lineno) {
> +		/* Find a corresponding line (filename and lineno) */
> +		if (cu_find_lineinfo_after(&cudie, (Dwarf_Addr)addr, &lineno,
> +								   basefname, baseline) < 0)
> +			lineno = 0;
> +		else
> +			fname = basefname;
> +	}
> +
> +post:
> +	if (lineno) {
>  		/* Verify the lineno and baseline are in a same file */
>  		tmp = die_get_decl_file(&spdie);
>  		if (!tmp || (fname && strcmp(tmp, fname) != 0))
>  			lineno = 0;
>  	}
>  
> -post:
>  	/* Make a relative line number or an offset */
>  	if (lineno)
>  		ppt->line = lineno - baseline;
> 

Re: [PATCH 2/2] perf probe: Fix the incorrect line number display in 'perf probe -l'

[Li Huafei]

On 2024/11/12 11:09, Li Huafei wrote:
> Hi Masami,
> 
> On 2024/11/11 20:05, Masami Hiramatsu (Google) wrote:
>> Hi Li,
>>
>> On Mon, 11 Nov 2024 17:05:49 +0900
>> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
>>
>>> Currently debuginfo__find_probe_point() does
>>>
>>>  (1) Get the line and file from CU's lineinfo
>>>  (2) Get the real function(function instance) of the address
>>>      (use this function's decl_line/decl_file as basement)
>>>  (2-1) Search the inlined function scope in the real function
>>>      for the given address.
>>>  (2-2) if there is inlined function, update basement line/file.
>>>  (2-3) verify the filename is same as basement filename.
>>>  (3) calculate the relative line number from basement.
>>>
>>> The problem is in (1). Since we have no basement file/line info,
>>> we can not verify that the file/line info from CU's lineinfo.
>>> As Li shown above, the lineinfo may have several different lines
>>> for one address. We need to find most appropriate one based on
>>> the basement file/line.
>>>
>>> Thus what we need are
>>>
>>>  - Introduce cu_find_lineinfo_at() which gives basement file/line
>>>    information so that it can choose correct one. (hopefully)
>>>  - Swap the order of (1) and (2*) so that we can pass the basement
>>>    file/line when searching lineinfo. (Also, (2-3) should be right
>>>    before (3))
>>>
>>
>> Can you check below change fixes your issue?
>>
> 
> Thank you for the detailed explanation in your previous email. I tested
> your patch, and the results are as follows:
> 
>   # perf probe -l
>     probe:schedule       (on schedule:5@kernel/sched/core.c)
> 

Sorry, I made a mistake. 5 is the offset from the function entry, not
the line number. So your patch is ok.

Thanks，
Huafei

> Is this not as expected? Should our expectation be:
> 
>   probe:schedule       (on schedule:6780@kernel/sched/core.c)
> 
> Thanks,
> Huafei
> 
>> Thank you,
>>
>> >From 3027c4d3c8be874a7c84a98e73fe0837fd135129 Mon Sep 17 00:00:00 2001
>> From: "Masami Hiramatsu (Google)" <mhiramat@kernel.org>
>> Date: Mon, 11 Nov 2024 20:56:00 +0900
>> Subject: [PATCH] perf-probe: Fix --line option to show correct offset line
>>  number from function
>>
>> Fix --line option to show correct offset if the DWARF line info of the
>> probe address has the statement lines in differnt functions.
>>
>> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>> ---
>>  tools/perf/util/dwarf-aux.c    | 78 ++++++++++++++++++++++++++++------
>>  tools/perf/util/dwarf-aux.h    |  7 +++
>>  tools/perf/util/probe-finder.c | 44 +++++++++++++------
>>  3 files changed, 105 insertions(+), 24 deletions(-)
>>
>> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
>> index 92eb9c8dc3e5..0bffec6962b8 100644
>> --- a/tools/perf/util/dwarf-aux.c
>> +++ b/tools/perf/util/dwarf-aux.c
>> @@ -60,14 +60,33 @@ const char *cu_get_comp_dir(Dwarf_Die *cu_die)
>>  	return dwarf_formstring(&attr);
>>  }
>>  
>> +static Dwarf_Line *get_next_statement_line(Dwarf_Lines *lines, size_t *idx, Dwarf_Addr addr)
>> +{
>> +	bool is_statement = false;
>> +	Dwarf_Line *line;
>> +	Dwarf_Addr laddr;
>> +	size_t l = *idx;
>> +
>> +	while (!is_statement) {
>> +		line = dwarf_onesrcline(lines, ++l);
>> +		if (!line || dwarf_lineaddr(line, &laddr) != 0 ||
>> +			dwarf_linebeginstatement(line, &is_statement) != 0)
>> +				return NULL;
>> +		if (laddr > addr)
>> +			return NULL;
>> +	}
>> +	*idx = l;
>> +	return line;
>> +}
>> +
>>  /* Unlike dwarf_getsrc_die(), cu_getsrc_die() only returns statement line */
>> -static Dwarf_Line *cu_getsrc_die(Dwarf_Die *cu_die, Dwarf_Addr addr)
>> +static Dwarf_Line *cu_getsrc_die(Dwarf_Die *cu_die, Dwarf_Addr addr,
>> +								 Dwarf_Lines **lines_p, size_t *idx)
>>  {
>>  	Dwarf_Addr laddr;
>>  	Dwarf_Lines *lines;
>>  	Dwarf_Line *line;
>>  	size_t nlines, l, u, n;
>> -	bool flag;
>>  
>>  	if (dwarf_getsrclines(cu_die, &lines, &nlines) != 0 ||
>>  	    nlines == 0)
>> @@ -91,16 +110,14 @@ static Dwarf_Line *cu_getsrc_die(Dwarf_Die *cu_die, Dwarf_Addr addr)
>>  		if (!line || dwarf_lineaddr(line, &laddr) != 0)
>>  			return NULL;
>>  	} while (laddr == addr);
>> -	l++;
>>  	/* Going forward to find the statement line */
>> -	do {
>> -		line = dwarf_onesrcline(lines, l++);
>> -		if (!line || dwarf_lineaddr(line, &laddr) != 0 ||
>> -		    dwarf_linebeginstatement(line, &flag) != 0)
>> -			return NULL;
>> -		if (laddr > addr)
>> -			return NULL;
>> -	} while (!flag);
>> +	line = get_next_statement_line(lines, &l, addr);
>> +	if (!line)
>> +		return NULL;
>> +	if (lines_p)
>> +		*lines_p = lines;
>> +	if (idx)
>> +		*idx = l;
>>  
>>  	return line;
>>  }
>> @@ -129,7 +146,7 @@ int cu_find_lineinfo(Dwarf_Die *cu_die, Dwarf_Addr addr,
>>  		goto out;
>>  	}
>>  
>> -	line = cu_getsrc_die(cu_die, addr);
>> +	line = cu_getsrc_die(cu_die, addr, NULL, NULL);
>>  	if (line && dwarf_lineno(line, lineno) == 0) {
>>  		*fname = dwarf_linesrc(line, NULL, NULL);
>>  		if (!*fname)
>> @@ -141,6 +158,43 @@ int cu_find_lineinfo(Dwarf_Die *cu_die, Dwarf_Addr addr,
>>  	return (*lineno && *fname) ? *lineno : -ENOENT;
>>  }
>>  
>> +/**
>> + * cu_find_lineinfo_after - Get a line number after file:line for given address
>> + * @cu_die: a CU DIE
>> + * @addr: An address
>> + * @lineno: a pointer which returns the line number
>> + * @fname: the filename where searching the line
>> + * @baseline: the basement line number
>> + *
>> + * Find a line number after @baseline in @fname for @addr in @cu_die.
>> + * Return the found line number, or -ENOENT if not found.
>> + */
>> +int cu_find_lineinfo_after(Dwarf_Die *cu_die, Dwarf_Addr addr,
>> +					int *lineno, const char *fname, int baseline)
>> +{
>> +	const char *line_fname;
>> +	Dwarf_Lines *lines;
>> +	Dwarf_Line *line;
>> +	size_t idx = 0;
>> +
>> +	if (cu_find_lineinfo(cu_die, addr, &line_fname, lineno) < 0)
>> +		return -ENOENT;
>> +
>> +	if (!strcmp(line_fname, fname) && baseline <= *lineno)
>> +		return *lineno;
>> +
>> +	line = cu_getsrc_die(cu_die, addr, &lines, &idx);
>> +
>> +	while (line && dwarf_lineno(line, lineno) == 0) {
>> +		line_fname = dwarf_linesrc(line, NULL, NULL);
>> +		if (line_fname && !strcmp(line_fname, fname) && baseline <= *lineno)
>> +			return *lineno;
>> +		line = get_next_statement_line(lines, &idx, addr);
>> +	}
>> +
>> +	return -ENOENT;
>> +}
>> +
>>  static int __die_find_inline_cb(Dwarf_Die *die_mem, void *data);
>>  
>>  /**
>> diff --git a/tools/perf/util/dwarf-aux.h b/tools/perf/util/dwarf-aux.h
>> index bd7505812569..19edf21e2f78 100644
>> --- a/tools/perf/util/dwarf-aux.h
>> +++ b/tools/perf/util/dwarf-aux.h
>> @@ -23,6 +23,13 @@ const char *cu_get_comp_dir(Dwarf_Die *cu_die);
>>  int cu_find_lineinfo(Dwarf_Die *cudie, Dwarf_Addr addr,
>>  		     const char **fname, int *lineno);
>>  
>> +/*
>> + * Get the most likely line number for given address in given filename
>> + * and basement line number.
>> + */
>> +int cu_find_lineinfo_after(Dwarf_Die *cudie, Dwarf_Addr addr,
>> +					int *lineno, const char *fname, int baseline);
>> +
>>  /* Walk on functions at given address */
>>  int cu_walk_functions_at(Dwarf_Die *cu_die, Dwarf_Addr addr,
>>  			 int (*callback)(Dwarf_Die *, void *), void *data);
>> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
>> index 13ff45d3d6a4..efcacb5568e5 100644
>> --- a/tools/perf/util/probe-finder.c
>> +++ b/tools/perf/util/probe-finder.c
>> @@ -1578,7 +1578,8 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
>>  {
>>  	Dwarf_Die cudie, spdie, indie;
>>  	Dwarf_Addr _addr = 0, baseaddr = 0;
>> -	const char *fname = NULL, *func = NULL, *basefunc = NULL, *tmp;
>> +	const char *fname = NULL, *func = NULL, *basefunc = NULL;
>> +	const char *basefname = NULL, *tmp;
>>  	int baseline = 0, lineno = 0, ret = 0;
>>  
>>  	/* We always need to relocate the address for aranges */
>> @@ -1592,11 +1593,7 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
>>  		goto end;
>>  	}
>>  
>> -	/* Find a corresponding line (filename and lineno) */
>> -	cu_find_lineinfo(&cudie, (Dwarf_Addr)addr, &fname, &lineno);
>> -	/* Don't care whether it failed or not */
>> -
>> -	/* Find a corresponding function (name, baseline and baseaddr) */
>> +	/* Find the basement function (name, baseline and baseaddr) */
>>  	if (die_find_realfunc(&cudie, (Dwarf_Addr)addr, &spdie)) {
>>  		/* Get function entry information */
>>  		func = basefunc = dwarf_diename(&spdie);
>> @@ -1607,10 +1604,16 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
>>  			goto post;
>>  		}
>>  
>> -		fname = die_get_decl_file(&spdie);
>> +		basefname = die_get_decl_file(&spdie);
>> +		if (!basefname) {
>> +			lineno = 0;
>> +			goto post;
>> +		}
>> +
>>  		if (addr == baseaddr) {
>>  			/* Function entry - Relative line number is 0 */
>>  			lineno = baseline;
>> +			fname = basefname;
>>  			goto post;
>>  		}
>>  
>> @@ -1627,7 +1630,9 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
>>  				 */
>>  				lineno = die_get_call_lineno(&indie);
>>  				fname = die_get_call_file(&indie);
>> -				break;
>> +				if (!fname || strcmp(fname, basefname))
>> +					lineno = 0;
>> +				goto post;
>>  			} else {
>>  				/*
>>  				 * addr is in an inline function body.
>> @@ -1636,20 +1641,35 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
>>  				 * be the entry line of the inline function.
>>  				 */
>>  				tmp = dwarf_diename(&indie);
>> -				if (!tmp ||
>> -				    dwarf_decl_line(&indie, &baseline) != 0)
>> -					break;
>> +				basefname = die_get_decl_file(&indie);
>> +				if (!tmp || !basefname ||
>> +				    dwarf_decl_line(&indie, &baseline) != 0) {
>> +					lineno = 0;
>> +					goto post;
>> +				}
>>  				func = tmp;
>>  				spdie = indie;
>>  			}
>>  		}
>> +	}
>> +
>> +	if (!lineno) {
>> +		/* Find a corresponding line (filename and lineno) */
>> +		if (cu_find_lineinfo_after(&cudie, (Dwarf_Addr)addr, &lineno,
>> +								   basefname, baseline) < 0)
>> +			lineno = 0;
>> +		else
>> +			fname = basefname;
>> +	}
>> +
>> +post:
>> +	if (lineno) {
>>  		/* Verify the lineno and baseline are in a same file */
>>  		tmp = die_get_decl_file(&spdie);
>>  		if (!tmp || (fname && strcmp(tmp, fname) != 0))
>>  			lineno = 0;
>>  	}
>>  
>> -post:
>>  	/* Make a relative line number or an offset */
>>  	if (lineno)
>>  		ppt->line = lineno - baseline;
>>

Re: [PATCH 2/2] perf probe: Fix the incorrect line number display in 'perf probe -l'

[Masami Hiramatsu]

Hi Li,

On Wed, 13 Nov 2024 16:39:54 +0800
Li Huafei <lihuafei1@huawei.com> wrote:

> 
> 
> On 2024/11/12 11:09, Li Huafei wrote:
> > Hi Masami,
> > 
> > On 2024/11/11 20:05, Masami Hiramatsu (Google) wrote:
> >> Hi Li,
> >>
> >> On Mon, 11 Nov 2024 17:05:49 +0900
> >> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> >>
> >>> Currently debuginfo__find_probe_point() does
> >>>
> >>>  (1) Get the line and file from CU's lineinfo
> >>>  (2) Get the real function(function instance) of the address
> >>>      (use this function's decl_line/decl_file as basement)
> >>>  (2-1) Search the inlined function scope in the real function
> >>>      for the given address.
> >>>  (2-2) if there is inlined function, update basement line/file.
> >>>  (2-3) verify the filename is same as basement filename.
> >>>  (3) calculate the relative line number from basement.
> >>>
> >>> The problem is in (1). Since we have no basement file/line info,
> >>> we can not verify that the file/line info from CU's lineinfo.
> >>> As Li shown above, the lineinfo may have several different lines
> >>> for one address. We need to find most appropriate one based on
> >>> the basement file/line.
> >>>
> >>> Thus what we need are
> >>>
> >>>  - Introduce cu_find_lineinfo_at() which gives basement file/line
> >>>    information so that it can choose correct one. (hopefully)
> >>>  - Swap the order of (1) and (2*) so that we can pass the basement
> >>>    file/line when searching lineinfo. (Also, (2-3) should be right
> >>>    before (3))
> >>>
> >>
> >> Can you check below change fixes your issue?
> >>
> > 
> > Thank you for the detailed explanation in your previous email. I tested
> > your patch, and the results are as follows:
> > 
> >   # perf probe -l
> >     probe:schedule       (on schedule:5@kernel/sched/core.c)
> > 
> 
> Sorry, I made a mistake. 5 is the offset from the function entry, not
> the line number. So your patch is ok.

Thanks for testing! OK, let me cleanup the patch.


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH 1/2] perf probe: Reset old content before processing the next event string

[Masami Hiramatsu]

On Sat, 9 Nov 2024 02:19:08 +0800
Li Huafei <lihuafei1@huawei.com> wrote:

> I added two probe events:
> 
>   # perf probe -f -a schedule+8
>   Added new event:
>     probe:schedule       (on schedule+8)
> 
>   You can now use it in all perf tools, such as:
> 
>           perf record -e probe:schedule -aR sleep 1
> 
>   # perf probe -f -a schedule+20
>   Added new event:
>     probe:schedule_1     (on schedule+20)
> 
>   You can now use it in all perf tools, such as:
> 
>           perf record -e probe:schedule_1 -aR sleep 1
> 
> However, 'perf probe -l' shows the same offset:
> 
>   # perf probe -l
>     probe:schedule       (on schedule+8@kernel/sched/core.c)
>     probe:schedule_1     (on schedule+8@kernel/sched/core.c)
> 
> __show_perf_probe_events() does not clean up the 'pev' content when
> parsing the rawlist. If the 'pev->offset' is not set while processing
> the next probe event string, the offset value of the previous event will
> be used.  After adding debug information, it was found that indeed there
> was line number information when processing 'probe:schedule_1', so the
> offset was not set and used the offset from 'probe:schedule'.
> 
> To fix this, in the loop that parses the rawlist, reset the contents of
> 'tev' and 'pev' to ensure it does not affect the next parsing.
> 
> After the modification,  'perf probe -l' shows the following:
> 
>   # perf probe -l
>     probe:schedule       (on schedule+8@kernel/sched/core.c)
>     probe:schedule_1     (on schedule:-6751@kernel/sched/core.c)
> 
> Note that 'probe:schedule_1' is displayed with line number, but the line
> number seem to be incorrect. This issue is independent of the problem
> fixed by the current patch and will be addressed in the next patch.
> 

Good catch! But we should do the cleanup in clear_perf_probe_event()
and clear_probe_trace_event().

> Fixes: d8f9da240495 ("perf tools: Use zfree() where applicable")

What we need is to revert this change for above 2 functions, because
without that, it "clear"s the structure correctly. Current code
releases allocated fields, but not clear all fields. This can lead
another bug.

Thank you,

> Signed-off-by: Li Huafei <lihuafei1@huawei.com>
> ---
>  tools/perf/util/probe-event.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index a17c9b8a7a79..ec0b11f8d881 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -2695,6 +2695,8 @@ static int __show_perf_probe_events(int fd, bool is_kprobe,
>  next:
>  		clear_perf_probe_event(&pev);
>  		clear_probe_trace_event(&tev);
> +		memset(&tev, 0, sizeof(tev));
> +		memset(&pev, 0, sizeof(pev));
>  		if (ret < 0)
>  			break;
>  	}
> -- 
> 2.25.1
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH 1/2] perf probe: Reset old content before processing the next event string

[Li Huafei]

On 2024/11/11 12:47, Masami Hiramatsu (Google) wrote:
> On Sat, 9 Nov 2024 02:19:08 +0800
> Li Huafei <lihuafei1@huawei.com> wrote:
> 
>> I added two probe events:
>>
>>   # perf probe -f -a schedule+8
>>   Added new event:
>>     probe:schedule       (on schedule+8)
>>
>>   You can now use it in all perf tools, such as:
>>
>>           perf record -e probe:schedule -aR sleep 1
>>
>>   # perf probe -f -a schedule+20
>>   Added new event:
>>     probe:schedule_1     (on schedule+20)
>>
>>   You can now use it in all perf tools, such as:
>>
>>           perf record -e probe:schedule_1 -aR sleep 1
>>
>> However, 'perf probe -l' shows the same offset:
>>
>>   # perf probe -l
>>     probe:schedule       (on schedule+8@kernel/sched/core.c)
>>     probe:schedule_1     (on schedule+8@kernel/sched/core.c)
>>
>> __show_perf_probe_events() does not clean up the 'pev' content when
>> parsing the rawlist. If the 'pev->offset' is not set while processing
>> the next probe event string, the offset value of the previous event will
>> be used.  After adding debug information, it was found that indeed there
>> was line number information when processing 'probe:schedule_1', so the
>> offset was not set and used the offset from 'probe:schedule'.
>>
>> To fix this, in the loop that parses the rawlist, reset the contents of
>> 'tev' and 'pev' to ensure it does not affect the next parsing.
>>
>> After the modification,  'perf probe -l' shows the following:
>>
>>   # perf probe -l
>>     probe:schedule       (on schedule+8@kernel/sched/core.c)
>>     probe:schedule_1     (on schedule:-6751@kernel/sched/core.c)
>>
>> Note that 'probe:schedule_1' is displayed with line number, but the line
>> number seem to be incorrect. This issue is independent of the problem
>> fixed by the current patch and will be addressed in the next patch.
>>
> 
> Good catch! But we should do the cleanup in clear_perf_probe_event()
> and clear_probe_trace_event().
> 
>> Fixes: d8f9da240495 ("perf tools: Use zfree() where applicable")
> 
> What we need is to revert this change for above 2 functions, because
> without that, it "clear"s the structure correctly. Current code
> releases allocated fields, but not clear all fields. This can lead
> another bug.


Right, I will make this change in the next version.

Thanks，
Huafei

> 
> Thank you,
> 
>> Signed-off-by: Li Huafei <lihuafei1@huawei.com>
>> ---
>>  tools/perf/util/probe-event.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>> index a17c9b8a7a79..ec0b11f8d881 100644
>> --- a/tools/perf/util/probe-event.c
>> +++ b/tools/perf/util/probe-event.c
>> @@ -2695,6 +2695,8 @@ static int __show_perf_probe_events(int fd, bool is_kprobe,
>>  next:
>>  		clear_perf_probe_event(&pev);
>>  		clear_probe_trace_event(&tev);
>> +		memset(&tev, 0, sizeof(tev));
>> +		memset(&pev, 0, sizeof(pev));
>>  		if (ret < 0)
>>  			break;
>>  	}
>> -- 
>> 2.25.1
>>
>>
> 
>