[PATCH v2 0/3] tools/perf: Fix perf probe crash by clang DWARF5 file

[PATCH v2 0/3] tools/perf: Fix perf probe crash by clang DWARF5 file

[Masami Hiramatsu (Google)]

Hi,

Here is the 2nd version of the patches for perf probe which improves the
robustness against clang DWARF5 file.

Since the Clang generates a bit different DWARF5 file, the perf probe
crashes or failes to analyze it. There are actually fragile code against
it, so I fixed it ([1/3]) to avoid crash by SEGV. And make it accepts
Clang's DWARF5 file ([2/3],[3/3]).

Without this series, the perf probe crashes with the DWARF5 file
which generated by clang as below;
 
  $ ./perf probe -k $BIN_PATH/vmlinux -s $SRC_PATH -L vfs_read:10
  Segmentation fault

This series fixes it to handle such file 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;


On the DWARF5 specification, Sec 2.14, there is
"The value 0 indicates that no source file has been specified."
for DW_AT_decl_file, but clang generated DWARF5 will use the value 0.

This issue is discussed on DWARF std ML;
https://www.mail-archive.com/dwarf-discuss@lists.dwarfstd.org/msg00884.html

And suggested that removing this part from the specification.
http://wiki.dwarfstd.org/index.php?title=DWARF5_Line_Table_File_Numbers

So as far as I understand, this is out of standard at this moment,
but the standard itself has a discussion on this part. And maybe updated
as currently clang does in the next release/revision.

Thank you,

---

Masami Hiramatsu (Google) (3):
      tools/perf: Fix to avoid crashing if DW_AT_decl_file is NULL
      tools/perf: Fix to use dwarf_attr_integrate for generic attr accessor
      tools/perf: Fix to get declared file name from clang DWARF5


 tools/perf/util/dwarf-aux.c    |   58 ++++++++++++++++++++++++++++------------
 tools/perf/util/dwarf-aux.h    |    3 ++
 tools/perf/util/probe-finder.c |   37 +++++++++++++++++---------
 3 files changed, 68 insertions(+), 30 deletions(-)

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

[PATCH v2 1/3] tools/perf: Fix to avoid crashing if DW_AT_decl_file is NULL

[Masami Hiramatsu (Google)]

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

Since Clang generates the DWARF5 which will set DW_AT_decl_file
as 0, dwarf_decl_file() thinks that is invalid and returns NULL.
In that case the perf probe will crash by SIGSEGV because it
doesn't expect the NULL decl_file.

This adds checks of the return value of dwarf_decl_file() to avoid
such SEGV with clang DWARF5 file.

Without this, perf probe crashes like below;
 $ ./perf probe -k $BIN_PATH/vmlinux -s $SRC_PATH -L vfs_read:10
 Segmentation fault

With this, perf probe just warns it;
 $ ./perf probe -k $BIN_PATH/vmlinux -s $SRC_PATH -L vfs_read:10
 Debuginfo analysis failed.
   Error: Failed to show lines.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 Changes in v2:
  - Update patch description.
---
 tools/perf/util/dwarf-aux.c    |    7 ++++++-
 tools/perf/util/probe-finder.c |   29 +++++++++++++++++++++--------
 2 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index 609ca1671501..406b7bdc851a 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -137,7 +137,7 @@ int cu_find_lineinfo(Dwarf_Die *cu_die, Dwarf_Addr addr,
 	}
 
 out:
-	return *lineno ?: -ENOENT;
+	return (*lineno && *fname) ? *lineno : -ENOENT;
 }
 
 static int __die_find_inline_cb(Dwarf_Die *die_mem, void *data);
@@ -874,6 +874,11 @@ int die_walk_lines(Dwarf_Die *rt_die, line_walk_callback_t callback, void *data)
 		cu_die = dwarf_diecu(rt_die, &die_mem, NULL, NULL);
 		dwarf_decl_line(rt_die, &decl);
 		decf = dwarf_decl_file(rt_die);
