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.bango...@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
