[PATCH v2 0/4] perf script: fix duplicate symbols in db-export

[PATCH v2 0/4] perf script: fix duplicate symbols in db-export

[Chris Phlipot]

Changes since v1:
 - fixed scripts/checkpatch.pl errors

This patch set contains 3 fixes for duplicate symbol creation in the
db-export implementation and one new symbol API required for the fixes.

commit 9c7b37cd63d0 ("perf symbols: Fix handling of zero-length symbols.")
already removed the majority of duplicates, but these fixes take care of
the remaining corner cases.

each patch (except for the 1st, which is a dependency for patch 2) reduces
the number of duplicate symbols exported. When all patches are applied,
my test workload has no more duplicate symbols being exported.

Tests ran:

$perf record --call-graph=dwarf stress -c 2 -t 20
$perf script -s scripts/python/export-to-postgresql.py test all callchains
$psql test

To show the effect of the changes we run the following query before/after
the changes on a database created using the export-to-postgresql.py script
with callchains enabled. If this query returns any value greater than 1,
then it means that there are duplicates present. 


In the test workload, at least one symbol occurs 299 times before applying
the fixes:

  test=# select count(*) as cnt from symbols group by 
	sym_start,sym_end,dso_id order by cnt desc limit 1;
   cnt 
  -----
   299
  (1 row)

After applying the fixes no symbol occurs more than once:

  test=# select count(*) as cnt from symbols group by
	sym_start,sym_end,dso_id order by cnt desc limit 1;
   cnt 
  -----
     1
  (1 row)

Chris Phlipot (4):
  perf symbols: add dso__insert_symbol function
  perf script: fix symbol insertion behavior in db-export
  perf script: fix callchain addresses in db-export
  perf script: fix export of callchains with recursion in db-export

 tools/perf/util/db-export.c | 12 ++++++------
 tools/perf/util/symbol.c    | 12 ++++++++++++
 tools/perf/util/symbol.h    |  3 +++
 3 files changed, 21 insertions(+), 6 deletions(-)

-- 
2.7.4

[PATCH v2 1/4] perf symbols: add dso__insert_symbol function

[Chris Phlipot]

The current method for inserting symbols is to use the symbols__insert
function. However symbols__insert does not update the dso symbol cache.
This causes problems in the following scenario:

1. symbol not found at ip using dso__find_symbol

2. symbol inserted at ip using the existing symbols__insert function

3. symbol still not found at ip using dso__find_symbol because cache isn't
updated. This is undesired behavior.

The undesired behavior in (3) is addressed by creating a new function,
dso__insert_symbol to both insert the symbol and update the symbol cache
if necessary.

If dso__insert_symbol is used in (2) instead of symbols__insert, then the
undesired behavior in (3) is avoided.

Signed-off-by: Chris Phlipot <cphlipot0@gmail.com>
---
 tools/perf/util/symbol.c | 12 ++++++++++++
 tools/perf/util/symbol.h |  3 +++
 2 files changed, 15 insertions(+)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 2946295..21af8e8 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -413,6 +413,18 @@ void dso__reset_find_symbol_cache(struct dso *dso)
 	}
 }
 
