Re: [PATCH 6/8] perf callchain: Add support for cross-platform unwind

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Fri, May 06, 2016 at 08:59:12AM +0000, He Kuang escreveu:
> Use thread specific unwind ops to unwind cross-platform callchains.
> 
> Before this patch, unwind methods is suitable for local unwind, this
> patch changes the fixed methods to thread/map related. Each time a map
> is inserted, we find the target arch and see if this platform can be
> remote unwind. In this patch, we test for x86 platform and only show
> proper messages. The real unwind methods are not implemented, will be
> introduced in next patch.
> 
> Signed-off-by: He Kuang <hekuang@huawei.com>
> ---
>  tools/perf/config/Makefile                | 19 ++++++++--
>  tools/perf/util/Build                     |  3 +-
>  tools/perf/util/thread.c                  | 29 +++++++++++----
>  tools/perf/util/thread.h                  | 14 +++++++-
>  tools/perf/util/unwind-libunwind.c        | 48 +++++++++++++++++++++----
>  tools/perf/util/unwind-libunwind_common.c | 60 +++++++++++++++++++++++++++++++
>  tools/perf/util/unwind.h                  | 22 ++++++++++++
>  7 files changed, 178 insertions(+), 17 deletions(-)
>  create mode 100644 tools/perf/util/unwind-libunwind_common.c
> 
> diff --git a/tools/perf/config/Makefile b/tools/perf/config/Makefile
> index a86b864..16f14b1 100644
> --- a/tools/perf/config/Makefile
> +++ b/tools/perf/config/Makefile
> @@ -345,14 +345,24 @@ ifeq ($(ARCH),powerpc)
>  endif
>  
>  ifndef NO_LIBUNWIND
> +  have_libunwind =
>    ifeq ($(feature-libunwind-x86), 1)
> -    LIBUNWIND_LIBS += -lunwind-x86
>      $(call detected,CONFIG_LIBUNWIND_X86)
>      CFLAGS += -DHAVE_LIBUNWIND_X86_SUPPORT
> +    LDFLAGS += -lunwind -lunwind-x86
> +    have_libunwind = 1
>    endif
>  
>    ifneq ($(feature-libunwind), 1)
>      msg := $(warning No libunwind found. Please install libunwind-dev[el] >= 1.1 and/or set LIBUNWIND_DIR);
> +    NO_LOCAL_LIBUNWIND := 1
> +  else
> +    have_libunwind = 1
> +    CFLAGS += -DHAVE_LIBUNWIND_LOCAL_SUPPORT
> +    $(call detected,CONFIG_LOCAL_LIBUNWIND)
> +  endif
> +
> +  ifneq ($(have_libunwind), 1)
>      NO_LIBUNWIND := 1
>    endif
>  endif
> @@ -392,7 +402,7 @@ else
>    NO_DWARF_UNWIND := 1
>  endif
>  
> -ifndef NO_LIBUNWIND
> +ifndef NO_LOCAL_LIBUNWIND
>    ifeq ($(ARCH),$(filter $(ARCH),arm arm64))
>      $(call feature_check,libunwind-debug-frame)
>      ifneq ($(feature-libunwind-debug-frame), 1)
> @@ -403,12 +413,15 @@ ifndef NO_LIBUNWIND
>      # non-ARM has no dwarf_find_debug_frame() function:
>      CFLAGS += -DNO_LIBUNWIND_DEBUG_FRAME
>    endif
> -  CFLAGS  += -DHAVE_LIBUNWIND_SUPPORT
>    EXTLIBS += $(LIBUNWIND_LIBS)
>    CFLAGS  += $(LIBUNWIND_CFLAGS)
>    LDFLAGS += $(LIBUNWIND_LDFLAGS)
>  endif
>  
> +ifndef NO_LIBUNWIND
> +  CFLAGS  += -DHAVE_LIBUNWIND_SUPPORT
> +endif
> +
>  ifndef NO_LIBAUDIT
>    ifneq ($(feature-libaudit), 1)
>      msg := $(warning No libaudit.h found, disables 'trace' tool, please install audit-libs-devel or libaudit-dev);
> diff --git a/tools/perf/util/Build b/tools/perf/util/Build
> index ea4ac03..2e21529 100644
> --- a/tools/perf/util/Build
> +++ b/tools/perf/util/Build
> @@ -97,7 +97,8 @@ libperf-$(CONFIG_DWARF) += probe-finder.o
>  libperf-$(CONFIG_DWARF) += dwarf-aux.o
>  
>  libperf-$(CONFIG_LIBDW_DWARF_UNWIND) += unwind-libdw.o
> -libperf-$(CONFIG_LIBUNWIND)          += unwind-libunwind.o
> +libperf-$(CONFIG_LOCAL_LIBUNWIND)    += unwind-libunwind.o
> +libperf-$(CONFIG_LIBUNWIND)          += unwind-libunwind_common.o
>  
>  libperf-$(CONFIG_LIBBABELTRACE) += data-convert-bt.o
>  
> diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
> index e0cdcf7..cf60db1 100644
> --- a/tools/perf/util/thread.c
> +++ b/tools/perf/util/thread.c
> @@ -41,9 +41,6 @@ struct thread *thread__new(pid_t pid, pid_t tid)
>  		thread->cpu = -1;
>  		INIT_LIST_HEAD(&thread->comm_list);
>  
> -		if (unwind__prepare_access(thread) < 0)
> -			goto err_thread;
> -
>  		comm_str = malloc(32);
>  		if (!comm_str)
>  			goto err_thread;
> @@ -57,6 +54,9 @@ struct thread *thread__new(pid_t pid, pid_t tid)
>  		list_add(&comm->list, &thread->comm_list);
>  		atomic_set(&thread->refcnt, 1);
>  		RB_CLEAR_NODE(&thread->rb_node);
> +#ifdef HAVE_LIBUNWIND_SUPPORT
> +		register_null_unwind_libunwind_ops(thread);
> +#endif

