Hi Milian,

On Wed, Sep 06, 2017 at 03:13:35PM +0200, Milian Wolff wrote:

On Sonntag, 20. August 2017 22:57:17 CEST Milian Wolff wrote:

quoted

On Mittwoch, 16. August 2017 09:53:13 CEST Namhyung Kim wrote:

quoted

Hi Milian,

On Sun, Aug 06, 2017 at 11:24:35PM +0200, Milian Wolff wrote:

quoted

The inlined frames use a fake symbol that is tracked in a special
map inside the dso, which is always sorted by name. All other

It seems the above is not true.  Fake symbols are maintained by
inline_node which in turn maintained by dso->inlines tree.

OK, I'll rephrase this to make it more explicit. But what I call "map" is
what you call "tree", no?

<snip>

quoted

quoted

@@ -201,7 +202,8 @@ static inline void callchain_cursor_reset(struct

callchain_cursor *cursor)>

 int callchain_cursor_append(struct callchain_cursor *cursor, u64 ip,
 
 			    struct map *map, struct symbol *sym,
 			    bool branch, struct branch_flags *flags,

-			    int nr_loop_iter, int samples, u64 branch_from);
+			    int nr_loop_iter, int samples, u64 branch_from,
+			    const char *srcline);

 /* Close a cursor writing session. Initialize for the reader */
 static inline void callchain_cursor_commit(struct callchain_cursor
 *cursor)

I think it'd be better splitting srcline change into a separate
commit.

OK, I'll try to see how this goes. It would simply add the boiler plate code
to pass the srcline around, and then set it from within the callchain code,
right?

quoted

quoted

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index b9e087fb8247..72e6e390fd26 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c

@@ -9,6 +9,7 @@

 #include "compress.h"
 #include "path.h"
 #include "symbol.h"

+#include "srcline.h"

 #include "dso.h"
 #include "machine.h"
 #include "auxtrace.h"

@@ -1233,6 +1234,7 @@ void dso__delete(struct dso *dso)

 		       dso->long_name);
 	
 	for (i = 0; i < MAP__NR_TYPES; ++i)
 	
 		symbols__delete(&dso->symbols[i]);

+	inlines__tree_delete(&dso->inlined_nodes);

Hmm.. inline_node is released after symbol but it seems to have a
problem.  Please see below.

