lkml.org 
[lkml]   [2016]   [May]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v5 3/5] perf callchain: Add support for cross-platform unwind
From
Date
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
>


\
 
 \ /
  Last update: 2016-05-27 09:21    [W:0.159 / U:0.512 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site