+		if (!decf) {
+			pr_debug2("Failed to get the declared file name of %s\n",
+				  dwarf_diename(rt_die));
+			return -EINVAL;
+		}
 	} else
 		cu_die = rt_die;
 	if (!cu_die) {
diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index 50d861a80f57..1aa8fcc41c76 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -1063,6 +1063,7 @@ static int probe_point_search_cb(Dwarf_Die *sp_die, void *data)
 	struct dwarf_callback_param *param = data;
 	struct probe_finder *pf = param->data;
 	struct perf_probe_point *pp = &pf->pev->point;
+	const char *fname;
 
 	/* Check tag and diename */
 	if (!die_is_func_def(sp_die) ||
@@ -1070,12 +1071,17 @@ static int probe_point_search_cb(Dwarf_Die *sp_die, void *data)
 		return DWARF_CB_OK;
 
 	/* Check declared file */
-	if (pp->file && strtailcmp(pp->file, dwarf_decl_file(sp_die)))
+	fname = dwarf_decl_file(sp_die);
+	if (!fname) {
+		pr_warning("A function DIE doesn't have decl_line. Maybe broken DWARF?\n");
+		return DWARF_CB_OK;
+	}
+	if (pp->file && fname && strtailcmp(pp->file, fname))
 		return DWARF_CB_OK;
 
 	pr_debug("Matched function: %s [%lx]\n", dwarf_diename(sp_die),
 		 (unsigned long)dwarf_dieoffset(sp_die));
-	pf->fname = dwarf_decl_file(sp_die);
+	pf->fname = fname;
 	if (pp->line) { /* Function relative line */
 		dwarf_decl_line(sp_die, &pf->lno);
 		pf->lno += pp->line;
@@ -1134,6 +1140,7 @@ struct pubname_callback_param {
 static int pubname_search_cb(Dwarf *dbg, Dwarf_Global *gl, void *data)
 {
 	struct pubname_callback_param *param = data;
+	const char *fname;
 
 	if (dwarf_offdie(dbg, gl->die_offset, param->sp_die)) {
 		if (dwarf_tag(param->sp_die) != DW_TAG_subprogram)
@@ -1143,9 +1150,11 @@ static int pubname_search_cb(Dwarf *dbg, Dwarf_Global *gl, void *data)
 			if (!dwarf_offdie(dbg, gl->cu_offset, param->cu_die))
 				return DWARF_CB_OK;
 
-			if (param->file &&
-			    strtailcmp(param->file, dwarf_decl_file(param->sp_die)))
-				return DWARF_CB_OK;
+			if (param->file) {
+				fname = dwarf_decl_file(param->sp_die);
+				if (!fname || strtailcmp(param->file, fname))
+					return DWARF_CB_OK;
+			}
 
 			param->found = 1;
 			return DWARF_CB_ABORT;
@@ -1779,7 +1788,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);
-		if (!tmp || strcmp(tmp, fname) != 0)
+		if (!tmp || (fname && strcmp(tmp, fname) != 0))
 			lineno = 0;
 	}
 
@@ -1889,10 +1898,14 @@ static int line_range_search_cb(Dwarf_Die *sp_die, void *data)
 	struct dwarf_callback_param *param = data;
 	struct line_finder *lf = param->data;
 	struct line_range *lr = lf->lr;
+	const char *fname;
 
 	/* Check declared file */
-	if (lr->file && strtailcmp(lr->file, dwarf_decl_file(sp_die)))
-		return DWARF_CB_OK;
+	if (lr->file) {
+		fname = dwarf_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);

[PATCH v2 2/3] tools/perf: Fix to use dwarf_attr_integrate for generic attr accessor

[Masami Hiramatsu (Google)]

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

Use dwarf_attr_integrate() instead of dwarf_attr() for generic attribute
acccessor functions, so that it can find the specified attribute from
abstact origin DIE etc.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 tools/perf/util/dwarf-aux.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index 406b7bdc851a..216fc3d959e8 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -308,7 +308,7 @@ static int die_get_attr_udata(Dwarf_Die *tp_die, unsigned int attr_name,
 {
 	Dwarf_Attribute attr;
 
-	if (dwarf_attr(tp_die, attr_name, &attr) == NULL ||
+	if (dwarf_attr_integrate(tp_die, attr_name, &attr) == NULL ||
 	    dwarf_formudata(&attr, result) != 0)
 		return -ENOENT;
 
@@ -321,7 +321,7 @@ static int die_get_attr_sdata(Dwarf_Die *tp_die, unsigned int attr_name,
 {
 	Dwarf_Attribute attr;
 
-	if (dwarf_attr(tp_die, attr_name, &attr) == NULL ||
+	if (dwarf_attr_integrate(tp_die, attr_name, &attr) == NULL ||
 	    dwarf_formsdata(&attr, result) != 0)
 		return -ENOENT;
 

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

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

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(-)

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;

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

[Georg Müller]

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

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

[Linux regression tracking #adding (Thorsten Leemhuis)]

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

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

[Linux regression tracking #update (Thorsten Leemhuis)]

[TLDR: This mail in primarily relevant for Linux kernel regression
tracking. See link in footer if these mails annoy you.]

On 15.06.23 13:42, Linux regression tracking #adding (Thorsten Leemhuis)
wrote:

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

#regzbot fix: c66e1c68c13b87250
#regzbot ignore-activity

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.

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

[Georg Müller]

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] perf probe: read DWARF files from the correct CU

[Georg Müller]

After switching from dwarf_decl_file() to die_get_decl_file(), it is not
possible to add probes for certain functions of certain binaries:

 $ 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.

The problem is that die_get_decl_file() uses the wrong CU to search for
the file. elfutils commit e1db5cdc9f has some good explanation for this:

    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.

This patch uses the same source of information as elfutils: use attribute
DW_AT_decl_file and use this CU to search for the file.

Fixes: dc9a5d2ccd5c ("perf probe: Fix to get declared file name from clang DWARF5")
Signed-off-by: Georg Müller <georgmueller@gmx.net>
Link: https://lore.kernel.org/r/5a00d5a5-7be7-ef8a-4044-9a16249fff25@gmx.net/
---
 tools/perf/util/dwarf-aux.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index b07414409771..137b3ed9897b 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -478,8 +478,10 @@ static const char *die_get_file_name(Dwarf_Die *dw_die, int idx)
 {
 	Dwarf_Die cu_die;
 	Dwarf_Files *files;
+	Dwarf_Attribute attr_mem;

-	if (idx < 0 || !dwarf_diecu(dw_die, &cu_die, NULL, NULL) ||
+	if (idx < 0 || !dwarf_attr_integrate(dw_die, DW_AT_decl_file, &attr_mem) ||
+	    !dwarf_cu_die(attr_mem.cu, &cu_die, NULL, NULL, NULL, NULL, NULL, NULL) ||
 	    dwarf_getsrcfiles(&cu_die, &files, NULL) != 0)
 		return NULL;

--
2.41.0

Re: [PATCH] perf probe: read DWARF files from the correct CU

[Georg Müller]

Hi,

Am 15.06.23 um 22:01 schrieb Georg Müller:
> After switching from dwarf_decl_file() to die_get_decl_file(), it is not
> possible to add probes for certain functions of certain binaries:
>
>   $ 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.
>
> The problem is that die_get_decl_file() uses the wrong CU to search for
> the file. elfutils commit e1db5cdc9f has some good explanation for this:
>
>      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.
>
> This patch uses the same source of information as elfutils: use attribute
> DW_AT_decl_file and use this CU to search for the file.
>
> Fixes: dc9a5d2ccd5c ("perf probe: Fix to get declared file name from clang DWARF5")
> Signed-off-by: Georg Müller <georgmueller@gmx.net>
> Link: https://lore.kernel.org/r/5a00d5a5-7be7-ef8a-4044-9a16249fff25@gmx.net/
> ---
>   tools/perf/util/dwarf-aux.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index b07414409771..137b3ed9897b 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
> @@ -478,8 +478,10 @@ static const char *die_get_file_name(Dwarf_Die *dw_die, int idx)
>   {
>   	Dwarf_Die cu_die;
>   	Dwarf_Files *files;
> +	Dwarf_Attribute attr_mem;
>
> -	if (idx < 0 || !dwarf_diecu(dw_die, &cu_die, NULL, NULL) ||
> +	if (idx < 0 || !dwarf_attr_integrate(dw_die, DW_AT_decl_file, &attr_mem) ||
> +	    !dwarf_cu_die(attr_mem.cu, &cu_die, NULL, NULL, NULL, NULL, NULL, NULL) ||
>   	    dwarf_getsrcfiles(&cu_die, &files, NULL) != 0)
>   		return NULL;
>
> --
> 2.41.0
>


Just a question regarding the patch:
Should I also add a testcase to show the issue or is this enough? It can be
reproduced quite simple with two .c files and one .h file, the same test as the
one added in elfutils with commit e1db5cdc9f, compiled with the same three steps
as in the comment of the shell script, but without "-flto":

https://sourceware.org/git/?p=elfutils.git;a=commitdiff;h=e1db5cdc9f230f8de4df1a0f38dca69b283ee57a

Best regards,
Georg

Re: [PATCH] perf probe: read DWARF files from the correct CU

[Arnaldo Carvalho de Melo]

Em Thu, Jun 15, 2023 at 10:01:37PM +0200, Georg Müller escreveu:
> After switching from dwarf_decl_file() to die_get_decl_file(), it is not
> possible to add probes for certain functions of certain binaries:
> 
>  $ 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.
> 
> The problem is that die_get_decl_file() uses the wrong CU to search for
> the file. elfutils commit e1db5cdc9f has some good explanation for this:
> 
>     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.
> 
> This patch uses the same source of information as elfutils: use attribute
> DW_AT_decl_file and use this CU to search for the file.

Thanks, applied to the perf-tools branch, that will be submitted for
Linux v6.5.

- Arnaldo
 
> Fixes: dc9a5d2ccd5c ("perf probe: Fix to get declared file name from clang DWARF5")
> Signed-off-by: Georg Müller <georgmueller@gmx.net>
> Link: https://lore.kernel.org/r/5a00d5a5-7be7-ef8a-4044-9a16249fff25@gmx.net/
> ---
>  tools/perf/util/dwarf-aux.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index b07414409771..137b3ed9897b 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
> @@ -478,8 +478,10 @@ static const char *die_get_file_name(Dwarf_Die *dw_die, int idx)
>  {
>  	Dwarf_Die cu_die;
>  	Dwarf_Files *files;
> +	Dwarf_Attribute attr_mem;
> 
> -	if (idx < 0 || !dwarf_diecu(dw_die, &cu_die, NULL, NULL) ||
> +	if (idx < 0 || !dwarf_attr_integrate(dw_die, DW_AT_decl_file, &attr_mem) ||
> +	    !dwarf_cu_die(attr_mem.cu, &cu_die, NULL, NULL, NULL, NULL, NULL, NULL) ||
>  	    dwarf_getsrcfiles(&cu_die, &files, NULL) != 0)
>  		return NULL;
> 
> --
> 2.41.0
> 

-- 

- Arnaldo

Re: [PATCH] perf probe: read DWARF files from the correct CU

[Masami Hiramatsu]

On Tue, 11 Jul 2023 09:57:45 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Em Thu, Jun 15, 2023 at 10:01:37PM +0200, Georg Müller escreveu:
> > After switching from dwarf_decl_file() to die_get_decl_file(), it is not
> > possible to add probes for certain functions of certain binaries:
> > 
> >  $ 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.
> > 
> > The problem is that die_get_decl_file() uses the wrong CU to search for
> > the file. elfutils commit e1db5cdc9f has some good explanation for this:
> > 
> >     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.
> > 
> > This patch uses the same source of information as elfutils: use attribute
> > DW_AT_decl_file and use this CU to search for the file.
> 
> Thanks, applied to the perf-tools branch, that will be submitted for
> Linux v6.5.

Thanks Arnaldo and Georg,

It sounds perfect reason why that happened. I didn't expect the case that
the attribute comes from another CU...

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

> 
> - Arnaldo
>  
> > Fixes: dc9a5d2ccd5c ("perf probe: Fix to get declared file name from clang DWARF5")
> > Signed-off-by: Georg Müller <georgmueller@gmx.net>
> > Link: https://lore.kernel.org/r/5a00d5a5-7be7-ef8a-4044-9a16249fff25@gmx.net/
> > ---
> >  tools/perf/util/dwarf-aux.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> > index b07414409771..137b3ed9897b 100644
> > --- a/tools/perf/util/dwarf-aux.c
> > +++ b/tools/perf/util/dwarf-aux.c
> > @@ -478,8 +478,10 @@ static const char *die_get_file_name(Dwarf_Die *dw_die, int idx)
> >  {
> >  	Dwarf_Die cu_die;
> >  	Dwarf_Files *files;
> > +	Dwarf_Attribute attr_mem;
> > 
> > -	if (idx < 0 || !dwarf_diecu(dw_die, &cu_die, NULL, NULL) ||
> > +	if (idx < 0 || !dwarf_attr_integrate(dw_die, DW_AT_decl_file, &attr_mem) ||
> > +	    !dwarf_cu_die(attr_mem.cu, &cu_die, NULL, NULL, NULL, NULL, NULL, NULL) ||
> >  	    dwarf_getsrcfiles(&cu_die, &files, NULL) != 0)
> >  		return NULL;
> > 
> > --
> > 2.41.0
> > 
> 
> -- 
> 
> - Arnaldo


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

Re: [PATCH] perf probe: read DWARF files from the correct CU

[Arnaldo Carvalho de Melo]

Em Tue, Jul 11, 2023 at 10:20:00PM +0900, Masami Hiramatsu escreveu:
> On Tue, 11 Jul 2023 09:57:45 -0300 Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > Em Thu, Jun 15, 2023 at 10:01:37PM +0200, Georg Müller escreveu:
> > > The problem is that die_get_decl_file() uses the wrong CU to search for
> > > the file. elfutils commit e1db5cdc9f has some good explanation for this:
> > > 
> > >     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.
> > > 
> > > This patch uses the same source of information as elfutils: use attribute
> > > DW_AT_decl_file and use this CU to search for the file.
> > 
> > Thanks, applied to the perf-tools branch, that will be submitted for
> > Linux v6.5.
 
> Thanks Arnaldo and Georg,
 
> It sounds perfect reason why that happened. I didn't expect the case that
> the attribute comes from another CU...
 
> Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
 
Thanks, added it to the cset.

- Arnaldo

[PATCH v2 0/2] perf probe: fix regression introduced by switch to die_get_decl_file

[Georg Müller]

When switching from dwarf_decl_file() to die_get_decl_file(), a regression
was introduced when having a binary where the DWARF info is split to
multiple CUs. It is not possible to add probes to certain functions.

These patches introduce a testcase which shows the current regression
and a fix for the issue

Signed-off-by: Georg Müller <georgmueller@gmx.net>
Link: https://lore.kernel.org/r/5a00d5a5-7be7-ef8a-4044-9a16249fff25@gmx.net/

---
Changes in v2:
 - Add testcase

Georg Müller (2):
  perf probe: add test for regression introduced by switch to
    die_get_decl_file
  perf probe: read DWARF files from the correct CU

 .../shell/test_uprobe_from_different_cu.sh    | 77 +++++++++++++++++++
 tools/perf/util/dwarf-aux.c                   |  4 +-
 2 files changed, 80 insertions(+), 1 deletion(-)
 create mode 100755 tools/perf/tests/shell/test_uprobe_from_different_cu.sh

--
2.41.0

[PATCH v2 1/2] perf probe: add test for regression introduced by switch to die_get_decl_file

[Georg Müller]

This patch adds a test to validate that perf probe works for binaries
where DWARF info is split into multiple CUs

Signed-off-by: Georg Müller <georgmueller@gmx.net>
Link: https://lore.kernel.org/r/5a00d5a5-7be7-ef8a-4044-9a16249fff25@gmx.net/
---
 .../shell/test_uprobe_from_different_cu.sh    | 77 +++++++++++++++++++
 1 file changed, 77 insertions(+)
 create mode 100755 tools/perf/tests/shell/test_uprobe_from_different_cu.sh

diff --git a/tools/perf/tests/shell/test_uprobe_from_different_cu.sh b/tools/perf/tests/shell/test_uprobe_from_different_cu.sh
new file mode 100755
index 000000000000..00d2e0e2e0c2
--- /dev/null
+++ b/tools/perf/tests/shell/test_uprobe_from_different_cu.sh
@@ -0,0 +1,77 @@
+#!/bin/bash
+# test perf probe of function from different CU
+# SPDX-License-Identifier: GPL-2.0
+
+set -e
+
+temp_dir=$(mktemp -d /tmp/perf-uprobe-different-cu-sh.XXXXXXXXXX)
+
+cleanup()
+{
+	trap - EXIT TERM INT
+	if [[ "${temp_dir}" =~ ^/tmp/perf-uprobe-different-cu-sh.*$ ]]; then
+		echo "--- Cleaning up ---"
+		perf probe -x ${temp_dir}/testfile -d foo
+		rm -f "${temp_dir}/"*
+		rmdir "${temp_dir}"
+	fi
+}
+
+trap_cleanup()
+{
+        cleanup
+        exit 1
+}
+
+trap trap_cleanup EXIT TERM INT
+
+cat > ${temp_dir}/testfile-foo.h << EOF
+struct t
+{
+  int *p;
+  int c;
+};
+
+extern int foo (int i, struct t *t);
+EOF
+
+cat > ${temp_dir}/testfile-foo.c << EOF
+#include "testfile-foo.h"
+
+int
+foo (int i, struct t *t)
+{
+  int j, res = 0;
+  for (j = 0; j < i && j < t->c; j++)
+    res += t->p[j];
+
+  return res;
+}
+EOF
+
+cat > ${temp_dir}/testfile-main.c << EOF
+#include "testfile-foo.h"
+
+static struct t g;
+
+int
+main (int argc, char **argv)
+{
+  int i;
+  int j[argc];
+  g.c = argc;
+  g.p = j;
+  for (i = 0; i < argc; i++)
+    j[i] = (int) argv[i][0];
+  return foo (3, &g);
+}
+EOF
+
+gcc -g -Og -flto -c ${temp_dir}/testfile-foo.c -o ${temp_dir}/testfile-foo.o
+gcc -g -Og -c ${temp_dir}/testfile-main.c -o ${temp_dir}/testfile-main.o
+gcc -g -Og -o ${temp_dir}/testfile ${temp_dir}/testfile-foo.o ${temp_dir}/testfile-main.o
+
+perf probe -x ${temp_dir}/testfile --funcs foo
+perf probe -x ${temp_dir}/testfile foo
+
+cleanup
--
2.41.0

Re: [PATCH v2 1/2] perf probe: add test for regression introduced by switch to die_get_decl_file

[Ian Rogers]

On Wed, Jun 28, 2023 at 1:25 AM Georg Müller <georgmueller@gmx.net> wrote:
>
> This patch adds a test to validate that perf probe works for binaries
> where DWARF info is split into multiple CUs
>
> Signed-off-by: Georg Müller <georgmueller@gmx.net>
> Link: https://lore.kernel.org/r/5a00d5a5-7be7-ef8a-4044-9a16249fff25@gmx.net/
> ---
>  .../shell/test_uprobe_from_different_cu.sh    | 77 +++++++++++++++++++
>  1 file changed, 77 insertions(+)
>  create mode 100755 tools/perf/tests/shell/test_uprobe_from_different_cu.sh
>
> diff --git a/tools/perf/tests/shell/test_uprobe_from_different_cu.sh b/tools/perf/tests/shell/test_uprobe_from_different_cu.sh
> new file mode 100755
> index 000000000000..00d2e0e2e0c2
> --- /dev/null
> +++ b/tools/perf/tests/shell/test_uprobe_from_different_cu.sh
> @@ -0,0 +1,77 @@
> +#!/bin/bash
> +# test perf probe of function from different CU
> +# SPDX-License-Identifier: GPL-2.0
> +
> +set -e
> +
> +temp_dir=$(mktemp -d /tmp/perf-uprobe-different-cu-sh.XXXXXXXXXX)
> +
> +cleanup()
> +{
> +       trap - EXIT TERM INT
> +       if [[ "${temp_dir}" =~ ^/tmp/perf-uprobe-different-cu-sh.*$ ]]; then
> +               echo "--- Cleaning up ---"
> +               perf probe -x ${temp_dir}/testfile -d foo
> +               rm -f "${temp_dir}/"*
> +               rmdir "${temp_dir}"
> +       fi
> +}
> +
> +trap_cleanup()
> +{
> +        cleanup
> +        exit 1
> +}
> +
> +trap trap_cleanup EXIT TERM INT
> +
> +cat > ${temp_dir}/testfile-foo.h << EOF
> +struct t
> +{
> +  int *p;
> +  int c;
> +};
> +
> +extern int foo (int i, struct t *t);
> +EOF
> +
> +cat > ${temp_dir}/testfile-foo.c << EOF
> +#include "testfile-foo.h"
> +
> +int
> +foo (int i, struct t *t)
> +{
> +  int j, res = 0;
> +  for (j = 0; j < i && j < t->c; j++)
> +    res += t->p[j];
> +
> +  return res;
> +}
> +EOF
> +
> +cat > ${temp_dir}/testfile-main.c << EOF
> +#include "testfile-foo.h"
> +
> +static struct t g;
> +
> +int
> +main (int argc, char **argv)
> +{
> +  int i;
> +  int j[argc];
> +  g.c = argc;
> +  g.p = j;
> +  for (i = 0; i < argc; i++)
> +    j[i] = (int) argv[i][0];
> +  return foo (3, &g);
> +}
> +EOF
> +
> +gcc -g -Og -flto -c ${temp_dir}/testfile-foo.c -o ${temp_dir}/testfile-foo.o
> +gcc -g -Og -c ${temp_dir}/testfile-main.c -o ${temp_dir}/testfile-main.o
> +gcc -g -Og -o ${temp_dir}/testfile ${temp_dir}/testfile-foo.o ${temp_dir}/testfile-main.o

Thanks for the test Georg! By directly relying on gcc this test fails
for me in some constrained environments, like containers. I think
there should be a skip if gcc isn't present. A different option is to
just build the test code into the perf binary itself as a test
workload:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/workloads?h=perf-tools-next

Wdyt? Thanks,
Ian

> +
> +perf probe -x ${temp_dir}/testfile --funcs foo
> +perf probe -x ${temp_dir}/testfile foo
> +
> +cleanup
> --
> 2.41.0
>

Re: [PATCH v2 1/2] perf probe: add test for regression introduced by switch to die_get_decl_file

[Georg Müller]

Am 27.07.23 um 19:45 schrieb Ian Rogers:
> On Wed, Jun 28, 2023 at 1:25 AM Georg Müller <georgmueller@gmx.net> wrote:
>
> Thanks for the test Georg! By directly relying on gcc this test fails
> for me in some constrained environments, like containers. I think
> there should be a skip if gcc isn't present. A different option is to
> just build the test code into the perf binary itself as a test
> workload:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/workloads?h=perf-tools-next
>
> Wdyt? Thanks,
> Ian
>

I prepare a commit which checks for gcc and skips the test in this case.

I think building thi test code into the perf binary itself is not an option
here, since the test relies on a special setup of using -flto for one of the
compilation units.

There is also a cleanup issue if anything fails. This will be included in
the patch as well.

Best regards,
Georg

[PATCH v2 2/2] perf probe: read DWARF files from the correct CU

[Georg Müller]

After switching from dwarf_decl_file() to die_get_decl_file(), it is not
possible to add probes for certain functions:

 $ 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.

The problem is that die_get_decl_file() uses the wrong CU to search for
the file. elfutils commit e1db5cdc9f has some good explanation for this:

    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.

This patch uses the same source of information as elfutils: use attribute
DW_AT_decl_file and use this CU to search for the file.

Fixes: dc9a5d2ccd5c ("perf probe: Fix to get declared file name from clang DWARF5")
Signed-off-by: Georg Müller <georgmueller@gmx.net>
Link: https://lore.kernel.org/r/5a00d5a5-7be7-ef8a-4044-9a16249fff25@gmx.net/
---
 tools/perf/util/dwarf-aux.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index b07414409771..137b3ed9897b 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -478,8 +478,10 @@ static const char *die_get_file_name(Dwarf_Die *dw_die, int idx)
 {
 	Dwarf_Die cu_die;
 	Dwarf_Files *files;
+	Dwarf_Attribute attr_mem;

-	if (idx < 0 || !dwarf_diecu(dw_die, &cu_die, NULL, NULL) ||
+	if (idx < 0 || !dwarf_attr_integrate(dw_die, DW_AT_decl_file, &attr_mem) ||
+	    !dwarf_cu_die(attr_mem.cu, &cu_die, NULL, NULL, NULL, NULL, NULL, NULL) ||
 	    dwarf_getsrcfiles(&cu_die, &files, NULL) != 0)
 		return NULL;

--
2.41.0

Re: [PATCH v2 0/2] perf probe: fix regression introduced by switch to die_get_decl_file

[Linux regression tracking (Thorsten Leemhuis)]

On 28.06.23 10:23, Georg Müller wrote:
> When switching from dwarf_decl_file() to die_get_decl_file(), a regression
> was introduced when having a binary where the DWARF info is split to
> multiple CUs. It is not possible to add probes to certain functions.
> 
> These patches introduce a testcase which shows the current regression
> and a fix for the issue
> 
> Signed-off-by: Georg Müller <georgmueller@gmx.net>
> Link: https://lore.kernel.org/r/5a00d5a5-7be7-ef8a-4044-9a16249fff25@gmx.net/

FWIW, you in both commits might want to add a

 Cc: stable@vger.kernel.org

to ensure the fix is backported to appropriate releases (e.g. 6.4 and
6.3, if the latter doesn't go EOL earlier).

Ciao, Thorsten

P.S.: Some (I guess: most) maintainers prefer if patch series are
startet as new threads

Re: [PATCH v2 0/3] tools/perf: Fix perf probe crash by clang DWARF5 file

[Namhyung Kim]

Hi Masami,

On Tue, Nov 1, 2022 at 6:48 AM Masami Hiramatsu (Google)
<mhiramat@kernel.org> wrote:
>
> Hi,
>
> Here is the 2nd version of the patches for perf probe which improves the
> robustness against clang DWARF5 file.
>
> Since the Clang generates a bit different DWARF5 file, the perf probe
> crashes or failes to analyze it. There are actually fragile code against
> it, so I fixed it ([1/3]) to avoid crash by SEGV. And make it accepts
> Clang's DWARF5 file ([2/3],[3/3]).
>
> Without this series, the perf probe crashes with the DWARF5 file
> which generated by clang as below;
>
>   $ ./perf probe -k $BIN_PATH/vmlinux -s $SRC_PATH -L vfs_read:10
>   Segmentation fault
>
> This series fixes it to handle such file 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;
>
>
> On the DWARF5 specification, Sec 2.14, there is
> "The value 0 indicates that no source file has been specified."
> for DW_AT_decl_file, but clang generated DWARF5 will use the value 0.
>
> This issue is discussed on DWARF std ML;
> https://www.mail-archive.com/dwarf-discuss@lists.dwarfstd.org/msg00884.html
>
> And suggested that removing this part from the specification.
> http://wiki.dwarfstd.org/index.php?title=DWARF5_Line_Table_File_Numbers
>
> So as far as I understand, this is out of standard at this moment,
> but the standard itself has a discussion on this part. And maybe updated
> as currently clang does in the next release/revision.
>
> Thank you,
>
> ---
>
> Masami Hiramatsu (Google) (3):
>       tools/perf: Fix to avoid crashing if DW_AT_decl_file is NULL
>       tools/perf: Fix to use dwarf_attr_integrate for generic attr accessor
>       tools/perf: Fix to get declared file name from clang DWARF5

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung


>
>
>  tools/perf/util/dwarf-aux.c    |   58 ++++++++++++++++++++++++++++------------
>  tools/perf/util/dwarf-aux.h    |    3 ++
>  tools/perf/util/probe-finder.c |   37 +++++++++++++++++---------
>  3 files changed, 68 insertions(+), 30 deletions(-)
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v2 0/3] tools/perf: Fix perf probe crash by clang DWARF5 file

[Arnaldo Carvalho de Melo]

Em Tue, Nov 01, 2022 at 05:03:51PM -0700, Namhyung Kim escreveu:
> On Tue, Nov 1, 2022 at 6:48 AM Masami Hiramatsu (Google)
> <mhiramat@kernel.org> wrote:
> > Here is the 2nd version of the patches for perf probe which improves the
> > robustness against clang DWARF5 file.
> >
> > Since the Clang generates a bit different DWARF5 file, the perf probe
> > crashes or failes to analyze it. There are actually fragile code against
> > it, so I fixed it ([1/3]) to avoid crash by SEGV. And make it accepts
> > Clang's DWARF5 file ([2/3],[3/3]).

> > Masami Hiramatsu (Google) (3):
> >       tools/perf: Fix to avoid crashing if DW_AT_decl_file is NULL
> >       tools/perf: Fix to use dwarf_attr_integrate for generic attr accessor
> >       tools/perf: Fix to get declared file name from clang DWARF5
> 
> Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks, applied.

- Arnaldo