+void dso__insert_symbol(struct dso *dso, enum map_type type, struct symbol *sym)
+{
+	symbols__insert(&dso->symbols[type], sym);
+
+	/* update the symbol cache if necessary */
+	if (dso->last_find_result[type].addr >= sym->start &&
+	    (dso->last_find_result[type].addr < sym->end ||
+	    sym->start == sym->end)) {
+		dso->last_find_result[type].symbol = sym;
+	}
+}
+
 struct symbol *dso__find_symbol(struct dso *dso,
 				enum map_type type, u64 addr)
 {
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 07211c2..2b5e4ed 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -246,6 +246,9 @@ int __dso__load_kallsyms(struct dso *dso, const char *filename, struct map *map,
 int dso__load_kallsyms(struct dso *dso, const char *filename, struct map *map,
 		       symbol_filter_t filter);
 
+void dso__insert_symbol(struct dso *dso, enum map_type type,
+			struct symbol *sym);
+
 struct symbol *dso__find_symbol(struct dso *dso, enum map_type type,
 				u64 addr);
 struct symbol *dso__find_symbol_by_name(struct dso *dso, enum map_type type,
-- 
2.7.4

[tip:perf/core] perf symbols: Add dso__insert_symbol function

[tip-bot for Chris Phlipot]

Commit-ID:  ae93a6c70838b87151ac12589dc507dbf4f2f067
Gitweb:     http://git.kernel.org/tip/ae93a6c70838b87151ac12589dc507dbf4f2f067
Author:     Chris Phlipot <cphlipot0@gmail.com>
AuthorDate: Tue, 10 May 2016 20:26:46 -0700
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 11 May 2016 12:24:57 -0300

perf symbols: Add dso__insert_symbol function

The current method for inserting symbols is to use the symbols__insert()
function. However symbols__insert() does not update the dso symbol
cache.  This causes problems in the following scenario:

1. symbol not found at addr using dso__find_symbol

2. symbol inserted at addr using the existing symbols__insert function

3. symbol still not found at addr using dso__find_symbol() because cache isn't
   updated. This is undesired behavior.

The undesired behavior in (3) is addressed by creating a new function,
dso__insert_symbol() to both insert the symbol and update the symbol
cache if necessary.

If dso__insert_symbol() is used in (2) instead of symbols__insert(),
then the undesired behavior in (3) is avoided.

Signed-off-by: Chris Phlipot <cphlipot0@gmail.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1462937209-6032-2-git-send-email-cphlipot0@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/symbol.c | 12 ++++++++++++
 tools/perf/util/symbol.h |  3 +++
 2 files changed, 15 insertions(+)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 2946295..21af8e8 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -413,6 +413,18 @@ void dso__reset_find_symbol_cache(struct dso *dso)
 	}
 }
 
+void dso__insert_symbol(struct dso *dso, enum map_type type, struct symbol *sym)
+{
+	symbols__insert(&dso->symbols[type], sym);
+
+	/* update the symbol cache if necessary */
+	if (dso->last_find_result[type].addr >= sym->start &&
+	    (dso->last_find_result[type].addr < sym->end ||
+	    sym->start == sym->end)) {
+		dso->last_find_result[type].symbol = sym;
+	}
+}
+
 struct symbol *dso__find_symbol(struct dso *dso,
 				enum map_type type, u64 addr)
 {
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 07211c2..2b5e4ed 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -246,6 +246,9 @@ int __dso__load_kallsyms(struct dso *dso, const char *filename, struct map *map,
 int dso__load_kallsyms(struct dso *dso, const char *filename, struct map *map,
 		       symbol_filter_t filter);
 
+void dso__insert_symbol(struct dso *dso, enum map_type type,
+			struct symbol *sym);
+
 struct symbol *dso__find_symbol(struct dso *dso, enum map_type type,
 				u64 addr);
 struct symbol *dso__find_symbol_by_name(struct dso *dso, enum map_type type,

[PATCH v2 2/4] perf script: fix symbol insertion behavior in db-export

[Chris Phlipot]

Use the dso__insert_symbol function instead of symbols__insert in order to
properly update the dso symbol cache.

If the cache is not updated, then duplicate symbols can be unintentionally
created, inserted, and exported.

This change prevents duplicate symbols from being exported due to
dso__find_symbol using a stale symbol cache.

Signed-off-by: Chris Phlipot <cphlipot0@gmail.com>
---
 tools/perf/util/db-export.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/perf/util/db-export.c b/tools/perf/util/db-export.c
index f8e3057..2ef1f69 100644
--- a/tools/perf/util/db-export.c
+++ b/tools/perf/util/db-export.c
@@ -260,8 +260,7 @@ static int db_ids_from_al(struct db_export *dbe, struct addr_location *al,
 		if (!al->sym) {
 			al->sym = symbol__new(al->addr, 0, 0, "unknown");
 			if (al->sym)
-				symbols__insert(&dso->symbols[al->map->type],
-						al->sym);
+				dso__insert_symbol(dso, al->map->type, al->sym);
 		}
 
 		if (al->sym) {
-- 
2.7.4

[tip:perf/core] perf script: Fix symbol insertion behavior in db-export

[tip-bot for Chris Phlipot]

Commit-ID:  bd0a51dd2794f1d17d4e7a34ad66db845cef3e5a
Gitweb:     http://git.kernel.org/tip/bd0a51dd2794f1d17d4e7a34ad66db845cef3e5a
Author:     Chris Phlipot <cphlipot0@gmail.com>
AuthorDate: Tue, 10 May 2016 20:26:47 -0700
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 11 May 2016 12:24:57 -0300

perf script: Fix symbol insertion behavior in db-export

Use the dso__insert_symbol function instead of symbols__insert() in
order to properly update the dso symbol cache.

If the cache is not updated, then duplicate symbols can be
unintentionally created, inserted, and exported.

This change prevents duplicate symbols from being exported due to
dso__find_symbol() using a stale symbol cache.

Signed-off-by: Chris Phlipot <cphlipot0@gmail.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1462937209-6032-3-git-send-email-cphlipot0@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/db-export.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/perf/util/db-export.c b/tools/perf/util/db-export.c
index f8e3057..2ef1f69 100644
--- a/tools/perf/util/db-export.c
+++ b/tools/perf/util/db-export.c
@@ -260,8 +260,7 @@ static int db_ids_from_al(struct db_export *dbe, struct addr_location *al,
 		if (!al->sym) {
 			al->sym = symbol__new(al->addr, 0, 0, "unknown");
 			if (al->sym)
-				symbols__insert(&dso->symbols[al->map->type],
-						al->sym);
+				dso__insert_symbol(dso, al->map->type, al->sym);
 		}
 
 		if (al->sym) {

[PATCH v2 3/4] perf script: fix callchain addresses in db-export

[Chris Phlipot]

Remove the call to map_ip, because it has already been called when
assembling the callchain. Calling it a second time can result in incorrect
addresses being used. This can have effects such as duplicate symbols
being created and exported.

Signed-off-by: Chris Phlipot <cphlipot0@gmail.com>
---
 tools/perf/util/db-export.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/perf/util/db-export.c b/tools/perf/util/db-export.c
index 2ef1f69..8ca4186 100644
--- a/tools/perf/util/db-export.c
+++ b/tools/perf/util/db-export.c
@@ -324,10 +324,7 @@ static struct call_path *call_path_from_sample(struct db_export *dbe,
 		al.sym = node->sym;
 		al.map = node->map;
 		al.machine = machine;
-		if (al.map)
-			al.addr = al.map->map_ip(al.map, node->ip);
-		else
-			al.addr = node->ip;
+		al.addr = node->ip;
 
 		db_ids_from_al(dbe, &al, &dso_db_id, &sym_db_id, &offset);
 
-- 
2.7.4

[tip:perf/core] perf script: Fix callchain addresses in db-export

[tip-bot for Chris Phlipot]

Commit-ID:  7a2544c004a6c576b1e307f30925b165affe6a22
Gitweb:     http://git.kernel.org/tip/7a2544c004a6c576b1e307f30925b165affe6a22
Author:     Chris Phlipot <cphlipot0@gmail.com>
AuthorDate: Tue, 10 May 2016 20:26:48 -0700
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 11 May 2016 12:24:58 -0300

perf script: Fix callchain addresses in db-export

Remove the call to map_ip() to adjust al.addr, because it has already
been called when assembling the callchain, in:

  thread__resolve_callchain_sample(perf_sample)
      add_callchain_ip(ip = perf_sample->callchain->ips[j])
          thread__find_addr_location(addr = ip)
              thread__find_addr_map(addr) {
                  al->addr = addr
                  if (al->map)
                      al->addr = al->map->map_ip(al->map, al->addr);
              }

Calling it a second time can result in incorrect addresses being used.
This can have effects such as duplicate symbols being created and
exported.

Signed-off-by: Chris Phlipot <cphlipot0@gmail.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1462937209-6032-4-git-send-email-cphlipot0@gmail.com
[ Show the callchain where it is done, to help reviewing this change down the line ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/db-export.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/perf/util/db-export.c b/tools/perf/util/db-export.c
index 2ef1f69..8ca4186 100644
--- a/tools/perf/util/db-export.c
+++ b/tools/perf/util/db-export.c
@@ -324,10 +324,7 @@ static struct call_path *call_path_from_sample(struct db_export *dbe,
 		al.sym = node->sym;
 		al.map = node->map;
 		al.machine = machine;
-		if (al.map)
-			al.addr = al.map->map_ip(al.map, node->ip);
-		else
-			al.addr = node->ip;
+		al.addr = node->ip;
 
 		db_ids_from_al(dbe, &al, &dso_db_id, &sym_db_id, &offset);
 

[PATCH v2 4/4] perf script: fix export of callchains with recursion in db-export

[Chris Phlipot]

When an IP with an unresolved symbol occurs in the callchain more than
once (ie. recursion), then duplicate symbols can be created because
the callchain nodes are never updated after they are first created.

To fix this issue we call dso__find_symbol whenever we encounter a NULL
symbol, in case we already added a symbol at that IP since we started
traversing the callchain.

This change prevents duplicate symbols from being exported when duplicate
IPs are present in the callchain.

Signed-off-by: Chris Phlipot <cphlipot0@gmail.com>
---
 tools/perf/util/db-export.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/perf/util/db-export.c b/tools/perf/util/db-export.c
index 8ca4186..8d96c80 100644
--- a/tools/perf/util/db-export.c
+++ b/tools/perf/util/db-export.c
@@ -326,6 +326,10 @@ static struct call_path *call_path_from_sample(struct db_export *dbe,
 		al.machine = machine;
 		al.addr = node->ip;
 
+		if (al.map && !al.sym)
+			al.sym = dso__find_symbol(al.map->dso, MAP__FUNCTION,
+						  al.addr);
+
 		db_ids_from_al(dbe, &al, &dso_db_id, &sym_db_id, &offset);
 
 		/* add node to the call path tree if it doesn't exist */
-- 
2.7.4

[tip:perf/core] perf script: Fix export of callchains with recursion in db-export

[tip-bot for Chris Phlipot]

Commit-ID:  83302e79b18f75266e4a44281e8432f61d57d441
Gitweb:     http://git.kernel.org/tip/83302e79b18f75266e4a44281e8432f61d57d441
Author:     Chris Phlipot <cphlipot0@gmail.com>
AuthorDate: Tue, 10 May 2016 20:26:49 -0700
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 11 May 2016 12:24:58 -0300

perf script: Fix export of callchains with recursion in db-export

When an IP with an unresolved symbol occurs in the callchain more than
once (ie. recursion), then duplicate symbols can be created because
the callchain nodes are never updated after they are first created.

To fix this issue we call dso__find_symbol whenever we encounter a NULL
symbol, in case we already added a symbol at that IP since we started
traversing the callchain.

This change prevents duplicate symbols from being exported when duplicate
IPs are present in the callchain.

Signed-off-by: Chris Phlipot <cphlipot0@gmail.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1462937209-6032-5-git-send-email-cphlipot0@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/db-export.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/perf/util/db-export.c b/tools/perf/util/db-export.c
index 8ca4186..8d96c80 100644
--- a/tools/perf/util/db-export.c
+++ b/tools/perf/util/db-export.c
@@ -326,6 +326,10 @@ static struct call_path *call_path_from_sample(struct db_export *dbe,
 		al.machine = machine;
 		al.addr = node->ip;
 
+		if (al.map && !al.sym)
+			al.sym = dso__find_symbol(al.map->dso, MAP__FUNCTION,
+						  al.addr);
+
 		db_ids_from_al(dbe, &al, &dso_db_id, &sym_db_id, &offset);
 
 		/* add node to the call path tree if it doesn't exist */