Re: [PATCH v7 5/6] perf annotate: Fix jump target outside of function address range

[Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>]

On Wednesday 05 October 2016 05:01 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Sep 21, 2016 at 09:17:55PM +0530, Ravi Bangoria escreveu:
>> If jump target is outside of function range, perf is not handling it
>> correctly. Especially when target address is lesser than function start
>> address, target offset will be negative. But, target address declared
>> to be unsigned, converts negative number into 2's complement. See below
>> example. Here target of 'jumpq' instruction at 34cf8 is 34ac0 which is
>> lesser than function start address(34cf0).
>>
>>         34ac0 - 34cf0 = -0x230 = 0xfffffffffffffdd0
> This one looks ok, but isn't applying.

This is applying fine for me on perf/core. Which branch are you trying?

-Ravi

>
> - Arnaldo
>
>> Objdump output:
>>
>>   0000000000034cf0 <__sigaction>:
>>   __GI___sigaction():
>>     34cf0: lea    -0x20(%rdi),%eax
>>     34cf3: cmp    -bashx1,%eax
>>     34cf6: jbe    34d00 <__sigaction+0x10>
>>     34cf8: jmpq   34ac0 <__GI___libc_sigaction>
>>     34cfd: nopl   (%rax)
>>     34d00: mov    0x386161(%rip),%rax        # 3bae68 <_DYNAMIC+0x2e8>
>>     34d07: movl   -bashx16,%fs:(%rax)
>>     34d0e: mov    -bashxffffffff,%eax
>>     34d13: retq
>>
>> perf annotate before applying patch:
>>
>>   __GI___sigaction  /usr/lib64/libc-2.22.so
>>            lea    -0x20(%rdi),%eax
>>            cmp    -bashx1,%eax
>>         v  jbe    10
>>         v  jmpq   fffffffffffffdd0
>>            nop
>>     10:    mov    _DYNAMIC+0x2e8,%rax
>>            movl   -bashx16,%fs:(%rax)
>>            mov    -bashxffffffff,%eax
>>            retq
>>
>> perf annotate after applying patch:
>>
>>   __GI___sigaction  /usr/lib64/libc-2.22.so
>>            lea    -0x20(%rdi),%eax
>>            cmp    -bashx1,%eax
>>         v  jbe    10
>>         ^  jmpq   34ac0 <__GI___libc_sigaction>
>>            nop
>>     10:    mov    _DYNAMIC+0x2e8,%rax
>>            movl   -bashx16,%fs:(%rax)
>>            mov    -bashxffffffff,%eax
>>            retq
>>
>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
>> ---
>> Changes in v7:
>>   - No changes
>>
>>  tools/perf/ui/browsers/annotate.c |  5 +++--
>>  tools/perf/util/annotate.c        | 14 +++++++++-----
>>  tools/perf/util/annotate.h        |  5 +++--
>>  3 files changed, 15 insertions(+), 9 deletions(-)
>>
>> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
>> index 214a14a..2d04bdf 100644
>> --- a/tools/perf/ui/browsers/annotate.c
>> +++ b/tools/perf/ui/browsers/annotate.c
>> @@ -215,7 +215,7 @@ static void annotate_browser__write(struct ui_browser *browser, void *entry, int
>>  			ui_browser__set_color(browser, color);
>>  		if (dl->ins && dl->ins->ops->scnprintf) {
>>  			if (ins__is_jump(dl->ins)) {
>> -				bool fwd = dl->ops.target.offset > (u64)dl->offset;
>> +				bool fwd = dl->ops.target.offset > dl->offset;
>>  
>>  				ui_browser__write_graph(browser, fwd ? SLSMG_DARROW_CHAR :
>>  								    SLSMG_UARROW_CHAR);
>> @@ -245,7 +245,8 @@ static bool disasm_line__is_valid_jump(struct disasm_line *dl, struct symbol *sy
>>  {
>>  	if (!dl || !dl->ins || !ins__is_jump(dl->ins)
>>  	    || !disasm_line__has_offset(dl)
>> -	    || dl->ops.target.offset >= symbol__size(sym))
>> +	    || dl->ops.target.offset < 0
>> +	    || dl->ops.target.offset >= (s64)symbol__size(sym))
>>  		return false;
>>  
>>  	return true;
>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> index a9dbac1..fc44dd1 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
>> @@ -129,10 +129,12 @@ static int jump__parse(struct ins_operands *ops, struct map *map __maybe_unused)
>>  	else
>>  		ops->target.addr = strtoull(ops->raw, NULL, 16);
>>  
>> -	if (s++ != NULL)
>> +	if (s++ != NULL) {
>>  		ops->target.offset = strtoull(s, NULL, 16);
>> -	else
>> -		ops->target.offset = UINT64_MAX;
>> +		ops->target.offset_avail = true;
>> +	} else {
>> +		ops->target.offset_avail = false;
>> +	}
>>  
>>  	return 0;
>>  }
>> @@ -140,7 +142,7 @@ static int jump__parse(struct ins_operands *ops, struct map *map __maybe_unused)
>>  static int jump__scnprintf(struct ins *ins, char *bf, size_t size,
>>  			   struct ins_operands *ops)
>>  {
>> -	if (!ops->target.addr)
>> +	if (!ops->target.addr || ops->target.offset < 0)
>>  		return ins__raw_scnprintf(ins, bf, size, ops);
>>  
>>  	return scnprintf(bf, size, "%-6.6s %" PRIx64, ins->name, ops->target.offset);
>> @@ -1373,9 +1375,11 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
>>  	if (dl == NULL)
>>  		return -1;
>>  
>> -	if (dl->ops.target.offset == UINT64_MAX)
>> +	if (!disasm_line__has_offset(dl)) {
>>  		dl->ops.target.offset = dl->ops.target.addr -
>>  					map__rip_2objdump(map, sym->start);
>> +		dl->ops.target.offset_avail = true;
>> +	}
>>  
>>  	/* kcore has no symbols, so add the call target name */
>>  	if (dl->ins && ins__is_call(dl->ins) && !dl->ops.target.name) {
>> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
>> index 4400269..7ba3579 100644
>> --- a/tools/perf/util/annotate.h
>> +++ b/tools/perf/util/annotate.h
>> @@ -19,7 +19,8 @@ struct ins_operands {
>>  		char	*raw;
>>  		char	*name;
>>  		u64	addr;
>> -		u64	offset;
>> +		s64	offset;
>> +		bool    offset_avail;
>>  	} target;
>>  	union {
>>  		struct {
>> @@ -67,7 +68,7 @@ struct disasm_line {
>>  
>>  static inline bool disasm_line__has_offset(const struct disasm_line *dl)
>>  {
>> -	return dl->ops.target.offset != UINT64_MAX;
>> +	return dl->ops.target.offset_avail;
>>  }
>>  
>>  void disasm_line__free(struct disasm_line *dl);
>> -- 
>> 2.5.5