[v2,3/3] tools/perf: Fix to get declared file name from clang DWARF5

Message ID 166731052936.2100653.13380621874859467731.stgit@devnote3
State New
Headers
Series tools/perf: Fix perf probe crash by clang DWARF5 file |

Commit Message

Masami Hiramatsu (Google) Nov. 1, 2022, 1:48 p.m. UTC
  From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Fix to get the declared file name even if it uses file index 0
in DWARF5, using custom die_get_decl_file() function.

Actually, the DWARF5 standard says file index 0 of the
DW_AT_decl_file is invalid(*), but there is a discussion and
maybe this will be updated. Anyway, the clang generates such
DWARF5 file for the linux kernel. Thus it must be handled.

Without this, the perf probe returns an error;
 $ ./perf probe -k $BIN_PATH/vmlinux -s $SRC_PATH -L vfs_read:10
 Debuginfo analysis failed.
   Error: Failed to show lines.

With this, it can handle the case correctly;
 $ ./perf probe -k $BIN_PATH/vmlinux -s $SRC_PATH -L vfs_read:10
 <vfs_read@$SRC_PATH/fs/read_write.c:10>

      11         ret = rw_verify_area(READ, file, pos, count);
      12         if (ret)
                          return ret;

(* DWARF5 specification 2.14 says "The value 0 indicates that no
source file has been specified.")
(**
http://wiki.dwarfstd.org/index.php?title=DWARF5_Line_Table_File_Numbers)

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 Changes in v2:
  - Update description so that it is clang version of DWARF5.
---
 tools/perf/util/dwarf-aux.c    |   47 +++++++++++++++++++++++++++-------------
 tools/perf/util/dwarf-aux.h    |    3 +++
 tools/perf/util/probe-finder.c |   14 ++++++------
 3 files changed, 42 insertions(+), 22 deletions(-)
  

Comments

Georg Müller June 9, 2023, 12:21 p.m. UTC | #1
Hi,

Am 01.11.22 um 14:48 schrieb Masami Hiramatsu (Google):
> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>
> Fix to get the declared file name even if it uses file index 0
> in DWARF5, using custom die_get_decl_file() function.
>

this patch breaks perf probe on fedora 38.

I was trying to a add a probe to systemd-logind using kernel-6.3.6-200.fc38.x86_64 with kernel-tools-6.3.3-200.fc38.x86_64.

When trying to add a probe, I get the following message:

# perf probe -x /usr/lib/systemd/systemd-logind --funcs="match_unit_removed"
match_unit_removed

Function exists and was found:

# perf probe -x /usr/lib/systemd/systemd-logind match_unit_removed
A function DIE doesn't have decl_line. Maybe broken DWARF?
A function DIE doesn't have decl_line. Maybe broken DWARF?
Probe point 'match_unit_removed' not found.
   Error: Failed to add events.

When reverting dc9a5d2ccd5c823cc05cafe75fcf19b682d8152c, I was able to add the probe point:

# ./perf probe -x /usr/lib/systemd/systemd-logind match_unit_removed
Added new event:
   probe_systemd:match_unit_removed (on match_unit_removed in /usr/lib/systemd/systemd-logind)

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

	perf record -e probe_systemd:match_unit_removed -aR sleep 1


Probe point is then visible with and without this commit:

# perf probe -l
   probe_systemd:match_unit_removed (on match_unit_removed in /usr/lib/systemd/systemd-logind)
# ./perf probe -l
   probe_systemd:match_unit_removed (on match_unit_removed@../src/login/logind-dbus.c in /usr/lib/systemd/systemd-logind)


Best regards,
Georg
  
Linux regression tracking (Thorsten Leemhuis) June 15, 2023, 11:42 a.m. UTC | #2
[CCing the regression list, as it should be in the loop for regressions:
https://docs.kernel.org/admin-guide/reporting-regressions.html]

[TLDR: I'm adding this report to the list of tracked Linux kernel
regressions; the text you find below is based on a few templates
paragraphs you might have encountered already in similar form.
See link in footer if these mails annoy you.]

On 09.06.23 14:21, Georg Müller wrote:
> 
> Am 01.11.22 um 14:48 schrieb Masami Hiramatsu (Google):
>> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>>
>> Fix to get the declared file name even if it uses file index 0
>> in DWARF5, using custom die_get_decl_file() function.
>>
> 
> this patch breaks perf probe on fedora 38.
> 
> I was trying to a add a probe to systemd-logind using
> kernel-6.3.6-200.fc38.x86_64 with kernel-tools-6.3.3-200.fc38.x86_64.
> 
> When trying to add a probe, I get the following message:
> 
> # perf probe -x /usr/lib/systemd/systemd-logind
> --funcs="match_unit_removed"
> match_unit_removed
> 
> Function exists and was found:
> 
> # perf probe -x /usr/lib/systemd/systemd-logind match_unit_removed
> A function DIE doesn't have decl_line. Maybe broken DWARF?
> A function DIE doesn't have decl_line. Maybe broken DWARF?
> Probe point 'match_unit_removed' not found.
>   Error: Failed to add events.
> 
> When reverting dc9a5d2ccd5c823cc05cafe75fcf19b682d8152c, I was able to
> add the probe point:
> 
> # ./perf probe -x /usr/lib/systemd/systemd-logind match_unit_removed
> Added new event:
>   probe_systemd:match_unit_removed (on match_unit_removed in
> /usr/lib/systemd/systemd-logind)
> 
> You can now use it in all perf tools, such as:
> 
>     perf record -e probe_systemd:match_unit_removed -aR sleep 1
> 
> 
> Probe point is then visible with and without this commit:
> 
> # perf probe -l
>   probe_systemd:match_unit_removed (on match_unit_removed in
> /usr/lib/systemd/systemd-logind)
> # ./perf probe -l
>   probe_systemd:match_unit_removed (on
> match_unit_removed@../src/login/logind-dbus.c in
> /usr/lib/systemd/systemd-logind)

Thanks for the report. To be sure the issue doesn't fall through the
cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression
tracking bot:

#regzbot ^introduced dc9a5d2ccd5c823cc05cafe75fcf19b682d8152c
#regzbot title tools/perf: adding a specific probe suddenly fails
#regzbot ignore-activity

This isn't a regression? This issue or a fix for it are already
discussed somewhere else? It was fixed already? You want to clarify when
the regression started to happen? Or point out I got the title or
something else totally wrong? Then just reply and tell me -- ideally
while also telling regzbot about it, as explained by the page listed in
the footer of this mail.

Developers: When fixing the issue, remember to add 'Link:' tags pointing
to the report (the parent of this mail). See page linked in footer for
details.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.
  
Georg Müller June 15, 2023, 2:02 p.m. UTC | #3
Hi all,

I have found a solution for this bug and will post a patch later today.

Am 09.06.23 um 14:21 schrieb Georg Müller:
>
> Am 01.11.22 um 14:48 schrieb Masami Hiramatsu (Google):
>> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>>
>> Fix to get the declared file name even if it uses file index 0
>> in DWARF5, using custom die_get_decl_file() function.
>>
>
> this patch breaks perf probe on fedora 38.
>

The problem is that die_get_file_name() uses the wrong cu_die.

I was pointed to the solution by reading elfutils commit e1db5cdc9:

     dwarf_decl_file uses dwarf_attr_integrate to get the DW_AT_decl_file
     attribute. This means the attribute might come from a different DIE
     in a different CU. If so, we need to use the CU associated with the
     attribute, not the original DIE, to resolve the file name.

The correct cu_die has to be obtained via

     dwarf_attr_integrate(dw_die, DW_AT_decl_file, &attr_mem)

and then cu_die from the cu from attr_mem (dwarf_cu_die(attr_mem.cu, &cu_die, ...)
to obtain it) has to be used.

Best regards,
Georg
  

Patch

diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index 216fc3d959e8..30b36b525681 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -123,7 +123,7 @@  int cu_find_lineinfo(Dwarf_Die *cu_die, Dwarf_Addr addr,
 	if (die_find_realfunc(cu_die, addr, &die_mem)
 	    && die_entrypc(&die_mem, &faddr) == 0 &&
 	    faddr == addr) {
-		*fname = dwarf_decl_file(&die_mem);
+		*fname = die_get_decl_file(&die_mem);
 		dwarf_decl_line(&die_mem, lineno);
 		goto out;
 	}
@@ -486,6 +486,19 @@  static int die_get_decl_fileno(Dwarf_Die *pdie)
 		return -ENOENT;
 }
 
+/* Return the file name by index */
+static const char *die_get_file_name(Dwarf_Die *dw_die, int idx)
+{
+	Dwarf_Die cu_die;
+	Dwarf_Files *files;
+
+	if (idx < 0 || !dwarf_diecu(dw_die, &cu_die, NULL, NULL) ||
+	    dwarf_getsrcfiles(&cu_die, &files, NULL) != 0)
+		return NULL;
+
+	return dwarf_filesrc(files, idx, NULL, NULL);
+}
+
 /**
  * die_get_call_file - Get callsite file name of inlined function instance
  * @in_die: a DIE of an inlined function instance
@@ -495,18 +508,22 @@  static int die_get_decl_fileno(Dwarf_Die *pdie)
  */
 const char *die_get_call_file(Dwarf_Die *in_die)
 {
-	Dwarf_Die cu_die;
-	Dwarf_Files *files;
-	int idx;
-
-	idx = die_get_call_fileno(in_die);
-	if (idx < 0 || !dwarf_diecu(in_die, &cu_die, NULL, NULL) ||
-	    dwarf_getsrcfiles(&cu_die, &files, NULL) != 0)
-		return NULL;
-
-	return dwarf_filesrc(files, idx, NULL, NULL);
+	return die_get_file_name(in_die, die_get_call_fileno(in_die));
 }
 
+/**
+ * die_get_decl_file - Find the declared file name of this DIE
+ * @dw_die: a DIE for something declared.
+ *
+ * Get declared file name of @dw_die.
+ * NOTE: Since some version of clang DWARF5 implementation incorrectly uses
+ * file index 0 for DW_AT_decl_file, die_get_decl_file() will return NULL for
+ * such cases. Use this function instead.
+ */
+const char *die_get_decl_file(Dwarf_Die *dw_die)
+{
+	return die_get_file_name(dw_die, die_get_decl_fileno(dw_die));
+}
 
 /**
  * die_find_child - Generic DIE search function in DIE tree
@@ -790,7 +807,7 @@  static int __die_walk_funclines_cb(Dwarf_Die *in_die, void *data)
 	}
 
 	if (addr) {
-		fname = dwarf_decl_file(in_die);
+		fname = die_get_decl_file(in_die);
 		if (fname && dwarf_decl_line(in_die, &lineno) == 0) {
 			lw->retval = lw->callback(fname, lineno, addr, lw->data);
 			if (lw->retval != 0)
@@ -818,7 +835,7 @@  static int __die_walk_funclines(Dwarf_Die *sp_die, bool recursive,
 	int lineno;
 
 	/* Handle function declaration line */
-	fname = dwarf_decl_file(sp_die);
+	fname = die_get_decl_file(sp_die);
 	if (fname && dwarf_decl_line(sp_die, &lineno) == 0 &&
 	    die_entrypc(sp_die, &addr) == 0) {
 		lw.retval = callback(fname, lineno, addr, data);
@@ -873,7 +890,7 @@  int die_walk_lines(Dwarf_Die *rt_die, line_walk_callback_t callback, void *data)
 	if (dwarf_tag(rt_die) != DW_TAG_compile_unit) {
 		cu_die = dwarf_diecu(rt_die, &die_mem, NULL, NULL);
 		dwarf_decl_line(rt_die, &decl);
-		decf = dwarf_decl_file(rt_die);
+		decf = die_get_decl_file(rt_die);
 		if (!decf) {
 			pr_debug2("Failed to get the declared file name of %s\n",
 				  dwarf_diename(rt_die));
@@ -928,7 +945,7 @@  int die_walk_lines(Dwarf_Die *rt_die, line_walk_callback_t callback, void *data)
 
 				dwarf_decl_line(&die_mem, &inl);
 				if (inl != decl ||
-				    decf != dwarf_decl_file(&die_mem))
+				    decf != die_get_decl_file(&die_mem))
 					continue;
 			}
 		}
diff --git a/tools/perf/util/dwarf-aux.h b/tools/perf/util/dwarf-aux.h
index 7ee0fa19b5c4..7ec8bc1083bb 100644
--- a/tools/perf/util/dwarf-aux.h
+++ b/tools/perf/util/dwarf-aux.h
@@ -50,6 +50,9 @@  int die_get_call_lineno(Dwarf_Die *in_die);
 /* Get callsite file name of inlined function instance */
 const char *die_get_call_file(Dwarf_Die *in_die);
 
+/* Get declared file name of a DIE */
+const char *die_get_decl_file(Dwarf_Die *dw_die);
+
 /* Get type die */
 Dwarf_Die *die_get_type(Dwarf_Die *vr_die, Dwarf_Die *die_mem);
 
diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index 1aa8fcc41c76..54b49ce85c9f 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -763,7 +763,7 @@  static int find_best_scope_cb(Dwarf_Die *fn_die, void *data)
 
 	/* Skip if declared file name does not match */
 	if (fsp->file) {
-		file = dwarf_decl_file(fn_die);
+		file = die_get_decl_file(fn_die);
 		if (!file || strcmp(fsp->file, file) != 0)
 			return 0;
 	}
@@ -1071,7 +1071,7 @@  static int probe_point_search_cb(Dwarf_Die *sp_die, void *data)
 		return DWARF_CB_OK;
 
 	/* Check declared file */
-	fname = dwarf_decl_file(sp_die);
+	fname = die_get_decl_file(sp_die);
 	if (!fname) {
 		pr_warning("A function DIE doesn't have decl_line. Maybe broken DWARF?\n");
 		return DWARF_CB_OK;
@@ -1151,7 +1151,7 @@  static int pubname_search_cb(Dwarf *dbg, Dwarf_Global *gl, void *data)
 				return DWARF_CB_OK;
 
 			if (param->file) {
-				fname = dwarf_decl_file(param->sp_die);
+				fname = die_get_decl_file(param->sp_die);
 				if (!fname || strtailcmp(param->file, fname))
 					return DWARF_CB_OK;
 			}
@@ -1750,7 +1750,7 @@  int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
 			goto post;
 		}
 
-		fname = dwarf_decl_file(&spdie);
+		fname = die_get_decl_file(&spdie);
 		if (addr == baseaddr) {
 			/* Function entry - Relative line number is 0 */
 			lineno = baseline;
@@ -1787,7 +1787,7 @@  int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
 			}
 		}
 		/* Verify the lineno and baseline are in a same file */
-		tmp = dwarf_decl_file(&spdie);
+		tmp = die_get_decl_file(&spdie);
 		if (!tmp || (fname && strcmp(tmp, fname) != 0))
 			lineno = 0;
 	}
@@ -1902,13 +1902,13 @@  static int line_range_search_cb(Dwarf_Die *sp_die, void *data)
 
 	/* Check declared file */
 	if (lr->file) {
-		fname = dwarf_decl_file(sp_die);
+		fname = die_get_decl_file(sp_die);
 		if (!fname || strtailcmp(lr->file, fname))
 			return DWARF_CB_OK;
 	}
 
 	if (die_match_name(sp_die, lr->function) && die_is_func_def(sp_die)) {
-		lf->fname = dwarf_decl_file(sp_die);
+		lf->fname = die_get_decl_file(sp_die);
 		dwarf_decl_line(sp_die, &lr->offset);
 		pr_debug("fname: %s, lineno:%d\n", lf->fname, lr->offset);
 		lf->lno_s = lr->offset + lr->start;