quoted

 	if (dso->short_name_allocated) {
 	
 		zfree((char **)&dso->short_name);

diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index f886141678eb..7d1e2b3c1f10 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h

@@ -141,6 +141,7 @@ struct dso {

 	struct rb_root	 *root;		/* root of rbtree that rb_node is in

*/

quoted

quoted

 	struct rb_root	 symbols[MAP__NR_TYPES];
 	struct rb_root	 symbol_names[MAP__NR_TYPES];

+	struct rb_root	 inlined_nodes;

 	struct {
 	
 		u64		addr;
 		struct symbol	*symbol;

[SNIP]

quoted

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index d4df353051af..a7f8499c8756 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
index ebc88a74e67b..a1fdf035d1dd 100644
--- a/tools/perf/util/srcline.c
+++ b/tools/perf/util/srcline.c

@@ -33,28 +33,17 @@ static const char *dso__name(struct dso *dso)

 	return dso_name;
 
 }

-static int inline_list__append(char *filename, char *funcname, int

line_nr, -			       struct inline_node *node, struct dso *dso)
+static int inline_list__append(struct symbol *symbol, char *srcline,
+			       struct inline_node *node)

 {
 
 	struct inline_list *ilist;

-	char *demangled;

 	ilist = zalloc(sizeof(*ilist));
 	if (ilist == NULL)
 	
 		return -1;

-	ilist->filename = filename;
-	ilist->line_nr = line_nr;
-
-	if (dso != NULL) {
-		demangled = dso__demangle_sym(dso, 0, funcname);
-		if (demangled == NULL) {
-			ilist->funcname = funcname;
-		} else {
-			ilist->funcname = demangled;
-			free(funcname);
-		}
-	}
+	ilist->symbol = symbol;
+	ilist->srcline = srcline;

 	if (callchain_param.order == ORDER_CALLEE)
 	
 		list_add_tail(&ilist->list, &node->val);

@@ -64,6 +53,30 @@ static int inline_list__append(char *filename, char

*funcname, int line_nr,>

 	return 0;
 
 }

+// basename version that takes a const input string
+static const char *gnu_basename(const char *path)
+{
+	const char *base = strrchr(path, '/');
+
+	return base ? base + 1 : path;
+}
+
+static char *srcline_from_fileline(const char *file, unsigned int line)
+{
+	char *srcline;
+
+	if (!file)
+		return NULL;
+
+	if (!srcline_full_filename)
+		file = gnu_basename(file);
+
+	if (asprintf(&srcline, "%s:%u", file, line) < 0)
+		return NULL;
+
+	return srcline;
+}
+

 #ifdef HAVE_LIBBFD_SUPPORT
 
 /*

@@ -203,19 +216,55 @@ static void addr2line_cleanup(struct a2l_data

*a2l)

 #define MAX_INLINE_NEST 1024

+static struct symbol *new_inline_sym(struct dso *dso,
+				     struct symbol *base_sym,
+				     const char *funcname)
+{
+	struct symbol *inline_sym;
+	char *demangled = NULL;
+
+	if (dso) {
+		demangled = dso__demangle_sym(dso, 0, funcname);
+		if (demangled)
+			funcname = demangled;
+	}
+
+	if (strcmp(funcname, base_sym->name) == 0) {
+		// reuse the real, existing symbol
+		inline_sym = base_sym;

So inline_node could refer the existing symbol.

Yes, that does indeed seem to be an issue. I wonder why I'm not seeing any
crashes or valgrind/ASAN reports though. I'll have to dig into this.

quoted

quoted

+	} else {
+		// create a fake symbol for the inline frame
+		inline_sym = symbol__new(base_sym ? base_sym->start : 0,
+					 base_sym ? base_sym->end : 0,
+					 base_sym ? base_sym->binding : 0,
+					 funcname);
+		if (inline_sym)
+			inline_sym->inlined = 1;
+	}
+
+	free(demangled);
+
+	return inline_sym;
+}
+

 static int inline_list__append_dso_a2l(struct dso *dso,

-				       struct inline_node *node)
+				       struct inline_node *node,
+				       struct symbol *sym)

 {
 
 	struct a2l_data *a2l = dso->a2l;

-	char *funcname = a2l->funcname ? strdup(a2l->funcname) : NULL;
-	char *filename = a2l->filename ? strdup(a2l->filename) : NULL;
+	struct symbol *inline_sym = new_inline_sym(dso, sym, a2l->funcname);
+	char *srcline = NULL;
+
+	if (a2l->filename)
+		srcline = srcline_from_fileline(a2l->filename, a2l->line);

-	return inline_list__append(filename, funcname, a2l->line, node, dso);
+	return inline_list__append(inline_sym, srcline, node);

 }

[SNIP]

quoted

@@ -511,10 +563,63 @@ void inline_node__delete(struct inline_node *node)

 	list_for_each_entry_safe(ilist, tmp, &node->val, list) {
 	
 		list_del_init(&ilist->list);

-		zfree(&ilist->filename);
-		zfree(&ilist->funcname);
+		zfree(&ilist->srcline);
+		// only the inlined symbols are owned by the list
+		if (ilist->symbol && ilist->symbol->inlined)
+			symbol__delete(ilist->symbol);

Existing symbols are released at this moment.

Thanks for the review, I'll try to look into these issues once I have more
time again.

OK, so I just dug into this part of the patch again. I don't think it's 
actually a problem after all:

When an inline node reuses the real symbol, that symbol won't have its 
`inlined` member set to true. Thus these symbols will never get deleted by 
inline_node__delete.

But ilist->symbol is a dangling pointer so accessing ->inlined would
be a problem, no?

If you have suggestions on how to make this clearer, I'm 
all ears. For now, I'll add a comment to where we alias/reuse the symbol.

I'll try to split up the patch now to make it somehow easier to review.

Thanks for doing this, but I'm afraid I don't have time to review
before going to OSS-NA.

Thanks,
Namhyung