Could you please avoid having all those ifdefs sprinkled in the .c file?
For instance, the above ifdef/endif should be dropped and instead, at a
central place, a header probably, or at most at the start of the .c
file, you should have one ifdef block, the else should just make the
above function, for instance, be:

#ifdef HAVE_LIBUNWIND_SUPPORT
void register_null_unwind_libunwind_ops(struct thread *thread);
#else
static inline
void register_null_unwind_libunwind_ops(struct thread *thread __maybe_unused)
{
}
#endif

>  	}
>  
>  	return thread;
> @@ -82,7 +82,9 @@ void thread__delete(struct thread *thread)
>  		list_del(&comm->list);
>  		comm__free(comm);
>  	}
> -	unwind__finish_access(thread);
> +#ifdef HAVE_LIBUNWIND_SUPPORT
> +	thread->unwind_libunwind_ops->finish_access(thread);
> +#endif
>  
>  	free(thread);
>  }
> @@ -144,8 +146,10 @@ int __thread__set_comm(struct thread *thread, const char *str, u64 timestamp,
>  			return -ENOMEM;
>  		list_add(&new->list, &thread->comm_list);
>  
> +#ifdef HAVE_LIBUNWIND_SUPPORT
>  		if (exec)
> -			unwind__flush_access(thread);
> +			thread->unwind_libunwind_ops->flush_access(thread);
> +#endif

You see, here the original code should be left untouched and you would
change unwind__flush_access() instead to be:

#ifdef HAVE_LIBUNWIND_SUPPORT
void unwind__flush_access(struct thread *thread)
{
	thread->unwind_libunwind_ops->flush_access(thread);
}
#else
void unwind__flush_access(struct thread *thread __maybe_unused)
{
}
#endif

combine the above if/else/endif with the previous and with all the
others, please.

>  	}
>  
>  	thread->comm_set = true;
> @@ -208,14 +212,27 @@ void thread__insert_map(struct thread *thread, struct map *map)
>  		   || !strcmp(arch, "x86")
>  		   || !strcmp(arch, "i686")) {
>  		pr_debug("Thread map is X86, 64bit is %d\n", is_64_bit);
> -		if (!is_64_bit)
> +		if (!is_64_bit) {
>  #ifdef HAVE_LIBUNWIND_X86_SUPPORT
>  			pr_err("target platform=%s is not implemented!\n",
>  			       arch);
>  #else
>  			pr_err("target platform=%s is not supported!\n", arch);
>  #endif
> +			goto err;
> +		}
> +	} else {
> +		register_local_unwind_libunwind_ops(thread);
>  	}
> +
> +	if (thread->unwind_libunwind_ops->prepare_access(thread) < 0)
> +		return;
> +
> +	return;
> +
> +err: __maybe_unused
> +	register_null_unwind_libunwind_ops(thread);
> +	return;
>  #endif
>  }
>  
> diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
> index e214207..6f2d4cd 100644
> --- a/tools/perf/util/thread.h
> +++ b/tools/perf/util/thread.h
> @@ -15,6 +15,17 @@
>  
>  struct thread_stack;
>  
> +struct unwind_entry;
> +typedef int (*unwind_entry_cb_t)(struct unwind_entry *entry, void *arg);
> +struct unwind_libunwind_ops {
> +	int (*prepare_access)(struct thread *thread);
> +	void (*flush_access)(struct thread *thread);
> +	void (*finish_access)(struct thread *thread);
> +	int (*get_entries)(unwind_entry_cb_t cb, void *arg,
> +			   struct thread *thread,
> +			   struct perf_sample *data, int max_stack);
> +};
> +
>  struct thread {
>  	union {
>  		struct rb_node	 rb_node;
> @@ -36,7 +47,8 @@ struct thread {
>  	void			*priv;
>  	struct thread_stack	*ts;
>  #ifdef HAVE_LIBUNWIND_SUPPORT
> -	unw_addr_space_t	addr_space;
> +	unw_addr_space_t		addr_space;
> +	struct unwind_libunwind_ops	*unwind_libunwind_ops;
>  #endif
>  };
>  
> diff --git a/tools/perf/util/unwind-libunwind.c b/tools/perf/util/unwind-libunwind.c
> index 63687d3..2558bf3 100644
> --- a/tools/perf/util/unwind-libunwind.c
> +++ b/tools/perf/util/unwind-libunwind.c
> @@ -22,6 +22,9 @@
>  #include <unistd.h>
>  #include <sys/mman.h>
>  #include <linux/list.h>
> +#ifdef ARCH_UNWIND_LIBUNWIND
> +#include "libunwind-arch.h"
> +#endif
>  #include <libunwind.h>
>  #include <libunwind-ptrace.h>
>  #include "callchain.h"
> @@ -34,6 +37,22 @@
>  #include "debug.h"
>  #include "asm/bug.h"
>  
> +#ifndef ARCH_UNWIND_LIBUNWIND
> +  #define LIBUNWIND__ARCH_REG_ID libunwind__arch_reg_id
> +  #define LOCAL_UNWIND_LIBUNWIND
> +  #undef UNWT_OBJ
> +  #define UNWT_OBJ(x) _##x
> +#else
> +  #undef NO_LIBUNWIND_DEBUG_FRAME
> +  #if defined(LIBUNWIND_ARM) && !defined(NO_LIBUNWIND_DEBUG_FRAME_ARM)
> +  #elif defined(LIBUNWIND_AARCH64) &&                    \
> +	  defined(NO_LIBUNWIND_DEBUG_FRAME_ARM_AARCH64)
> +  #else
> +    #define NO_LIBUNWIND_DEBUG_FRAME
> +  #endif
> +#endif
> +
> +
>  extern int
>  UNW_OBJ(dwarf_search_unwind_table) (unw_addr_space_t as,
>  				    unw_word_t ip,
> @@ -579,7 +598,7 @@ static unw_accessors_t accessors = {
>  	.get_proc_name		= get_proc_name,
>  };
>  
> -int unwind__prepare_access(struct thread *thread)
> +static int UNWT_OBJ(_unwind__prepare_access)(struct thread *thread)
>  {
>  	if (callchain_param.record_mode != CALLCHAIN_DWARF)
>  		return 0;
> @@ -594,7 +613,7 @@ int unwind__prepare_access(struct thread *thread)
>  	return 0;
>  }
>  
> -void unwind__flush_access(struct thread *thread)
> +static void UNWT_OBJ(_unwind__flush_access)(struct thread *thread)
>  {
>  	if (callchain_param.record_mode != CALLCHAIN_DWARF)
>  		return;
> @@ -602,7 +621,7 @@ void unwind__flush_access(struct thread *thread)
>  	unw_flush_cache(thread->addr_space, 0, 0);
>  }
>  
> -void unwind__finish_access(struct thread *thread)
> +static void UNWT_OBJ(_unwind__finish_access)(struct thread *thread)
>  {
>  	if (callchain_param.record_mode != CALLCHAIN_DWARF)
>  		return;
> @@ -662,9 +681,10 @@ static int get_entries(struct unwind_info *ui, unwind_entry_cb_t cb,
>  	return ret;
>  }
>  
> -int unwind__get_entries(unwind_entry_cb_t cb, void *arg,
> -			struct thread *thread,
> -			struct perf_sample *data, int max_stack)
> +static int UNWT_OBJ(_unwind__get_entries)(unwind_entry_cb_t cb, void *arg,
> +					 struct thread *thread,
> +					 struct perf_sample *data,
> +					 int max_stack)
>  {
>  	struct unwind_info ui = {
>  		.sample       = data,
> @@ -680,3 +700,19 @@ int unwind__get_entries(unwind_entry_cb_t cb, void *arg,
>  
>  	return get_entries(&ui, cb, arg, max_stack);
>  }
> +
> +struct unwind_libunwind_ops
> +UNWT_OBJ(unwind_libunwind_ops) = {
> +	.prepare_access = UNWT_OBJ(_unwind__prepare_access),
> +	.flush_access   = UNWT_OBJ(_unwind__flush_access),
> +	.finish_access  = UNWT_OBJ(_unwind__finish_access),
> +	.get_entries    = UNWT_OBJ(_unwind__get_entries),
> +};
> +
> +#ifdef LOCAL_UNWIND_LIBUNWIND
> +int register_local_unwind_libunwind_ops(struct thread *thread)
> +{
> +	thread->unwind_libunwind_ops = &UNWT_OBJ(unwind_libunwind_ops);
> +	return 0;
> +}
> +#endif
> diff --git a/tools/perf/util/unwind-libunwind_common.c b/tools/perf/util/unwind-libunwind_common.c
> new file mode 100644
> index 0000000..47433a4
> --- /dev/null
> +++ b/tools/perf/util/unwind-libunwind_common.c
> @@ -0,0 +1,60 @@
> +#include <elf.h>
> +#include <gelf.h>
> +#include <fcntl.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <sys/mman.h>
> +#include <linux/list.h>
> +#include <libunwind.h>
> +#include <libunwind-ptrace.h>
> +#include "callchain.h"
> +#include "thread.h"
> +#include "session.h"
> +#include "perf_regs.h"
> +#include "unwind.h"
> +#include "symbol.h"
> +#include "util.h"
> +#include "debug.h"
> +#include "asm/bug.h"
> +
> +static int __null__prepare_access(struct thread *thread __maybe_unused)
> +{
> +	return 0;
> +}
> +
> +static void __null__flush_access(struct thread *thread __maybe_unused)
> +{
> +}
> +
> +static void __null__finish_access(struct thread *thread __maybe_unused)
> +{
> +}
> +
> +static int __null__get_entries(unwind_entry_cb_t cb __maybe_unused,
> +			       void *arg __maybe_unused,
> +			       struct thread *thread __maybe_unused,
> +			       struct perf_sample *data __maybe_unused,
> +			       int max_stack __maybe_unused)
> +{
> +	return 0;
> +}
> +
> +static struct unwind_libunwind_ops null_unwind_libunwind_ops = {
> +	.prepare_access = __null__prepare_access,
> +	.flush_access   = __null__flush_access,
> +	.finish_access  = __null__finish_access,
> +	.get_entries    = __null__get_entries,
> +};
> +
> +int register_null_unwind_libunwind_ops(struct thread *thread)
> +{
> +	thread->unwind_libunwind_ops = &null_unwind_libunwind_ops;
> +	return 0;
> +}
> +
> +int register_unwind_libunwind_ops(struct unwind_libunwind_ops *ops,
> +				  struct thread *thread)
> +{
> +	thread->unwind_libunwind_ops = ops;
> +	return 0;
> +}
> diff --git a/tools/perf/util/unwind.h b/tools/perf/util/unwind.h
> index 12790cf..61b44a6 100644
> --- a/tools/perf/util/unwind.h
> +++ b/tools/perf/util/unwind.h
> @@ -24,6 +24,28 @@ int libunwind__arch_reg_id(int regnum);
>  int unwind__prepare_access(struct thread *thread);
>  void unwind__flush_access(struct thread *thread);
>  void unwind__finish_access(struct thread *thread);
> +int register_unwind_libunwind_ops(struct unwind_libunwind_ops *ops,
> +				  struct thread *thread);
> +int register_null_unwind_libunwind_ops(struct thread *thread);
> +
> +#ifndef HAVE_LIBUNWIND_LOCAL_SUPPORT
> +static inline int
> +register_local_unwind_libunwind_ops(struct thread *thread) {
> +	return register_null_unwind_libunwind_ops(thread);
> +}
> +#else
> +int register_local_unwind_libunwind_ops(struct thread *thread);
> +#endif
> +
> +#define unwind__get_entries(cb, arg,					\
> +			    thread,					\
> +			    data, max_stack)				\
> +	thread->unwind_libunwind_ops->get_entries(cb,			\
> +						  arg,			\
> +						  thread,		\
> +						  data,			\
> +						  max_stack)
> +
>  #else
>  static inline int unwind__prepare_access(struct thread *thread __maybe_unused)
>  {
> -- 
> 1.8.5.2