Messages in this thread | | | Subject | Re: [PATCH v5 3/5] perf callchain: Add support for cross-platform unwind | From | Hekuang <> | Date | Fri, 27 May 2016 15:13:04 +0800 |
| |
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 >
| |