Messages in this thread |  | | Date | Wed, 12 Nov 2014 13:33:43 +0100 | From | Jiri Olsa <> | Subject | Re: [PATCH 2/2] perf tools: Construct LBR call chain |
| |
On Thu, Nov 06, 2014 at 09:58:06AM -0500, kan.liang@intel.com wrote: > From: Kan Liang <kan.liang@intel.com> >
SNIP
> + /* LBR call stack */ > + if (lbr) { > + struct branch_stack *lbr_stack = sample->branch_stack; > + int lbr_nr = lbr_stack->nr; > + int mix_chain_nr; > > - if (callchain_param.order == ORDER_CALLEE) > - j = i; > - else > - j = chain->nr - i - 1; > + for (i = 0; i < chain_nr; i++) { > + if (chain->ips[i] == PERF_CONTEXT_USER) > + break; > + } > > -#ifdef HAVE_SKIP_CALLCHAIN_IDX > - if (j == skip_idx) > - continue; > -#endif > - ip = chain->ips[j]; > + /* LBR only affects the user callchain */ > + if (i == chain_nr) { > + lbr = 0; > + goto again; > + }
'goto again' sounds like u want to try something again, but you prepare the condition already with lbr = 0, could you please restruct the code in another way?
SNIP
> + goto exit; > } > + } else { > > - err = callchain_cursor_append(&callchain_cursor, > - ip, al.map, al.sym); > - if (err) > - return err; > - } > + /* > + * Based on DWARF debug information, some architectures skip > + * a callchain entry saved by the kernel. > + */ > + skip_idx = arch_skip_callchain_idx(thread, chain); > > - return 0; > + for (i = 0; i < chain_nr; i++) { > + struct addr_location al; > + > + if (callchain_param.order == ORDER_CALLEE) > + j = i; > + else > + j = chain->nr - i - 1; > + > +#ifdef HAVE_SKIP_CALLCHAIN_IDX > + if (j == skip_idx) > + continue; > +#endif > + ip = chain->ips[j]; > + err = __thread__resolve_callchain_sample(thread,
Uou factored out the common functionality into __thread__resolve_callchain_sample function and added your own functionality.. could you please split this into 2 separate patches? (first the new function, then your change) IMO It'll make the change simple and more obvious.
thanks, jirka
|  |