Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1752929AbdIFNNk (ORCPT <rfc822;w@1wt.eu>);
        Wed, 6 Sep 2017 09:13:40 -0400
Received: from mail.kdab.com ([176.9.126.58]:58244 "EHLO mail.kdab.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1750739AbdIFNNj (ORCPT <rfc822;Linux-kernel@vger.kernel.org>);
        Wed, 6 Sep 2017 09:13:39 -0400
From: Milian Wolff <milian.wolff@kdab.com>
To: Milian Wolff <milian.wolff@kdab.com>
Cc: Namhyung Kim <namhyung@kernel.org>, acme@kernel.org,
        Jin Yao <yao.jin@linux.intel.com>, Linux-kernel@vger.kernel.org,
        linux-perf-users@vger.kernel.org,
        Arnaldo Carvalho de Melo <acme@redhat.com>,
        David Ahern <dsahern@gmail.com>,
        Peter Zijlstra <a.p.zijlstra@chello.nl>, kernel-team@lge.com
Subject: Re: [PATCH v2 03/14] perf report: create real callchain entries for inlined frames
Date: Wed, 06 Sep 2017 15:13:35 +0200
Message-ID: <1935427.QE1zWV8lNS@agathebauer>
Organization: KDAB (Deutschland) GmbH&Co KG, a KDAB Group company
In-Reply-To: <15340121.XrrsYTu5UN@agathebauer>
References: <20170806212446.24925-1-milian.wolff@kdab.com> <20170816075313.GA26397@sejong> <15340121.XrrsYTu5UN@agathebauer>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 8640
Lines: 283

On Sonntag, 20. August 2017 22:57:17 CEST Milian Wolff wrote:
> On Mittwoch, 16. August 2017 09:53:13 CEST Namhyung Kim wrote:
> > Hi Milian,
> > 
> > On Sun, Aug 06, 2017 at 11:24:35PM +0200, Milian Wolff wrote:
> > > 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>
> 
> > > @@ -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?
> 
> > > 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.
> > 
> > >  	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
> 
> */
> 
> > >  	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]
> > 
> > > 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.
> 
> > > +	} 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]
> > 
> > > @@ -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. 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.

Cheers

-- 
Milian Wolff | milian.wolff@kdab.com | Senior Software Engineer
KDAB (Deutschland) GmbH&Co KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts