[PATCH] perf annotate: Fix objdump comment parsing for Intel mov dissassembly

[PATCH] perf annotate: Fix objdump comment parsing for Intel mov dissassembly

[Thomas Richter]

The command 'perf annotate' parses the output of objdump and also
investigates the comments produced by objdump. For example the
output of objdump produces (on x86):

23eee:  4c 8b 3d 13 01 21 00 mov 0x210113(%rip),%r15
                                # 234008 <stderr@@GLIBC_2.2.5+0x9a8>

and the function mov__parse() is called to investigate the complete
line. Mov__parse() breaks this line into several parts and finally
calls function comment__symbol() to parse the data after the comment
character '#'. Comment__symbol() expects a hexadecimal address followed
by a symbol in '<' and '>' brackets.

However the 2nd parameter given to function comment__symbol()
always points to the comment character '#'. The address parsing
always returns 0 because the character '#' is not a digit and
strtoull() fails without being noticed.

Fix this by advancing the second parameter to function comment__symbol()
by one byte before invocation and add an error check after strtoull()
has been called.

Signed-off-by: Thomas Richter <tmricht@linux.vnet.ibm.com>
Reviewed-by: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
---
 tools/perf/util/annotate.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index aa66791b1bfc..900147454fe9 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -323,6 +323,8 @@ static int comment__symbol(char *raw, char *comment, u64 *addrp, char **namep)
 		return 0;
 
 	*addrp = strtoull(comment, &endptr, 16);
+	if (endptr == comment)
+		return 0;
 	name = strchr(endptr, '<');
 	if (name == NULL)
 		return -1;
@@ -436,8 +438,8 @@ static int mov__parse(struct arch *arch, struct ins_operands *ops, struct map *m
 		return 0;
 
 	comment = ltrim(comment);
-	comment__symbol(ops->source.raw, comment, &ops->source.addr, &ops->source.name);
-	comment__symbol(ops->target.raw, comment, &ops->target.addr, &ops->target.name);
+	comment__symbol(ops->source.raw, comment + 1, &ops->source.addr, &ops->source.name);
+	comment__symbol(ops->target.raw, comment + 1, &ops->target.addr, &ops->target.name);
 
 	return 0;
 
@@ -481,7 +483,7 @@ static int dec__parse(struct arch *arch __maybe_unused, struct ins_operands *ops
 		return 0;
 
 	comment = ltrim(comment);
-	comment__symbol(ops->target.raw, comment, &ops->target.addr, &ops->target.name);
+	comment__symbol(ops->target.raw, comment + 1, &ops->target.addr, &ops->target.name);
 
 	return 0;
 }
-- 
2.13.4

Re: [PATCH] perf annotate: Fix objdump comment parsing for Intel mov dissassembly

[Ravi Bangoria]

On 11/28/2017 01:26 PM, Thomas Richter wrote:
> The command 'perf annotate' parses the output of objdump and also
> investigates the comments produced by objdump. For example the
> output of objdump produces (on x86):
>
> 23eee:  4c 8b 3d 13 01 21 00 mov 0x210113(%rip),%r15
>                                 # 234008 <stderr@@GLIBC_2.2.5+0x9a8>
>
> and the function mov__parse() is called to investigate the complete
> line. Mov__parse() breaks this line into several parts and finally
> calls function comment__symbol() to parse the data after the comment
> character '#'. Comment__symbol() expects a hexadecimal address followed
> by a symbol in '<' and '>' brackets.
>
> However the 2nd parameter given to function comment__symbol()
> always points to the comment character '#'. The address parsing
> always returns 0 because the character '#' is not a digit and
> strtoull() fails without being noticed.
>
> Fix this by advancing the second parameter to function comment__symbol()
> by one byte before invocation and add an error check after strtoull()
> has been called.

Yeah, looks like it fails to get correct value in 'addrp'.

Can you please show the difference in perf annotate output before
and after patch.

Thanks,
Ravi

Re: [PATCH] perf annotate: Fix objdump comment parsing for Intel mov dissassembly

[Thomas-Mich Richter]

On 11/29/2017 02:24 PM, Ravi Bangoria wrote:
> 
> 
> On 11/28/2017 01:26 PM, Thomas Richter wrote:
>> The command 'perf annotate' parses the output of objdump and also
>> investigates the comments produced by objdump. For example the
>> output of objdump produces (on x86):
>>
>> 23eee:  4c 8b 3d 13 01 21 00 mov 0x210113(%rip),%r15
>>                                 # 234008 <stderr@@GLIBC_2.2.5+0x9a8>
>>
>> and the function mov__parse() is called to investigate the complete
>> line. Mov__parse() breaks this line into several parts and finally
>> calls function comment__symbol() to parse the data after the comment
>> character '#'. Comment__symbol() expects a hexadecimal address followed
>> by a symbol in '<' and '>' brackets.
>>
>> However the 2nd parameter given to function comment__symbol()
>> always points to the comment character '#'. The address parsing
>> always returns 0 because the character '#' is not a digit and
>> strtoull() fails without being noticed.
>>
>> Fix this by advancing the second parameter to function comment__symbol()
>> by one byte before invocation and add an error check after strtoull()
>> has been called.
> 
> Yeah, looks like it fails to get correct value in 'addrp'.
> 
> Can you please show the difference in perf annotate output before
> and after patch.
> 
> Thanks,
> Ravi
> 


There is no difference in output of --stdio. The adress value is not
read and remains 0x0 in ops->source.addr or ops->target.addr.
That is not visible because in function mov__scnprintf() that wrong
address is not printed:

static int mov__scnprintf(struct ins *ins, char *bf, size_t size,
                           struct ins_operands *ops)
{
        return scnprintf(bf, size, "%-6.6s %s,%s", ins->name,
                         ops->source.name ?: ops->source.raw,
                         ops->target.name ?: ops->target.raw);
}


-- 
Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH] perf annotate: Fix objdump comment parsing for Intel mov dissassembly

