Re: [PATCH 1/5] perf tools: fix incorrect ordering of callchain entries

[Adrian Hunter]

+Jiri since he wrote the original code

On 22/04/16 10:55, Adrian Hunter wrote:
> On 19/04/16 11:56, Chris Phlipot wrote:
>> The existing implentation implementation of thread__resolve_callchain,
> 
> Remove 'implentation'
> 
>> under certain circumstanes, can assemble callchain entries in the
> 
> 'circumstanes' -> 'circumstances'
> 
>> incorrect order.
>>
>> A the callchain entries are resolved incorrectly for a sample when all
>> of the following conditions are met:
>>
>> 1. callchain_param.order is set to ORDER_CALLER
>>
>> 2. thread__resolve_callchain_sample is able to resolve callchain entries
>>    for the sample.
>>
>> 3. unwind__get_entries is also able to resolve callchain entries for the
>>    sample.
>>
>> The fix is accomplished by reversing the order in which
>> thread__resolve_callchain_sample and unwind__get_entries are called
>> when callchain_param.order is set to ORDER_CALLER.
> 
> Can you give an example of the commands you used and what the call chain
> looked like before and after.
> 
> Also please run ./scripts/checkpatch.pl
> 
>>
>> Unwind specific code from thread__resolve_callchain is also moved into a
>> new static function to improve readability of the fix.
>>
>> Signed-off-by: Chris Phlipot <cphlipot0@xxxxxxxxx>
>> ---
>>  tools/perf/util/machine.c | 57 ++++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 41 insertions(+), 16 deletions(-)
>>
>> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
>> index 0c4dabc..dd086c8 100644
>> --- a/tools/perf/util/machine.c
>> +++ b/tools/perf/util/machine.c
>> @@ -1806,8 +1806,6 @@ static int thread__resolve_callchain_sample(struct thread *thread,
>>  	int skip_idx = -1;
>>  	int first_call = 0;
>>  
>> -	callchain_cursor_reset(cursor);
>> -
>>  	if (has_branch_callstack(evsel)) {
>>  		err = resolve_lbr_callchain_sample(thread, cursor, sample, parent,
>>  						   root_al, max_stack);
>> @@ -1918,20 +1916,12 @@ static int unwind_entry(struct unwind_entry *entry, void *arg)
>>  				       entry->map, entry->sym);
>>  }
>>  
>> -int thread__resolve_callchain(struct thread *thread,
>> -			      struct callchain_cursor *cursor,
>> -			      struct perf_evsel *evsel,
>> -			      struct perf_sample *sample,
>> -			      struct symbol **parent,
>> -			      struct addr_location *root_al,
>> -			      int max_stack)
>> -{
>> -	int ret = thread__resolve_callchain_sample(thread, cursor, evsel,
>> -						   sample, parent,
>> -						   root_al, max_stack);
>> -	if (ret)
>> -		return ret;
>> -
>> +static int thread__resolve_callchain_unwind(struct thread *thread,
>> +                                            struct callchain_cursor *cursor,
>> +					    struct perf_evsel *evsel,
>> +					    struct perf_sample *sample,
>> +					    int max_stack)
>> +{
>>  	/* Can we do dwarf post unwind? */
>>  	if (!((evsel->attr.sample_type & PERF_SAMPLE_REGS_USER) &&
>>  	      (evsel->attr.sample_type & PERF_SAMPLE_STACK_USER)))
>> @@ -1944,7 +1934,42 @@ int thread__resolve_callchain(struct thread *thread,
>>  
>>  	return unwind__get_entries(unwind_entry, cursor,
>>  				   thread, sample, max_stack);
>> +}
>> +
>> +int thread__resolve_callchain(struct thread *thread,
>> +                              struct callchain_cursor *cursor,
>> +                              struct perf_evsel *evsel,
>> +                              struct perf_sample *sample,
>> +                              struct symbol **parent,
>> +                              struct addr_location *root_al,
>> +                              int max_stack)
>> +{
>> +	int ret = 0;
>> +	callchain_cursor_reset(&callchain_cursor);
>> +
>> +	if (callchain_param.order == ORDER_CALLEE) {
>> +                ret = thread__resolve_callchain_sample(thread, cursor,
>> +                                                       evsel, sample,
>> +						       parent, root_al,
>> +						       max_stack);
>> +		if (ret)
>> +			return ret;
>> +                ret = thread__resolve_callchain_unwind(thread, cursor,
>> +                                                       evsel, sample,
>> +						       max_stack);
>> +	} else {
>> +                ret = thread__resolve_callchain_unwind(thread, cursor,
>> +                                                       evsel, sample,
>> +						       max_stack);
>> +		if (ret)
>> +			return ret;
>> +                ret = thread__resolve_callchain_sample(thread, cursor,
>> +                                                       evsel, sample,
>> +						       parent, root_al,
>> +						       max_stack);
>> +	}
>>  
>> +	return ret;
>>  }
>>  
>>  int machine__for_each_thread(struct machine *machine,
>>
> 
>