Perf tool regression from 07d369857808

Perf tool regression from 07d369857808

[Alexandre Ghiti]

Hi Masami,

The commit 07d369857808 ("perf probe: Fix wrong address verification) 
found its way in kernel 4.19.98 (and debian 10) and I observed some 
regressions when I try to add probes in shared libraries.
The symptoms are:

$ perf probe -f --exec=/home/aghiti/lib/libdpuhw.so --add 
'log_rank_path=log_rank_path rank path:string'
   Failed to find symbol at 0x3bf0
     Error: Failed to add events.

Whereas when I revert this patch, on the same shared library:

$ perf probe -f --exec=/home/aghiti/lib/libdpuhw.so --add 
'log_rank_path=log_rank_path rank path:string'
Added new event:
   probe_libdpuhw:log_rank_path_4 (on log_rank_path in 
/home/aghiti/lib/libdpuhw.so.2020.1.0 with rank path:string)

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

	perf record -e probe_libdpuhw:log_rank_path_4 -aR sleep 1

Actually, I noted that this patch reverts a previous patch that stated 
that dwfl_module_addrsym could fail on shared libraries 664fee3dc379 
("perf probe: Do not use dwfl_module_addrsym if dwarf_diename finds 
symbol name").

Let me know if I can be of any help,

Thanks,

Alexandre Ghiti

Re: Perf tool regression from 07d369857808

[Masami Hiramatsu]

Hi,

On Thu, 27 Feb 2020 13:23:19 +0100
Alexandre Ghiti <alex@ghiti.fr> wrote:

> Hi Masami,
> 
> The commit 07d369857808 ("perf probe: Fix wrong address verification) 
> found its way in kernel 4.19.98 (and debian 10) and I observed some 
> regressions when I try to add probes in shared libraries.
> The symptoms are:
> 
> $ perf probe -f --exec=/home/aghiti/lib/libdpuhw.so --add 
> 'log_rank_path=log_rank_path rank path:string'
>    Failed to find symbol at 0x3bf0
>      Error: Failed to add events.

Hm...

> 
> Whereas when I revert this patch, on the same shared library:
> 
> $ perf probe -f --exec=/home/aghiti/lib/libdpuhw.so --add 
> 'log_rank_path=log_rank_path rank path:string'
> Added new event:
>    probe_libdpuhw:log_rank_path_4 (on log_rank_path in 
> /home/aghiti/lib/libdpuhw.so.2020.1.0 with rank path:string)
> 
> You can now use it in all perf tools, such as:
> 
> 	perf record -e probe_libdpuhw:log_rank_path_4 -aR sleep 1
> 
> Actually, I noted that this patch reverts a previous patch that stated 
> that dwfl_module_addrsym could fail on shared libraries 664fee3dc379 
> ("perf probe: Do not use dwfl_module_addrsym if dwarf_diename finds 
> symbol name").

Ah, OK. Hmm, actually, the reason why reverted it was the actuall
address of symbol is unclear if the DIE only has address range.
OK, at first try to find entrypc and if not, try dwlf_module_addrsym().
That will be better idea.

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[BUGFIX PATCH] perf probe: Do not depend on dwfl_module_addrsym()

[Masami Hiramatsu]

Do not depend on dwfl_module_addrsym() because it can fail
on user-space shared libraries.

Actually, same bug was fixed by commit 664fee3dc379 ("perf
probe: Do not use dwfl_module_addrsym if dwarf_diename finds
symbol name"), but commit 07d369857808 ("perf probe: Fix
wrong address verification) reverted to get actual symbol
address from symtab.

This fixes it again by getting symbol address from DIE,
and only if the DIE has only address range, it uses
dwfl_module_addrsym().

Fixes: 07d369857808 ("perf probe: Fix wrong address verification)
Reported-by: Alexandre Ghiti <alex@ghiti.fr>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/perf/util/probe-finder.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index 1c817add6ca4..e4cff49384f4 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -637,14 +637,19 @@ static int convert_to_trace_point(Dwarf_Die *sp_die, Dwfl_Module *mod,
 		return -EINVAL;
 	}
 
-	/* Try to get actual symbol name from symtab */
-	symbol = dwfl_module_addrsym(mod, paddr, &sym, NULL);
+	if (dwarf_entrypc(sp_die, &eaddr) == 0) {
+		/* If the DIE has entrypc, use it. */
+		symbol = dwarf_diename(sp_die);
+	} else {
+		/* Try to get actual symbol name and address from symtab */
+		symbol = dwfl_module_addrsym(mod, paddr, &sym, NULL);
+		eaddr = sym.st_value;
+	}
 	if (!symbol) {
 		pr_warning("Failed to find symbol at 0x%lx\n",
 			   (unsigned long)paddr);
 		return -ENOENT;
 	}
-	eaddr = sym.st_value;
 
 	tp->offset = (unsigned long)(paddr - eaddr);
 	tp->address = (unsigned long)paddr;

Re: [BUGFIX PATCH] perf probe: Do not depend on dwfl_module_addrsym()

[Alexandre Ghiti]

On 2/27/20 4:42 PM, Masami Hiramatsu wrote:
> Do not depend on dwfl_module_addrsym() because it can fail
> on user-space shared libraries.
> 
> Actually, same bug was fixed by commit 664fee3dc379 ("perf
> probe: Do not use dwfl_module_addrsym if dwarf_diename finds
> symbol name"), but commit 07d369857808 ("perf probe: Fix
> wrong address verification) reverted to get actual symbol
> address from symtab.
> 
> This fixes it again by getting symbol address from DIE,
> and only if the DIE has only address range, it uses
> dwfl_module_addrsym().
> 
> Fixes: 07d369857808 ("perf probe: Fix wrong address verification)
> Reported-by: Alexandre Ghiti <alex@ghiti.fr>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>   tools/perf/util/probe-finder.c |   11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> index 1c817add6ca4..e4cff49384f4 100644
> --- a/tools/perf/util/probe-finder.c
> +++ b/tools/perf/util/probe-finder.c
> @@ -637,14 +637,19 @@ static int convert_to_trace_point(Dwarf_Die *sp_die, Dwfl_Module *mod,
>   		return -EINVAL;
>   	}
>   
> -	/* Try to get actual symbol name from symtab */
> -	symbol = dwfl_module_addrsym(mod, paddr, &sym, NULL);
> +	if (dwarf_entrypc(sp_die, &eaddr) == 0) {
> +		/* If the DIE has entrypc, use it. */
> +		symbol = dwarf_diename(sp_die);
> +	} else {
> +		/* Try to get actual symbol name and address from symtab */
> +		symbol = dwfl_module_addrsym(mod, paddr, &sym, NULL);
> +		eaddr = sym.st_value;
> +	}
>   	if (!symbol) {
>   		pr_warning("Failed to find symbol at 0x%lx\n",
>   			   (unsigned long)paddr);
>   		return -ENOENT;
>   	}
> -	eaddr = sym.st_value;
>   
>   	tp->offset = (unsigned long)(paddr - eaddr);
>   	tp->address = (unsigned long)paddr;
> 

I have just tested your patch, that fixes the issue, so you can add:

Tested-by: Alexandre Ghiti <alex@ghiti.fr>

I added stable in cc.

Thanks,

Alex

Re: [BUGFIX PATCH] perf probe: Do not depend on dwfl_module_addrsym()

[Masami Hiramatsu]

On Thu, 27 Feb 2020 17:20:39 +0100
Alexandre Ghiti <alex@ghiti.fr> wrote:

> On 2/27/20 4:42 PM, Masami Hiramatsu wrote:
> > Do not depend on dwfl_module_addrsym() because it can fail
> > on user-space shared libraries.
> > 
> > Actually, same bug was fixed by commit 664fee3dc379 ("perf
> > probe: Do not use dwfl_module_addrsym if dwarf_diename finds
> > symbol name"), but commit 07d369857808 ("perf probe: Fix
> > wrong address verification) reverted to get actual symbol
> > address from symtab.
> > 
> > This fixes it again by getting symbol address from DIE,
> > and only if the DIE has only address range, it uses
> > dwfl_module_addrsym().
> > 
> > Fixes: 07d369857808 ("perf probe: Fix wrong address verification)
> > Reported-by: Alexandre Ghiti <alex@ghiti.fr>
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >   tools/perf/util/probe-finder.c |   11 ++++++++---
> >   1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> > index 1c817add6ca4..e4cff49384f4 100644
> > --- a/tools/perf/util/probe-finder.c
> > +++ b/tools/perf/util/probe-finder.c
> > @@ -637,14 +637,19 @@ static int convert_to_trace_point(Dwarf_Die *sp_die, Dwfl_Module *mod,
> >   		return -EINVAL;
> >   	}
> >   
> > -	/* Try to get actual symbol name from symtab */
> > -	symbol = dwfl_module_addrsym(mod, paddr, &sym, NULL);
> > +	if (dwarf_entrypc(sp_die, &eaddr) == 0) {
> > +		/* If the DIE has entrypc, use it. */
> > +		symbol = dwarf_diename(sp_die);
> > +	} else {
> > +		/* Try to get actual symbol name and address from symtab */
> > +		symbol = dwfl_module_addrsym(mod, paddr, &sym, NULL);
> > +		eaddr = sym.st_value;
> > +	}
> >   	if (!symbol) {
> >   		pr_warning("Failed to find symbol at 0x%lx\n",
> >   			   (unsigned long)paddr);
> >   		return -ENOENT;
> >   	}
> > -	eaddr = sym.st_value;
> >   
> >   	tp->offset = (unsigned long)(paddr - eaddr);
> >   	tp->address = (unsigned long)paddr;
> > 
> 
> I have just tested your patch, that fixes the issue, so you can add:
> 
> Tested-by: Alexandre Ghiti <alex@ghiti.fr>

Thanks Alexandre,
Arnaldo, could you also pick this for fix?

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [BUGFIX PATCH] perf probe: Do not depend on dwfl_module_addrsym()

[Arnaldo Carvalho de Melo]

Em Fri, Feb 28, 2020 at 08:52:38AM +0900, Masami Hiramatsu escreveu:
> On Thu, 27 Feb 2020 17:20:39 +0100
> Alexandre Ghiti <alex@ghiti.fr> wrote:
> 
> > On 2/27/20 4:42 PM, Masami Hiramatsu wrote:
> > > Do not depend on dwfl_module_addrsym() because it can fail
> > > on user-space shared libraries.
> > > 
> > > Actually, same bug was fixed by commit 664fee3dc379 ("perf
> > > probe: Do not use dwfl_module_addrsym if dwarf_diename finds
> > > symbol name"), but commit 07d369857808 ("perf probe: Fix
> > > wrong address verification) reverted to get actual symbol
> > > address from symtab.
> > > 
> > > This fixes it again by getting symbol address from DIE,
> > > and only if the DIE has only address range, it uses
> > > dwfl_module_addrsym().
> > > 
> > > Fixes: 07d369857808 ("perf probe: Fix wrong address verification)
> > > Reported-by: Alexandre Ghiti <alex@ghiti.fr>
> > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > ---
> > >   tools/perf/util/probe-finder.c |   11 ++++++++---
> > >   1 file changed, 8 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> > > index 1c817add6ca4..e4cff49384f4 100644
> > > --- a/tools/perf/util/probe-finder.c
> > > +++ b/tools/perf/util/probe-finder.c
> > > @@ -637,14 +637,19 @@ static int convert_to_trace_point(Dwarf_Die *sp_die, Dwfl_Module *mod,
> > >   		return -EINVAL;
> > >   	}
> > >   
> > > -	/* Try to get actual symbol name from symtab */
> > > -	symbol = dwfl_module_addrsym(mod, paddr, &sym, NULL);
> > > +	if (dwarf_entrypc(sp_die, &eaddr) == 0) {
> > > +		/* If the DIE has entrypc, use it. */
> > > +		symbol = dwarf_diename(sp_die);
> > > +	} else {
> > > +		/* Try to get actual symbol name and address from symtab */
> > > +		symbol = dwfl_module_addrsym(mod, paddr, &sym, NULL);
> > > +		eaddr = sym.st_value;
> > > +	}
> > >   	if (!symbol) {
> > >   		pr_warning("Failed to find symbol at 0x%lx\n",
> > >   			   (unsigned long)paddr);
> > >   		return -ENOENT;
> > >   	}
> > > -	eaddr = sym.st_value;
> > >   
> > >   	tp->offset = (unsigned long)(paddr - eaddr);
> > >   	tp->address = (unsigned long)paddr;
> > > 
> > 
> > I have just tested your patch, that fixes the issue, so you can add:
> > 
> > Tested-by: Alexandre Ghiti <alex@ghiti.fr>
> 
> Thanks Alexandre,
> Arnaldo, could you also pick this for fix?


Thanks, applied.

- Arnaldo

[tip: perf/urgent] perf probe: Do not depend on dwfl_module_addrsym()

[tip-bot2 for Masami Hiramatsu]

The following commit has been merged into the perf/urgent branch of tip:

Commit-ID:     1efde2754275dbd9d11c6e0132a4f09facf297ab
Gitweb:        https://git.kernel.org/tip/1efde2754275dbd9d11c6e0132a4f09facf297ab
Author:        Masami Hiramatsu <mhiramat@kernel.org>
AuthorDate:    Fri, 28 Feb 2020 00:42:01 +09:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Mon, 09 Mar 2020 10:43:53 -03:00

perf probe: Do not depend on dwfl_module_addrsym()

Do not depend on dwfl_module_addrsym() because it can fail on user-space
shared libraries.

Actually, same bug was fixed by commit 664fee3dc379 ("perf probe: Do not
use dwfl_module_addrsym if dwarf_diename finds symbol name"), but commit
07d369857808 ("perf probe: Fix wrong address verification) reverted to
get actual symbol address from symtab.

This fixes it again by getting symbol address from DIE, and only if the
DIE has only address range, it uses dwfl_module_addrsym().

Fixes: 07d369857808 ("perf probe: Fix wrong address verification)
Reported-by: Alexandre Ghiti <alex@ghiti.fr>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Tested-by: Alexandre Ghiti <alex@ghiti.fr>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Sasha Levin <sashal@kernel.org>
Link: http://lore.kernel.org/lkml/158281812176.476.14164573830975116234.stgit@devnote2
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/probe-finder.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index 1c817ad..e4cff49 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -637,14 +637,19 @@ static int convert_to_trace_point(Dwarf_Die *sp_die, Dwfl_Module *mod,
 		return -EINVAL;
 	}
 
-	/* Try to get actual symbol name from symtab */
-	symbol = dwfl_module_addrsym(mod, paddr, &sym, NULL);
+	if (dwarf_entrypc(sp_die, &eaddr) == 0) {
+		/* If the DIE has entrypc, use it. */
+		symbol = dwarf_diename(sp_die);
+	} else {
+		/* Try to get actual symbol name and address from symtab */
+		symbol = dwfl_module_addrsym(mod, paddr, &sym, NULL);
+		eaddr = sym.st_value;
+	}
 	if (!symbol) {
 		pr_warning("Failed to find symbol at 0x%lx\n",
 			   (unsigned long)paddr);
 		return -ENOENT;
 	}
-	eaddr = sym.st_value;
 
 	tp->offset = (unsigned long)(paddr - eaddr);
 	tp->address = (unsigned long)paddr;