[Ravi Bangoria]

On 11/29/2017 08:33 PM, Thomas-Mich Richter wrote:
> On 11/29/2017 02:24 PM, Ravi Bangoria wrote:
>>
>> On 11/28/2017 01:26 PM, Thomas Richter wrote:
>>> The command 'perf annotate' parses the output of objdump and also
>>> investigates the comments produced by objdump. For example the
>>> output of objdump produces (on x86):
>>>
>>> 23eee:  4c 8b 3d 13 01 21 00 mov 0x210113(%rip),%r15
>>>                                 # 234008 <stderr@@GLIBC_2.2.5+0x9a8>
>>>
>>> and the function mov__parse() is called to investigate the complete
>>> line. Mov__parse() breaks this line into several parts and finally
>>> calls function comment__symbol() to parse the data after the comment
>>> character '#'. Comment__symbol() expects a hexadecimal address followed
>>> by a symbol in '<' and '>' brackets.
>>>
>>> However the 2nd parameter given to function comment__symbol()
>>> always points to the comment character '#'. The address parsing
>>> always returns 0 because the character '#' is not a digit and
>>> strtoull() fails without being noticed.
>>>
>>> Fix this by advancing the second parameter to function comment__symbol()
>>> by one byte before invocation and add an error check after strtoull()
>>> has been called.
>> Yeah, looks like it fails to get correct value in 'addrp'.
>>
>> Can you please show the difference in perf annotate output before
>> and after patch.
>>
>> Thanks,
>> Ravi
>>
>
> There is no difference in output of --stdio. The adress value is not
> read and remains 0x0 in ops->source.addr or ops->target.addr.
> That is not visible because in function mov__scnprintf() that wrong
> address is not printed:
>
> static int mov__scnprintf(struct ins *ins, char *bf, size_t size,
>                            struct ins_operands *ops)
> {
>         return scnprintf(bf, size, "%-6.6s %s,%s", ins->name,
>                          ops->source.name ?: ops->source.raw,
>                          ops->target.name ?: ops->target.raw);
> }

Looks good.  Ack-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>

Thanks,
Ravi

Re: [PATCH] perf annotate: Fix objdump comment parsing for Intel mov dissassembly

[Arnaldo Carvalho de Melo]

Em Wed, Nov 29, 2017 at 08:52:13PM +0530, Ravi Bangoria escreveu:
> 
> 
> On 11/29/2017 08:33 PM, Thomas-Mich Richter wrote:
> > On 11/29/2017 02:24 PM, Ravi Bangoria wrote:
> >>
> >> On 11/28/2017 01:26 PM, Thomas Richter wrote:
> >>> The command 'perf annotate' parses the output of objdump and also
> >>> investigates the comments produced by objdump. For example the
> >>> output of objdump produces (on x86):
> >>>
> >>> 23eee:  4c 8b 3d 13 01 21 00 mov 0x210113(%rip),%r15
> >>>                                 # 234008 <stderr@@GLIBC_2.2.5+0x9a8>
> >>>
> >>> and the function mov__parse() is called to investigate the complete
> >>> line. Mov__parse() breaks this line into several parts and finally
> >>> calls function comment__symbol() to parse the data after the comment
> >>> character '#'. Comment__symbol() expects a hexadecimal address followed
> >>> by a symbol in '<' and '>' brackets.
> >>>
> >>> However the 2nd parameter given to function comment__symbol()
> >>> always points to the comment character '#'. The address parsing
> >>> always returns 0 because the character '#' is not a digit and
> >>> strtoull() fails without being noticed.
> >>>
> >>> Fix this by advancing the second parameter to function comment__symbol()
> >>> by one byte before invocation and add an error check after strtoull()
> >>> has been called.
> >> Yeah, looks like it fails to get correct value in 'addrp'.
> >>
> >> Can you please show the difference in perf annotate output before
> >> and after patch.
> >>
> >> Thanks,
> >> Ravi
> >>
> >
> > There is no difference in output of --stdio. The adress value is not
> > read and remains 0x0 in ops->source.addr or ops->target.addr.
> > That is not visible because in function mov__scnprintf() that wrong
> > address is not printed:
> >
> > static int mov__scnprintf(struct ins *ins, char *bf, size_t size,
> >                            struct ins_operands *ops)
> > {
> >         return scnprintf(bf, size, "%-6.6s %s,%s", ins->name,
> >                          ops->source.name ?: ops->source.raw,
> >                          ops->target.name ?: ops->target.raw);
> > }
> 
> Looks good.  Ack-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>

Thanks, applied.

- Arnaldo