The issue with `perf report -s comm`

The issue with `perf report -s comm`

[Alexander Pozdneev]

Hello,

I have the following issue with perf `report -s comm` that seems to be
a bug. This is how the code looks like:

int main() {
    double a, b, c;
    a = do_things_main(WAIT_TIME * 2); // 40% of time, a.out
    b = do_things1(WAIT_TIME);         // 20% of time, libdo1.so
    c = do_things2(WAIT_TIME * 2);     // 20% of time, libdo2.so
    return (int)(a + b + c);
}

This is how I run it:

$ LD_LIBRARY_PATH="." perf record -g ./a.out
[ perf record: Woken up 3 times to write data ]
[ perf record: Captured and wrote 0.743 MB perf.data (11911 samples) ]

This is the regular output of `perf report`:

$ perf report | grep -v '#' | head -n 12
    39.99%    39.99%  a.out    a.out              [.] main
            |
            ---main

    39.94%    39.94%  a.out    libdo2.so          [.] do_things2
            |
            ---do_things2

    20.05%    20.05%  a.out    libdo1.so          [.] do_things1
            |
            ---do_things1

It looks exactly as expected. If I sort by `dso`, the output is also OK:

$ perf report -s dso | grep -v '#'
    39.99%    39.99%  a.out
            |
            ---main

    39.94%    39.94%  libdo2.so
            |
            ---do_things2

    20.05%    20.05%  libdo1.so
            |
            ---do_things1

However, If I try to sort by `comm`, the output looks strange:

$ perf report -s comm | grep -v '#'
   100.00%   100.00%  a.out
            |
            |--59.99%--do_things1
            |
             --39.99%--main

Specifically, `do_things2()` is missing in the report.

