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

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

[Chris Phlipot]

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

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

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

[PATCH 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..424c77c 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