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

[Hekuang]

hi

å 2016/5/27 1:42, Jiri Olsa åé:
> 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

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

The only concern is that, if later we support more platforms,
there will be too much files named as 'tools/perf/util/unwind-libunwind*.c'
Is it acceptable or not?

And I thought all files belongs to specific archs should
go to folder under 'tools/perf/arch/xxx', is that right?

Thanks.
> thanks,
> jirka