[BUG] perf probe segfaulting when asked for variable it doesn't find

[BUG] perf probe segfaulting when asked for variable it doesn't find

[Arnaldo Carvalho de Melo]

Masami-san,

	While trying:

[root@zoo ~]# perf probe 'vfs_getname=getname_flags:65 pathname=result->name:string'
Failed to find the location of result at this address.
 Perhaps, it has been optimized out.
Failed to find 'result' in this function.
  Error: Failed to add events. (-2)
[root@zoo ~]# perf probe 'vfs_getname=getname_flags:65 pathname=result->name:string'
Added new event:
Segmentation fault (core dumped)

I got segfaulted while in the past I would get the much nicer:

[root@zoo ~]# perf probe 'vfs_getname=getname_flags:65 pathname=result->name:string'
Failed to find the location of result at this address.
 Perhaps, it has been optimized out.
Failed to find 'result' in this function.
  Error: Failed to add events. (-2)
[root@zoo ~]#

The "Error:"  part can and should be trimmed, just the two middle ones should
be enough, but I'm digressing, I bisected it down to the cset at the bottom of this
message.

In the past there was a 'result' variable at getname_flags, but now 'probe'
isn't finding any, as it seems to have been optimized away, still haven't checked
thoroughly:

[root@zoo ~]# perf probe -V getname_flags
Available variables at getname_flags
        @<getname_flags+0>
                char*   filename
                int     flags
                int*    empty
        @<getname+18>
                (No matched variables)
        @<user_path_create+37>
                (No matched variables)
        @<user_path_parent+37>
                (No matched variables)
        @<user_path_at_empty+50>
                (No matched variables)
        @<user_path_mountpoint_at+37>
                (No matched variables)
        @<sys_symlinkat+38>
                (No matched variables)
        @<sys_symlink+34>
                (No matched variables)
[root@zoo ~]

3d918a12a1b3088ac16ff37fa52760639d6e2403 is the first bad commit
commit 3d918a12a1b3088ac16ff37fa52760639d6e2403
Author: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Date:   Fri Oct 11 16:10:26 2013 +0900

    perf probe: Find fentry mcount fuzzed parameter location
    
    At this point, --fentry (mcount function entry) option for gcc fuzzes
    the debuginfo variable locations by skipping the mcount instruction
    offset (on x86, this is a 5 byte call instruction).
    
    This makes variable searching fail at the entry of functions which
    are mcount'ed.
    
    e.g.)
    Available variables at vfs_read
            @<vfs_read+0>
                    (No matched variables)
    
    This patch adds additional location search at the function entry point
    to solve this issue, which tries to find the earliest address for the
    variable location.
    
    Note that this only works with function parameters (formal parameters)
    because any local variables should not exist on the function entry
    address (those are not initialized yet).
    
    With this patch, perf probe shows correct parameters if possible;
     # perf probe --vars vfs_read
     Available variables at vfs_read
             @<vfs_read+0>
                     char*   buf
                     loff_t* pos
                     size_t  count
                     struct file*    file
    
    Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
    Cc: Ingo Molnar <mingo@redhat.com>
    Cc: Paul Mackerras <paulus@samba.org>
    Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
    Link: http://lkml.kernel.org/r/20131011071025.15557.13275.stgit@udc4-manage.rcp.hitachi.co.jp
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

:040000 040000 4c416906285aa1488ed2badbaf3b3feee86f9578 978c6b47f442845e6e93a79a24aeb36bbca0b7da M	tools
[acme@zoo linux]$

Re: [BUG] perf probe segfaulting when asked for variable it doesn't find

[Masami Hiramatsu]

(2014/05/29 6:44), Arnaldo Carvalho de Melo wrote:
> Masami-san,
> 
> 	While trying:
> 
> [root@zoo ~]# perf probe 'vfs_getname=getname_flags:65 pathname=result->name:string'
> Failed to find the location of result at this address.
>  Perhaps, it has been optimized out.
> Failed to find 'result' in this function.
>   Error: Failed to add events. (-2)
> [root@zoo ~]# perf probe 'vfs_getname=getname_flags:65 pathname=result->name:string'
> Added new event:
> Segmentation fault (core dumped)
> 
> I got segfaulted while in the past I would get the much nicer:
> 
> [root@zoo ~]# perf probe 'vfs_getname=getname_flags:65 pathname=result->name:string'
> Failed to find the location of result at this address.
>  Perhaps, it has been optimized out.
> Failed to find 'result' in this function.
>   Error: Failed to add events. (-2)
> [root@zoo ~]#

Oops, I got that. Bad SEGV was reproduced here too.
And I've found an error-handling miss in the convert_variable(),
I'll send a patch for fixing it.
Thank you very much for bisecting and finding my fault.

-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

[PATCH -tip/urgent ] [BUGFIX] perf/probe: Fix a segfault if asked for variable it doesn't find

[Masami Hiramatsu]

Fix a segfault bug by asking for variable it doesn't find.
Since the convert_variable() didn't handle error code returned
from convert_variable_location(), it just passed an incomplete
variable field and then a segfault was occured when formatting
the field.