Here is the full source code of this example:
https://github.com/pozdneev/perf-report-s-comm-bug
Originally, Louis Stuber (IBM Client Center Montpellier) discovered
this behaviour when we have been playing with Flame Graphs
(http://www.brendangregg.com/blog/2016-04-30/linux-perf-folded.html).

This is the list of Linux/perf versions that I've tried:

* Ubuntu 14.04.5 (3.19.0-80-generic), perf version 3.19.8-ckt22
* RHEL 7.3 ppc64le (3.10.0-514.el7.ppc64le), perf version
3.10.0-514.el7.ppc64le.debug
* My colleague confirms that he observes the same behavior with kernel
version 4.13.

Could you please check if the issue remains in the current development
version of Linux perf?

Thanks!

Alexander

[RFC] perf callchain: Compare dsos (as well) for CCKEY_FUNCTION

[Ravi Bangoria]

Two functions from different binaries can have same start
address. Thus, comparing only start address in match_chain()
leads to inconsistent callchains. Fix this by adding a check
for dsos as well.

Ex, https://www.spinics.net/lists/linux-perf-users/msg04067.html

Reported-by: Alexander Pozdneev <pozdneyev@gmail.com>
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
 tools/perf/util/callchain.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index 510b513..6d5a483 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -678,6 +678,9 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
 {
 	struct symbol *sym = node->sym;
 	u64 left, right;
+	struct dso *left_dso = NULL;
+	struct dso *right_dso = NULL;
+
 
 	if (callchain_param.key == CCKEY_SRCLINE) {
 		enum match_result match = match_chain_srcline(node, cnode);
@@ -689,12 +692,16 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
 	if (cnode->ms.sym && sym && callchain_param.key == CCKEY_FUNCTION) {
 		left = cnode->ms.sym->start;
 		right = sym->start;
+		if (cnode->ms.map && node->map) {
+			left_dso = cnode->ms.map->dso;
+			right_dso = node->map->dso;
+		}
 	} else {
 		left = cnode->ip;
 		right = node->ip;
 	}
 
-	if (left == right) {
+	if (left == right && left_dso == right_dso) {
 		if (node->branch) {
 			cnode->branch_count++;
 
-- 
1.8.3.1

Re: [RFC] perf callchain: Compare dsos (as well) for CCKEY_FUNCTION

[Jiri Olsa]

On Wed, Oct 04, 2017 at 11:43:08AM +0530, Ravi Bangoria wrote:
> Two functions from different binaries can have same start
> address. Thus, comparing only start address in match_chain()
> leads to inconsistent callchains. Fix this by adding a check
> for dsos as well.
> 
> Ex, https://www.spinics.net/lists/linux-perf-users/msg04067.html
> 
> Reported-by: Alexander Pozdneev <pozdneyev@gmail.com>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> ---
>  tools/perf/util/callchain.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> index 510b513..6d5a483 100644
> --- a/tools/perf/util/callchain.c
> +++ b/tools/perf/util/callchain.c
> @@ -678,6 +678,9 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
>  {
>  	struct symbol *sym = node->sym;
>  	u64 left, right;
> +	struct dso *left_dso = NULL;
> +	struct dso *right_dso = NULL;
> +
>  
>  	if (callchain_param.key == CCKEY_SRCLINE) {
>  		enum match_result match = match_chain_srcline(node, cnode);
> @@ -689,12 +692,16 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
>  	if (cnode->ms.sym && sym && callchain_param.key == CCKEY_FUNCTION) {
>  		left = cnode->ms.sym->start;
>  		right = sym->start;
> +		if (cnode->ms.map && node->map) {
> +			left_dso = cnode->ms.map->dso;
> +			right_dso = node->map->dso;

makes sense.. but why not to get those maps separately?

	if (cnode->ms.map)
		left_dso = cnode->ms.map->dso;
	if (node->map) {
		right_dso = node->map->dso;

I'd think that if one is missing, it's most likely different
map/dso and you want to fail the == check

jirka

> +		}
>  	} else {
>  		left = cnode->ip;
>  		right = node->ip;
>  	}
>  
> -	if (left == right) {
> +	if (left == right && left_dso == right_dso) {
>  		if (node->branch) {
>  			cnode->branch_count++;
>  
> -- 
> 1.8.3.1
> 

[PATCH] perf callchain: Compare dsos (as well) for CCKEY_FUNCTION

[Ravi Bangoria]

Two functions from different binaries can have same start
address. Thus, comparing only start address in match_chain()
leads to inconsistent callchains. Fix this by adding a check
for dsos as well.

Ex, https://www.spinics.net/lists/linux-perf-users/msg04067.html

Reported-by: Alexander Pozdneev <pozdneyev@gmail.com>
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
 tools/perf/util/callchain.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index be09d77..6d7f645 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -685,6 +685,9 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
 {
 	struct symbol *sym = node->sym;
 	u64 left, right;
+	struct dso *left_dso = NULL;
+	struct dso *right_dso = NULL;
+
 
 	if (callchain_param.key == CCKEY_SRCLINE) {
 		enum match_result match = match_chain_srcline(node, cnode);
@@ -696,12 +699,16 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
 	if (cnode->ms.sym && sym && callchain_param.key == CCKEY_FUNCTION) {
 		left = cnode->ms.sym->start;
 		right = sym->start;
+		if (cnode->ms.map)
+			left_dso = cnode->ms.map->dso;
+		if (node->map)
+			right_dso = node->map->dso;
 	} else {
 		left = cnode->ip;
 		right = node->ip;
 	}
 
-	if (left == right) {
+	if (left == right && left_dso == right_dso) {
 		if (node->branch) {
 			cnode->branch_count++;
 
-- 
1.8.3.1

Re: [PATCH] perf callchain: Compare dsos (as well) for CCKEY_FUNCTION

[Namhyung Kim]

Hi,

On Thu, Oct 05, 2017 at 09:20:21AM +0530, Ravi Bangoria wrote:
> Two functions from different binaries can have same start
> address. Thus, comparing only start address in match_chain()
> leads to inconsistent callchains. Fix this by adding a check
> for dsos as well.
> 
> Ex, https://www.spinics.net/lists/linux-perf-users/msg04067.html
> 
> Reported-by: Alexander Pozdneev <pozdneyev@gmail.com>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> ---
>  tools/perf/util/callchain.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> index be09d77..6d7f645 100644
> --- a/tools/perf/util/callchain.c
> +++ b/tools/perf/util/callchain.c
> @@ -685,6 +685,9 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
>  {
>  	struct symbol *sym = node->sym;
>  	u64 left, right;
> +	struct dso *left_dso = NULL;
> +	struct dso *right_dso = NULL;
> +
>  
>  	if (callchain_param.key == CCKEY_SRCLINE) {
>  		enum match_result match = match_chain_srcline(node, cnode);
> @@ -696,12 +699,16 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
>  	if (cnode->ms.sym && sym && callchain_param.key == CCKEY_FUNCTION) {
>  		left = cnode->ms.sym->start;
>  		right = sym->start;
> +		if (cnode->ms.map)
> +			left_dso = cnode->ms.map->dso;
> +		if (node->map)
> +			right_dso = node->map->dso;

AFAIK having a symbol guarantees having a map too.  So it could simply
be:

		left_dso = cnode->ms.map->dso;
		right_dso = node->map->dso;

Thanks,
Namhyung


>  	} else {
>  		left = cnode->ip;
>  		right = node->ip;
>  	}
>  
> -	if (left == right) {
> +	if (left == right && left_dso == right_dso) {
>  		if (node->branch) {
>  			cnode->branch_count++;
>  
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] perf callchain: Compare dsos (as well) for CCKEY_FUNCTION

[Ravi Bangoria]

Two functions from different binaries can have same start
address. Thus, comparing only start address in match_chain()
leads to inconsistent callchains. Fix this by adding a check
for dsos as well.

Ex, https://www.spinics.net/lists/linux-perf-users/msg04067.html

Reported-by: Alexander Pozdneev <pozdneyev@gmail.com>
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
Changes in v2:
  - Remove unnecessary checks for 'map'

 tools/perf/util/callchain.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index be09d77..a971caf 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -685,6 +685,8 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
 {
 	struct symbol *sym = node->sym;
 	u64 left, right;
+	struct dso *left_dso = NULL;
+	struct dso *right_dso = NULL;
 
 	if (callchain_param.key == CCKEY_SRCLINE) {
 		enum match_result match = match_chain_srcline(node, cnode);
@@ -696,12 +698,14 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
 	if (cnode->ms.sym && sym && callchain_param.key == CCKEY_FUNCTION) {
 		left = cnode->ms.sym->start;
 		right = sym->start;
+		left_dso = cnode->ms.map->dso;
+		right_dso = node->map->dso;
 	} else {
 		left = cnode->ip;
 		right = node->ip;
 	}
 
-	if (left == right) {
+	if (left == right && left_dso == right_dso) {
 		if (node->branch) {
 			cnode->branch_count++;
 
-- 
1.8.3.1

Re: [PATCH v2] perf callchain: Compare dsos (as well) for CCKEY_FUNCTION

[Jiri Olsa]

On Thu, Oct 05, 2017 at 02:42:34PM +0530, Ravi Bangoria wrote:
> Two functions from different binaries can have same start
> address. Thus, comparing only start address in match_chain()
> leads to inconsistent callchains. Fix this by adding a check
> for dsos as well.
> 
> Ex, https://www.spinics.net/lists/linux-perf-users/msg04067.html
> 
> Reported-by: Alexander Pozdneev <pozdneyev@gmail.com>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> ---
> Changes in v2:
>   - Remove unnecessary checks for 'map'

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> 
>  tools/perf/util/callchain.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> index be09d77..a971caf 100644
> --- a/tools/perf/util/callchain.c
> +++ b/tools/perf/util/callchain.c
> @@ -685,6 +685,8 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
>  {
>  	struct symbol *sym = node->sym;
>  	u64 left, right;
> +	struct dso *left_dso = NULL;
> +	struct dso *right_dso = NULL;
>  
>  	if (callchain_param.key == CCKEY_SRCLINE) {
>  		enum match_result match = match_chain_srcline(node, cnode);
> @@ -696,12 +698,14 @@ static enum match_result match_chain(struct callchain_cursor_node *node,
>  	if (cnode->ms.sym && sym && callchain_param.key == CCKEY_FUNCTION) {
>  		left = cnode->ms.sym->start;
>  		right = sym->start;
> +		left_dso = cnode->ms.map->dso;
> +		right_dso = node->map->dso;
>  	} else {
>  		left = cnode->ip;
>  		right = node->ip;
>  	}
>  
> -	if (left == right) {
> +	if (left == right && left_dso == right_dso) {
>  		if (node->branch) {
>  			cnode->branch_count++;
>  
> -- 
> 1.8.3.1
>