Re: [PATCH v5 3/5] perf callchain: Add support for cross-platform unwind

[Jiri Olsa <jolsa@redhat.com>]

On Tue, May 24, 2016 at 09:20:27AM +0000, He Kuang wrote:
> Use thread specific unwind ops to unwind cross-platform callchains.
> 
> Currently, 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. We test for x86 platform and only show proper
> messages. The real unwind methods are not implemented, will be
> introduced in next patch.
> 
> CONFIG_LIBUNWIND/NO_LIBUNWIND are changed to
> CONFIG_LOCAL_LIBUNWIND/NO_LOCAL_LIBUNWIND for retaining local unwind
> features. CONFIG_LIBUNWIND stands for either local or remote or both
> unwind are supported and NO_LIBUNWIND means neither local nor remote
> libunwind are supported.

hi,
I think this is too complex and error prone, I'd rather see it split
to several pieces. Basically every logicaly single piece should be
in separate patch for better readablebility and review.

I might be missing some but I'd mainly sepatate following:

  - introducing struct unwind_libunwind_ops for local unwind
  - moving unwind__prepare_access from thread_new into thread__insert_map
  - keep unwind__prepare_access name instead of unwind__get_arch
    and keep the return value, we need to bail out in case of error
  - I wouldn't use null ops.. just check for for ops == NULL in wrapper function

  - I understand we need to compile 3 objects from unwind-libunwind.c,
    how about we create 3 files like:

    util/unwind-libunwind-local.c
    util/unwind-libunwind-x86_32.c
    util/unwind-libunwind-arm64.c

    which would setup all necessary defines and include unwind-libunwind.c like:

    ---
    /* comments explaining every define ;-) */
    ...
    #define LOCAL... REMOTE..
    ...
    #include <util/unwind-libunwind-local.c>
    ...
    ----

    this way we will keep all the special setup for given unwind object
    in one place and you can also use simple rule in the Build file like
    without defining special rule:

    libperf-$(CONFIG_LIBUNWIND_X86)      += unwind-libunwind_x86_32.o
    libperf-$(CONFIG_LIBUNWIND_AARCH64)  += unwind-libunwind_arm64.o

    the same way for the arch object:

    arch/x86/util/unwind-libunwind-local.c
    arch/x86/util/unwind-libunwind-x86_32.c


Not sure I thought everything through, but I think this way
we'll keep it more maintainable and readable..

let me know what you think

thanks,
jirka