This fixes that bug by handling success code correctly in
convert_variable(). Other callers of convert_variable_location()
are correctly checking the return code.

This bug was introduced by following commit. But another hidden
erroneous error handling has been there previuosly (-ENOMEM case).

 commit 3d918a12a1b3088ac16ff37fa52760639d6e2403

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/probe-finder.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index 5627621..9d8eb26 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -511,12 +511,12 @@ static int convert_variable(Dwarf_Die *vr_die, struct probe_finder *pf)
 
 	ret = convert_variable_location(vr_die, pf->addr, pf->fb_ops,
 					&pf->sp_die, pf->tvar);
-	if (ret == -ENOENT)
+	if (ret == -ENOENT || ret == -EINVAL)
 		pr_err("Failed to find the location of %s at this address.\n"
 		       " Perhaps, it has been optimized out.\n", pf->pvar->var);
 	else if (ret == -ENOTSUP)
 		pr_err("Sorry, we don't support this variable location yet.\n");
-	else if (pf->pvar->field) {
+	else if (ret == 0 && pf->pvar->field) {
 		ret = convert_variable_fields(vr_die, pf->pvar->var,
 					      pf->pvar->field, &pf->tvar->ref,
 					      &die_mem);

Re: [PATCH -tip/urgent ] [BUGFIX] perf/probe: Fix a segfault if asked for variable it doesn't find

[Masami Hiramatsu]

Arnaldo, this fixes the SEGV bug which you reported.

But I've found that perf probe sometimes loses the location of variables
with recent DWARF implementation. I need to check and fix that too.

Anyway, this patch should be applied for fixing critical bug.

Thank you,

(2014/05/29 19:52), Masami Hiramatsu wrote:
> Fix a segfault bug by asking for variable it doesn't find.
> Since the convert_variable() didn't handle error code returned
> from convert_variable_location(), it just passed an incomplete
> variable field and then a segfault was occured when formatting
> the field.
> 
> This fixes that bug by handling success code correctly in
> convert_variable(). Other callers of convert_variable_location()
> are correctly checking the return code.
> 
> This bug was introduced by following commit. But another hidden
> erroneous error handling has been there previuosly (-ENOMEM case).
> 
>  commit 3d918a12a1b3088ac16ff37fa52760639d6e2403
> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Jiri Olsa <jolsa@redhat.com>
> ---
>  tools/perf/util/probe-finder.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> index 5627621..9d8eb26 100644
> --- a/tools/perf/util/probe-finder.c
> +++ b/tools/perf/util/probe-finder.c
> @@ -511,12 +511,12 @@ static int convert_variable(Dwarf_Die *vr_die, struct probe_finder *pf)
>  
>  	ret = convert_variable_location(vr_die, pf->addr, pf->fb_ops,
>  					&pf->sp_die, pf->tvar);
> -	if (ret == -ENOENT)
> +	if (ret == -ENOENT || ret == -EINVAL)
>  		pr_err("Failed to find the location of %s at this address.\n"
>  		       " Perhaps, it has been optimized out.\n", pf->pvar->var);
>  	else if (ret == -ENOTSUP)
>  		pr_err("Sorry, we don't support this variable location yet.\n");
> -	else if (pf->pvar->field) {
> +	else if (ret == 0 && pf->pvar->field) {
>  		ret = convert_variable_fields(vr_die, pf->pvar->var,
>  					      pf->pvar->field, &pf->tvar->ref,
>  					      &die_mem);
> 
> 
> 
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

[PATCH -tip ] [BUGFIX] perf/probe: Fix perf probe to find correct variable DIE

[Masami Hiramatsu]

Fix perf probe to find correct variable DIE which has location or
external instance by tracking down the lexical blocks.

Current die_find_variable() expects that the all variable DIEs
which has DW_TAG_variable have a location. However, since recent
dwarf information may have declaration variable DIEs at the
entry of function (subprogram), die_find_variable() returns it.

To solve this problem, it must track down the DIE tree to find
a DIE which has an actual location or a reference for external
instance.

e.g. finding a DIE which origin is <0xdc73>;

 <1><11496>: Abbrev Number: 95 (DW_TAG_subprogram)
    <11497>   DW_AT_abstract_origin: <0xdc42>
    <1149b>   DW_AT_low_pc      : 0x1850
[...]
 <2><114cc>: Abbrev Number: 119 (DW_TAG_variable) <- this is a declaration
    <114cd>   DW_AT_abstract_origin: <0xdc73>
 <2><114d1>: Abbrev Number: 119 (DW_TAG_variable)
[...]
 <3><115a7>: Abbrev Number: 105 (DW_TAG_lexical_block)
    <115a8>   DW_AT_ranges      : 0xaa0
 <4><115ac>: Abbrev Number: 96 (DW_TAG_variable) <- this has a location
    <115ad>   DW_AT_abstract_origin: <0xdc73>
    <115b1>   DW_AT_location    : 0x486c        (location list)

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/dwarf-aux.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index 7defd77..cc66c40 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -747,14 +747,17 @@ struct __find_variable_param {
 static int __die_find_variable_cb(Dwarf_Die *die_mem, void *data)
 {
 	struct __find_variable_param *fvp = data;
+	Dwarf_Attribute attr;
 	int tag;
 
 	tag = dwarf_tag(die_mem);
 	if ((tag == DW_TAG_formal_parameter ||
 	     tag == DW_TAG_variable) &&
-	    die_compare_name(die_mem, fvp->name))
+	    die_compare_name(die_mem, fvp->name) &&
+	/* Does the DIE have location information or external instance? */
+	    (dwarf_attr(die_mem, DW_AT_external, &attr) ||
+	     dwarf_attr(die_mem, DW_AT_location, &attr)))
 		return DIE_FIND_CB_END;
-
 	if (dwarf_haspc(die_mem, fvp->addr))
 		return DIE_FIND_CB_CONTINUE;
 	else

Re: [PATCH -tip ] [BUGFIX] perf/probe: Fix perf probe to find correct variable DIE

[Masami Hiramatsu]

Hi Arnaldo,

Here is the patch which fixes perf probe to find variable
location correctly, on the recent dwarf format. This is not
related to the SEGV issue which I fixed in previous mail.

Thank you,

(2014/05/29 21:19), Masami Hiramatsu wrote:
> Fix perf probe to find correct variable DIE which has location or
> external instance by tracking down the lexical blocks.
> 
> Current die_find_variable() expects that the all variable DIEs
> which has DW_TAG_variable have a location. However, since recent
> dwarf information may have declaration variable DIEs at the
> entry of function (subprogram), die_find_variable() returns it.
> 
> To solve this problem, it must track down the DIE tree to find
> a DIE which has an actual location or a reference for external
> instance.
> 
> e.g. finding a DIE which origin is <0xdc73>;
> 
>  <1><11496>: Abbrev Number: 95 (DW_TAG_subprogram)
>     <11497>   DW_AT_abstract_origin: <0xdc42>
>     <1149b>   DW_AT_low_pc      : 0x1850
> [...]
>  <2><114cc>: Abbrev Number: 119 (DW_TAG_variable) <- this is a declaration
>     <114cd>   DW_AT_abstract_origin: <0xdc73>
>  <2><114d1>: Abbrev Number: 119 (DW_TAG_variable)
> [...]
>  <3><115a7>: Abbrev Number: 105 (DW_TAG_lexical_block)
>     <115a8>   DW_AT_ranges      : 0xaa0
>  <4><115ac>: Abbrev Number: 96 (DW_TAG_variable) <- this has a location
>     <115ad>   DW_AT_abstract_origin: <0xdc73>
>     <115b1>   DW_AT_location    : 0x486c        (location list)
> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Jiri Olsa <jolsa@redhat.com>
> ---
>  tools/perf/util/dwarf-aux.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index 7defd77..cc66c40 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
> @@ -747,14 +747,17 @@ struct __find_variable_param {
>  static int __die_find_variable_cb(Dwarf_Die *die_mem, void *data)
>  {
>  	struct __find_variable_param *fvp = data;
> +	Dwarf_Attribute attr;
>  	int tag;
>  
>  	tag = dwarf_tag(die_mem);
>  	if ((tag == DW_TAG_formal_parameter ||
>  	     tag == DW_TAG_variable) &&
> -	    die_compare_name(die_mem, fvp->name))
> +	    die_compare_name(die_mem, fvp->name) &&
> +	/* Does the DIE have location information or external instance? */
> +	    (dwarf_attr(die_mem, DW_AT_external, &attr) ||
> +	     dwarf_attr(die_mem, DW_AT_location, &attr)))
>  		return DIE_FIND_CB_END;
> -
>  	if (dwarf_haspc(die_mem, fvp->addr))
>  		return DIE_FIND_CB_CONTINUE;
>  	else
> 
> 
> 
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH -tip ] [BUGFIX] perf/probe: Fix perf probe to find correct variable DIE

[Jiri Olsa]

On Thu, May 29, 2014 at 09:39:14PM +0900, Masami Hiramatsu wrote:
> Hi Arnaldo,
> 
> Here is the patch which fixes perf probe to find variable
> location correctly, on the recent dwarf format. This is not
> related to the SEGV issue which I fixed in previous mail.
> 
> Thank you,

hi,
I took both patches to my perf/core.. will check
if Ingo wants the segfault fix via perf/urgent

thanks,
jirka

Re: [PATCH -tip ] [BUGFIX] perf/probe: Fix perf probe to find correct variable DIE

[Arnaldo Carvalho de Melo]

Em Thu, May 29, 2014 at 09:39:14PM +0900, Masami Hiramatsu escreveu:
> Hi Arnaldo,
> 
> Here is the patch which fixes perf probe to find variable
> location correctly, on the recent dwarf format. This is not
> related to the SEGV issue which I fixed in previous mail.

Thanks for the prompt reply! I'm testing them and will report results
here!

- Arnaldo

Re: [PATCH -tip/urgent ] [BUGFIX] perf/probe: Fix a segfault if asked for variable it doesn't find

[Namhyung Kim]

Hi Masami,

On Thu, 29 May 2014 19:52:32 +0900, Masami Hiramatsu wrote:
> Fix a segfault bug by asking for variable it doesn't find.
> Since the convert_variable() didn't handle error code returned
> from convert_variable_location(), it just passed an incomplete
> variable field and then a segfault was occured when formatting
> the field.
>
> This fixes that bug by handling success code correctly in
> convert_variable(). Other callers of convert_variable_location()
> are correctly checking the return code.
>
> This bug was introduced by following commit. But another hidden
> erroneous error handling has been there previuosly (-ENOMEM case).

And -ERANGE too.. :)

Thanks,
Namhyung

>
>  commit 3d918a12a1b3088ac16ff37fa52760639d6e2403
>
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Jiri Olsa <jolsa@redhat.com>
> ---
>  tools/perf/util/probe-finder.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> index 5627621..9d8eb26 100644
> --- a/tools/perf/util/probe-finder.c
> +++ b/tools/perf/util/probe-finder.c
> @@ -511,12 +511,12 @@ static int convert_variable(Dwarf_Die *vr_die, struct probe_finder *pf)
>  
>  	ret = convert_variable_location(vr_die, pf->addr, pf->fb_ops,
>  					&pf->sp_die, pf->tvar);
> -	if (ret == -ENOENT)
> +	if (ret == -ENOENT || ret == -EINVAL)
>  		pr_err("Failed to find the location of %s at this address.\n"
>  		       " Perhaps, it has been optimized out.\n", pf->pvar->var);
>  	else if (ret == -ENOTSUP)
>  		pr_err("Sorry, we don't support this variable location yet.\n");
> -	else if (pf->pvar->field) {
> +	else if (ret == 0 && pf->pvar->field) {
>  		ret = convert_variable_fields(vr_die, pf->pvar->var,
>  					      pf->pvar->field, &pf->tvar->ref,
>  					      &die_mem);

Re: [PATCH -tip/urgent ] [BUGFIX] perf/probe: Fix a segfault if asked for variable it doesn't find

[Masami Hiramatsu]

(2014/05/30 15:03), Namhyung Kim wrote:
> Hi Masami,
> 
> On Thu, 29 May 2014 19:52:32 +0900, Masami Hiramatsu wrote:
>> Fix a segfault bug by asking for variable it doesn't find.
>> Since the convert_variable() didn't handle error code returned
>> from convert_variable_location(), it just passed an incomplete
>> variable field and then a segfault was occured when formatting
>> the field.
>>
>> This fixes that bug by handling success code correctly in
>> convert_variable(). Other callers of convert_variable_location()
>> are correctly checking the return code.
>>
>> This bug was introduced by following commit. But another hidden
>> erroneous error handling has been there previuosly (-ENOMEM case).
> 
> And -ERANGE too.. :)

Right, Anyway, I've added ret==0 sanity check. That error should be
handled too. :-)

Thank you,

> 
> Thanks,
> Namhyung
> 
>>
>>  commit 3d918a12a1b3088ac16ff37fa52760639d6e2403
>>
>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
>> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
>> Cc: Paul Mackerras <paulus@samba.org>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Jiri Olsa <jolsa@redhat.com>
>> ---
>>  tools/perf/util/probe-finder.c |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
>> index 5627621..9d8eb26 100644
>> --- a/tools/perf/util/probe-finder.c
>> +++ b/tools/perf/util/probe-finder.c
>> @@ -511,12 +511,12 @@ static int convert_variable(Dwarf_Die *vr_die, struct probe_finder *pf)
>>  
>>  	ret = convert_variable_location(vr_die, pf->addr, pf->fb_ops,
>>  					&pf->sp_die, pf->tvar);
>> -	if (ret == -ENOENT)
>> +	if (ret == -ENOENT || ret == -EINVAL)
>>  		pr_err("Failed to find the location of %s at this address.\n"
>>  		       " Perhaps, it has been optimized out.\n", pf->pvar->var);
>>  	else if (ret == -ENOTSUP)
>>  		pr_err("Sorry, we don't support this variable location yet.\n");
>> -	else if (pf->pvar->field) {
>> +	else if (ret == 0 && pf->pvar->field) {
>>  		ret = convert_variable_fields(vr_die, pf->pvar->var,
>>  					      pf->pvar->field, &pf->tvar->ref,
>>  					      &die_mem);
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH -tip ] [BUGFIX] perf/probe: Fix perf probe to find correct variable DIE

[Ingo Molnar]

* Jiri Olsa <jolsa@redhat.com> wrote:

> On Thu, May 29, 2014 at 09:39:14PM +0900, Masami Hiramatsu wrote:
> > Hi Arnaldo,
> > 
> > Here is the patch which fixes perf probe to find variable
> > location correctly, on the recent dwarf format. This is not
> > related to the SEGV issue which I fixed in previous mail.
> > 
> > Thank you,
> 
> hi,
> I took both patches to my perf/core.. will check
> if Ingo wants the segfault fix via perf/urgent

Yeah, segfaults (or any other random/uncontrolled user space behavior) 
we generally want to fix via perf/urgent. (Please also put a Cc: 
stable@vger.kernel.org tag into it, in case it misses v3.15.)

Thanks,

	Ingo

Re: [PATCH -tip ] [BUGFIX] perf/probe: Fix perf probe to find correct variable DIE

[Arnaldo Carvalho de Melo]

Em Thu, May 29, 2014 at 09:19:30PM +0900, Masami Hiramatsu escreveu:
> Fix perf probe to find correct variable DIE which has location or
> external instance by tracking down the lexical blocks.
> 
> Current die_find_variable() expects that the all variable DIEs
> which has DW_TAG_variable have a location. However, since recent
> dwarf information may have declaration variable DIEs at the
> entry of function (subprogram), die_find_variable() returns it.
> 
> To solve this problem, it must track down the DIE tree to find
> a DIE which has an actual location or a reference for external
> instance.

Acked-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Before:

[root@zoo ~]# perf probe --del probe:vfs_getname
Removed event: probe:vfs_getname
[root@zoo ~]# perf probe 'vfs_getname=getname_flags:65 pathname=result->name:string'
Added new event:
Segmentation fault (core dumped)

After:

[root@zoo ~]# perf probe 'vfs_getname=getname_flags:65 pathname=result->name:string'
Added new event:
  probe:vfs_getname    (on getname_flags:65 with pathname=result->name:string)

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

	perf record -e probe:vfs_getname -aR sleep 1

[root@zoo ~]# perf record -e probe:vfs_getname -a sleep 1
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.550 MB perf.data (~24022 samples) ]
[root@zoo ~]# perf script | head -5
            perf 27758 [001] 12022.551584: probe:vfs_getname: (ffffffff811c2e43) pathname="/home/acme/libexec/perf-core/sleep"
            perf 27758 [001] 12022.551616: probe:vfs_getname: (ffffffff811c2e43) pathname="/usr/lib64/ccache/sleep"
            perf 27758 [001] 12022.551628: probe:vfs_getname: (ffffffff811c2e43) pathname="/usr/local/sbin/sleep"
            perf 27758 [001] 12022.551636: probe:vfs_getname: (ffffffff811c2e43) pathname="/usr/local/bin/sleep"
            perf 27758 [001] 12022.551644: probe:vfs_getname: (ffffffff811c2e43) pathname="/sbin/sleep"
[root@zoo ~]#

- Arnaldo

Re: [PATCH -tip ] [BUGFIX] perf/probe: Fix perf probe to find correct variable DIE

[Arnaldo Carvalho de Melo]

Em Thu, May 29, 2014 at 09:39:14PM +0900, Masami Hiramatsu escreveu:
> Hi Arnaldo,
> 
> Here is the patch which fixes perf probe to find variable
> location correctly, on the recent dwarf format. This is not
> related to the SEGV issue which I fixed in previous mail.

Hey, if I apply just this one I got my problem solved, i.e. no more
segfaults and also I managed to add that problem, the variable is there,
all works, why do I need the first one?

Without your two patches:

[root@zoo ~]# perf probe 'vfs_getname=getname_flags:65 pathname=result->name:string'
Added new event:
Segmentation fault (core dumped)
[root@zoo ~]#

With this ([BUGFIX] perf/probe: Fix perf probe to find correct variable DIE):

Trying to reference a bogus variable, one that is not available at this
function:

Removed event: probe:vfs_getname
[root@zoo ~]# perf probe 'vfs_getname=getname_flags:65 pathname=BOGUS->name:string'
Failed to find 'BOGUS' in this function.
  Error: Failed to add events. (-2)
[root@zoo ~]#

Perfect!

Now trying to reference a bogus member name:

[root@zoo ~]# perf probe 'vfs_getname=getname_flags:65 pathname=result->BOGUS:string'
result(type:filename) has no member BOGUS.
Failed to find 'result' in this function.
  Error: Failed to add events. (-22)
[root@zoo ~]#

No segfault, albeit it produces a bogus error message, as it clearly
_finds_ the 'result' variable, its just that it is of a struct type that
_has_ no such 'BOGUS' _member_.

Also I suggest removing that last "Error:" line, what is its value for
users or developers?

Now if I do it all correctly:

[root@zoo ~]# perf probe 'vfs_getname=getname_flags:65 pathname=result->name:string'
Added new event:
  probe:vfs_getname    (on getname_flags:65 with pathname=result->name:string)

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

	perf record -e probe:vfs_getname -aR sleep 1

[root@zoo ~]#

Ok, it works again!

And if I try to use it:

[root@zoo ~]# perf record -e probe:vfs_getname -a sleep 1
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.562 MB perf.data (~24570 samples) ]
[root@zoo ~]# perf script | head -5
            perf   716 [000] 17842.271777: probe:vfs_getname: (ffffffff811c2e43) pathname="/home/acme/libexec/perf-core/sleep"
            perf   716 [000] 17842.271794: probe:vfs_getname: (ffffffff811c2e43) pathname="/usr/lib64/ccache/sleep"
            perf   716 [000] 17842.271800: probe:vfs_getname: (ffffffff811c2e43) pathname="/usr/local/sbin/sleep"
            perf   716 [000] 17842.271804: probe:vfs_getname: (ffffffff811c2e43) pathname="/usr/local/bin/sleep"
            perf   716 [000] 17842.271808: probe:vfs_getname: (ffffffff811c2e43) pathname="/sbin/sleep"
[root@zoo ~]#

Works as expected.

So no segfault anywhere, while if I apply just that one that fixes the segfault I get:

[root@zoo ~]# perf probe 'vfs_getname=getname_flags:65 pathname=result->name:string'
Failed to find the location of result at this address.
 Perhaps, it has been optimized out.
Failed to find 'result' in this function.
  Error: Failed to add events. (-22)
[root@zoo ~]#

It doesn't work, which is a regression, as applying just the first one allows
us to retrieve that variable.

[root@zoo ~]# perf probe -V getname_flags:65
Available variables at getname_flags:65
        @<getname_flags+227>
                char*   filename
                int     len
                int*    empty
                long int        max
                struct filename*        result
[root@zoo ~]#

I.e. applying just the one that fixes just the segfault:

  perf probe: Fix a segfault if asked for variable it doesn't find

"Fixes" the segfault but introduces a regression.

Can we apply just this one:

 perf probe: Fix to find correct variable DIE

Because even when we specify a "variable that it doesn't find", it works well.

Regards,

- Arnaldo
 
> Thank you,
> 
> (2014/05/29 21:19), Masami Hiramatsu wrote:
> > Fix perf probe to find correct variable DIE which has location or
> > external instance by tracking down the lexical blocks.
> > 
> > Current die_find_variable() expects that the all variable DIEs
> > which has DW_TAG_variable have a location. However, since recent
> > dwarf information may have declaration variable DIEs at the
> > entry of function (subprogram), die_find_variable() returns it.
> > 
> > To solve this problem, it must track down the DIE tree to find
> > a DIE which has an actual location or a reference for external
> > instance.
> > 
> > e.g. finding a DIE which origin is <0xdc73>;
> > 
> >  <1><11496>: Abbrev Number: 95 (DW_TAG_subprogram)
> >     <11497>   DW_AT_abstract_origin: <0xdc42>
> >     <1149b>   DW_AT_low_pc      : 0x1850
> > [...]
> >  <2><114cc>: Abbrev Number: 119 (DW_TAG_variable) <- this is a declaration
> >     <114cd>   DW_AT_abstract_origin: <0xdc73>
> >  <2><114d1>: Abbrev Number: 119 (DW_TAG_variable)
> > [...]
> >  <3><115a7>: Abbrev Number: 105 (DW_TAG_lexical_block)
> >     <115a8>   DW_AT_ranges      : 0xaa0
> >  <4><115ac>: Abbrev Number: 96 (DW_TAG_variable) <- this has a location
> >     <115ad>   DW_AT_abstract_origin: <0xdc73>
> >     <115b1>   DW_AT_location    : 0x486c        (location list)
> > 
> > Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> > Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Cc: Paul Mackerras <paulus@samba.org>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Cc: Jiri Olsa <jolsa@redhat.com>
> > ---
> >  tools/perf/util/dwarf-aux.c |    7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> > index 7defd77..cc66c40 100644
> > --- a/tools/perf/util/dwarf-aux.c
> > +++ b/tools/perf/util/dwarf-aux.c
> > @@ -747,14 +747,17 @@ struct __find_variable_param {
> >  static int __die_find_variable_cb(Dwarf_Die *die_mem, void *data)
> >  {
> >  	struct __find_variable_param *fvp = data;
> > +	Dwarf_Attribute attr;
> >  	int tag;
> >  
> >  	tag = dwarf_tag(die_mem);
> >  	if ((tag == DW_TAG_formal_parameter ||
> >  	     tag == DW_TAG_variable) &&
> > -	    die_compare_name(die_mem, fvp->name))
> > +	    die_compare_name(die_mem, fvp->name) &&
> > +	/* Does the DIE have location information or external instance? */
> > +	    (dwarf_attr(die_mem, DW_AT_external, &attr) ||
> > +	     dwarf_attr(die_mem, DW_AT_location, &attr)))
> >  		return DIE_FIND_CB_END;
> > -
> >  	if (dwarf_haspc(die_mem, fvp->addr))
> >  		return DIE_FIND_CB_CONTINUE;
> >  	else
> > 
> > 
> > 
> > 
> 
> 
> -- 
> Masami HIRAMATSU
> Software Platform Research Dept. Linux Technology Research Center
> Hitachi, Ltd., Yokohama Research Laboratory
> E-mail: masami.hiramatsu.pt@hitachi.com
> 

Re: [PATCH -tip ] [BUGFIX] perf/probe: Fix perf probe to find correct variable DIE

[Masami Hiramatsu]

(2014/06/04 5:38), Arnaldo Carvalho de Melo wrote:
> Now trying to reference a bogus member name:
> 
> [root@zoo ~]# perf probe 'vfs_getname=getname_flags:65 pathname=result->BOGUS:string'
> result(type:filename) has no member BOGUS.
> Failed to find 'result' in this function.
>   Error: Failed to add events. (-22)
> [root@zoo ~]#
> 
> No segfault, albeit it produces a bogus error message, as it clearly
> _finds_ the 'result' variable, its just that it is of a struct type that
> _has_ no such 'BOGUS' _member_.

Right, I'll fix that.

> 
> Also I suggest removing that last "Error:" line, what is its value for
> users or developers?

It tells them the perf probe finally failed because of an error (with
error code). Indeed that message is bluntness. I'll update it to use
strerr instead of giving mysterious error code.
FYI, if it gets some general errors, like -ENOMEM, will just indicate
the last error message.

Thank you!

-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH -tip ] [BUGFIX] perf/probe: Fix perf probe to find correct variable DIE

[Jiri Olsa]

On Wed, Jun 04, 2014 at 05:25:02PM +0900, Masami Hiramatsu wrote:
> (2014/06/04 5:38), Arnaldo Carvalho de Melo wrote:
> > Now trying to reference a bogus member name:
> > 
> > [root@zoo ~]# perf probe 'vfs_getname=getname_flags:65 pathname=result->BOGUS:string'
> > result(type:filename) has no member BOGUS.
> > Failed to find 'result' in this function.
> >   Error: Failed to add events. (-22)
> > [root@zoo ~]#
> > 
> > No segfault, albeit it produces a bogus error message, as it clearly
> > _finds_ the 'result' variable, its just that it is of a struct type that
> > _has_ no such 'BOGUS' _member_.
> 
> Right, I'll fix that.

should I wait for new patchset, or should I take those 2?

thanks,
jirka

Re: [PATCH -tip ] [BUGFIX] perf/probe: Fix perf probe to find correct variable DIE

[Masami Hiramatsu]

(2014/06/04 20:31), Jiri Olsa wrote:
> On Wed, Jun 04, 2014 at 05:25:02PM +0900, Masami Hiramatsu wrote:
>> (2014/06/04 5:38), Arnaldo Carvalho de Melo wrote:
>>> Now trying to reference a bogus member name:
>>>
>>> [root@zoo ~]# perf probe 'vfs_getname=getname_flags:65 pathname=result->BOGUS:string'
>>> result(type:filename) has no member BOGUS.
>>> Failed to find 'result' in this function.
>>>   Error: Failed to add events. (-22)
>>> [root@zoo ~]#
>>>
>>> No segfault, albeit it produces a bogus error message, as it clearly
>>> _finds_ the 'result' variable, its just that it is of a struct type that
>>> _has_ no such 'BOGUS' _member_.
>>
>> Right, I'll fix that.
> 
> should I wait for new patchset, or should I take those 2?

No, those 2 patches are urgently required, because one fixes SEGV critical
error, and the other fixes a wrong behavior (catch up to the latest
dwarf), which is also degradation.
I think changing error messages is a kind of usability enhancement.

Thank you,

-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH -tip ] [BUGFIX] perf/probe: Fix perf probe to find correct variable DIE

[Jiri Olsa]

On Wed, Jun 04, 2014 at 09:19:57PM +0900, Masami Hiramatsu wrote:
> (2014/06/04 20:31), Jiri Olsa wrote:
> > On Wed, Jun 04, 2014 at 05:25:02PM +0900, Masami Hiramatsu wrote:
> >> (2014/06/04 5:38), Arnaldo Carvalho de Melo wrote:
> >>> Now trying to reference a bogus member name:
> >>>
> >>> [root@zoo ~]# perf probe 'vfs_getname=getname_flags:65 pathname=result->BOGUS:string'
> >>> result(type:filename) has no member BOGUS.
> >>> Failed to find 'result' in this function.
> >>>   Error: Failed to add events. (-22)
> >>> [root@zoo ~]#
> >>>
> >>> No segfault, albeit it produces a bogus error message, as it clearly
> >>> _finds_ the 'result' variable, its just that it is of a struct type that
> >>> _has_ no such 'BOGUS' _member_.
> >>
> >> Right, I'll fix that.
> > 
> > should I wait for new patchset, or should I take those 2?
> 
> No, those 2 patches are urgently required, because one fixes SEGV critical
> error, and the other fixes a wrong behavior (catch up to the latest
> dwarf), which is also degradation.

ok, I'll queue both of them to perf/urgent

thanks,
jirka

[tip:perf/urgent] perf probe: Fix a segfault if asked for variable it doesn't find

[tip-bot for Masami Hiramatsu]

Commit-ID:  0c188a07b6a399e3df66534c29fef0a2082aaf57
Gitweb:     http://git.kernel.org/tip/0c188a07b6a399e3df66534c29fef0a2082aaf57
Author:     Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
AuthorDate: Thu, 29 May 2014 19:52:32 +0900
Committer:  Jiri Olsa <jolsa@kernel.org>
CommitDate: Wed, 4 Jun 2014 14:48:03 +0200

perf probe: Fix a segfault if asked for variable it doesn't find

Fix a segfault bug by asking for variable it doesn't find.
Since the convert_variable() didn't handle error code returned
from convert_variable_location(), it just passed an incomplete
variable field and then a segfault was occurred when formatting
the field.

This fixes that bug by handling success code correctly in
convert_variable(). Other callers of convert_variable_location()
are correctly checking the return code.

This bug was introduced by following commit. But another hidden
erroneous error handling has been there previously (-ENOMEM case).

 commit 3d918a12a1b3088ac16ff37fa52760639d6e2403

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Link: http://lkml.kernel.org/r/20140529105232.28251.30447.stgit@ltc230.yrl.intra.hitachi.co.jp
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/probe-finder.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index 5627621..9d8eb26 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -511,12 +511,12 @@ static int convert_variable(Dwarf_Die *vr_die, struct probe_finder *pf)
 
 	ret = convert_variable_location(vr_die, pf->addr, pf->fb_ops,
 					&pf->sp_die, pf->tvar);
-	if (ret == -ENOENT)
+	if (ret == -ENOENT || ret == -EINVAL)
 		pr_err("Failed to find the location of %s at this address.\n"
 		       " Perhaps, it has been optimized out.\n", pf->pvar->var);
 	else if (ret == -ENOTSUP)
 		pr_err("Sorry, we don't support this variable location yet.\n");
-	else if (pf->pvar->field) {
+	else if (ret == 0 && pf->pvar->field) {
 		ret = convert_variable_fields(vr_die, pf->pvar->var,
 					      pf->pvar->field, &pf->tvar->ref,
 					      &die_mem);

[tip:perf/urgent] perf probe: Fix perf probe to find correct variable DIE

[tip-bot for Masami Hiramatsu]

Commit-ID:  082f96a93eb5ba9bf771518a0dda590624568e8e
Gitweb:     http://git.kernel.org/tip/082f96a93eb5ba9bf771518a0dda590624568e8e
Author:     Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
AuthorDate: Thu, 29 May 2014 21:19:30 +0900
Committer:  Jiri Olsa <jolsa@kernel.org>
CommitDate: Wed, 4 Jun 2014 14:49:20 +0200

perf probe: Fix perf probe to find correct variable DIE

Fix perf probe to find correct variable DIE which has location or
external instance by tracking down the lexical blocks.

Current die_find_variable() expects that the all variable DIEs
which has DW_TAG_variable have a location. However, since recent
dwarf information may have declaration variable DIEs at the
entry of function (subprogram), die_find_variable() returns it.

To solve this problem, it must track down the DIE tree to find
a DIE which has an actual location or a reference for external
instance.

e.g. finding a DIE which origin is <0xdc73>;

 <1><11496>: Abbrev Number: 95 (DW_TAG_subprogram)
    <11497>   DW_AT_abstract_origin: <0xdc42>
    <1149b>   DW_AT_low_pc      : 0x1850
[...]
 <2><114cc>: Abbrev Number: 119 (DW_TAG_variable) <- this is a declaration
    <114cd>   DW_AT_abstract_origin: <0xdc73>
 <2><114d1>: Abbrev Number: 119 (DW_TAG_variable)
[...]
 <3><115a7>: Abbrev Number: 105 (DW_TAG_lexical_block)
    <115a8>   DW_AT_ranges      : 0xaa0
 <4><115ac>: Abbrev Number: 96 (DW_TAG_variable) <- this has a location
    <115ad>   DW_AT_abstract_origin: <0xdc73>
    <115b1>   DW_AT_location    : 0x486c        (location list)

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Tested-by: Arnaldo Carvalho de Melo <acme@kernel.org>
Acked-by: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Link: http://lkml.kernel.org/r/20140529121930.30879.87092.stgit@ltc230.yrl.intra.hitachi.co.jp
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/dwarf-aux.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index 7defd77..cc66c40 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -747,14 +747,17 @@ struct __find_variable_param {
 static int __die_find_variable_cb(Dwarf_Die *die_mem, void *data)
 {
 	struct __find_variable_param *fvp = data;
+	Dwarf_Attribute attr;
 	int tag;
 
 	tag = dwarf_tag(die_mem);
 	if ((tag == DW_TAG_formal_parameter ||
 	     tag == DW_TAG_variable) &&
-	    die_compare_name(die_mem, fvp->name))
+	    die_compare_name(die_mem, fvp->name) &&
+	/* Does the DIE have location information or external instance? */
+	    (dwarf_attr(die_mem, DW_AT_external, &attr) ||
+	     dwarf_attr(die_mem, DW_AT_location, &attr)))
 		return DIE_FIND_CB_END;
-
 	if (dwarf_haspc(die_mem, fvp->addr))
 		return DIE_FIND_CB_CONTINUE;
 	else