Re: [PATCH] Properly interpret indirect call in perf annotate.

[Martin Liška]

On 08/27/2018 12:37 PM, Namhyung Kim wrote:
> Hello,
> 
> On Thu, Aug 23, 2018 at 02:29:34PM +0200, Martin Liška wrote:
>> The patch changes interpretation of:
>> callq  *0x8(%rbx)
>>
>> from:
>>   0.26 │     → callq  *8
>> to:
>>   0.26 │     → callq  *0x8(%rbx)
>>
>> in this can an address is followed by a register, thus
>> one can't parse only address.
> 
> Also there's a case with no offset like:  callq  *%rbx

Yes. But this case is fine as strtoull returns 0 for that:
'If there were no digits at all, strtoul() stores the original value of nptr in 
*endptr (and returns 0).'
So ops->target.addr is then 0 and it's fine.

> 
> 
>>
>> Signed-off-by: Martin Liška <mli...@suse.cz>
>> ---
>>  tools/perf/util/annotate.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>>
> 
>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> index e4268b948e0e..e32ead4744bd 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
>> @@ -246,8 +246,14 @@ static int call__parse(struct arch *arch, struct 
>> ins_operands *ops, struct map_s
>>  
>>  indirect_call:
>>      tok = strchr(endptr, '*');
>> -    if (tok != NULL)
>> -            ops->target.addr = strtoull(tok + 1, NULL, 16);
>> +    if (tok != NULL) {
>> +            endptr++;
>> +
>> +            /* Indirect call can use a non-rip register and offset: callq  
>> *0x8(%rbx).
>> +             * Do not parse such instruction.  */
>> +            if (strstr(endptr, "(%r") == NULL)
>> +                    ops->target.addr = strtoull(endptr, NULL, 16);
> 
> It seems too x86-specific, what about this? (not tested)

It is, I'm fine with that. I've just tested that for the callq  *0x8(%rbx) 
example.
I'm sending patch for that version.

Martin

> 
> 
> indirect_call:
>       tok = strchr(endptr, '*');
>       if (tok != NULL) {
>               endptr++;
>               if (!isdigit(*endptr))
>                       return 0;
> 
>               addr = strtoull(endptr, &endptr, 0);
>               if (*endptr != '('))
>                       ops->target.addr = addr;
> 
> 
> Thanks,
> Namhyung
> 
> 
>> +    }
>>      goto find_target;
>>  }
>>  
>>
> 


>From 58a0eca544be8cc9e15b2ab5ecd9d9401ff4d2ec Mon Sep 17 00:00:00 2001
From: marxin <mli...@suse.cz>
Date: Thu, 23 Aug 2018 14:25:33 +0200
Subject: [PATCH] Properly interpret indirect call in perf annotate.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The patch changes interpretation of:
callq  *0x8(%rbx)

from:
  0.26 │     → callq  *8
to:
  0.26 │     → callq  *0x8(%rbx)

in this can an address is followed by a register, thus
one can't parse only address.

Signed-off-by: Martin Liška <mli...@suse.cz>
---
 tools/perf/util/annotate.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index e4268b948e0e..18a8477d4664 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -212,6 +212,7 @@ static int call__parse(struct arch *arch, struct ins_operands *ops, struct map_s
 	struct addr_map_symbol target = {
 		.map = map,
 	};
+  u64 addr;
 
 	ops->target.addr = strtoull(ops->raw, &endptr, 16);
 
@@ -246,8 +247,15 @@ static int call__parse(struct arch *arch, struct ins_operands *ops, struct map_s
 
 indirect_call:
 	tok = strchr(endptr, '*');
-	if (tok != NULL)
-		ops->target.addr = strtoull(tok + 1, NULL, 16);
+	if (tok != NULL) {
+		endptr++;
+		if (!isdigit(*endptr))
+			return 0;
+
+		addr = strtoull(endptr, &endptr, 0);
+		if (*endptr != '(')
+			ops->target.addr = addr;
+	}
 	goto find_target;
 }
 
-- 
2.18.0