Prev: [PATCH 2/5] Package Level Thermal Control and Power Limit Notification: pkgtemp hwmon driver
Next: xstat: Add a pair of system calls to make extended file stats available [ver #6]
From: Gleb Natapov on 22 Jul 2010 12:40
On Thu, Jul 22, 2010 at 11:33:45AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jul 22, 2010 at 10:20:44AM +0300, Gleb Natapov escreveu:
> > strtoull() returns valid number when it gets line with label and following
> > test passes too. I can't think of a way to unambiguously distinguish between
> > label and valid rip. May be running objdump with --prefix-addresses will
> > help, but it may make other thing unambiguous.
>
> [root(a)emilia ~]# objdump --start-address=0x0000000000400474 --stop-address=0x0000000000400496 -dS ./a.out | grep -P ':\t'
> 400474: 55 push %rbp
> 400475: 48 89 e5 mov %rsp,%rbp
> 400478: 89 7d ec mov %edi,-0x14(%rbp)
> 40047b: 48 89 75 e0 mov %rsi,-0x20(%rbp)
> 40047f: eb 01 jmp 400482 <main+0xe>
> 400481: 90 nop
> 400482: 83 45 fc 01 addl $0x1,-0x4(%rbp)
> 400486: 81 7d fc 80 96 98 00 cmpl $0x989680,-0x4(%rbp)
> 40048d: 75 f2 jne 400481 <main+0xd>
> 40048f: 90 nop
> 400490: b8 00 00 00 00 mov $0x0,%eax
> 400495: c9 leaveq
> [root(a)emilia ~]# objdump --start-address=0x0000000000400474
> --stop-address=0x0000000000400496 -dS ./a.out | grep ':$'
> Disassembly of section .text:
> 0000000000400474 <main>:
> add:
> [root(a)emilia ~]#
>
> Can you try the attached patch?
>
I can, only later. But if I underhand your patch correctly perf will
still crash if there is a comment after the label on the same line.

> With it we get:
>
>
> [root(a)emilia ~]# perf annotate
>
> ------------------------------------------------
> Percent | Source code & Disassembly of a.out
> ------------------------------------------------
> :
> :
> :
> : Disassembly of section .text:
> :
> : 0000000000400474 <main>:
> : int main(int argc, char **argv)
> : {
> 0.00 : 400474: 55 push %rbp
> 0.00 : 400475: 48 89 e5 mov %rsp,%rbp
> 0.00 : 400478: 89 7d ec mov %edi,-0x14(%rbp)
> 0.00 : 40047b: 48 89 75 e0 mov %rsi,-0x20(%rbp)
> 0.00 : 40047f: eb 01 jmp 400482 <main+0xe>
> :
> : while(1) {
> : i++;
> : if (i == 10000000)
> : goto add;
> : }
> 21.05 : 400481: 90 nop
> : int main(int argc, char **argv)
> : {
> : int i;
> :
> : while(1) {
> : i++;
> 0.00 : 400482: 83 45 fc 01 addl $0x1,-0x4(%rbp)
> : if (i == 10000000)
> 15.79 : 400486: 81 7d fc 80 96 98 00 cmpl $0x989680,-0x4(%rbp)
> 63.16 : 40048d: 75 f2 jne 400481 <main+0xd>
> : goto add;
> 0.00 : 40048f: 90 nop
> : }
> : add:
> : return 0;
> 0.00 : 400490: b8 00 00 00 00 mov $0x0,%eax
> : }
> 0.00 : 400495: c9 leaveq

> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 699cf81..e3486d5 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -976,7 +976,7 @@ static int hist_entry__parse_objdump_line(struct hist_entry *self, FILE *file,
> * Parse hexa addresses followed by ':'
> */
> line_ip = strtoull(tmp, &tmp2, 16);
> - if (*tmp2 != ':' || tmp == tmp2)
> + if (*tmp2 != ':' || tmp == tmp2 || tmp2[1] == '\0')
> line_ip = -1;
> }
>


--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Arnaldo Carvalho de Melo on 22 Jul 2010 12:50
Em Thu, Jul 22, 2010 at 07:38:51PM +0300, Gleb Natapov escreveu:
> On Thu, Jul 22, 2010 at 11:33:45AM -0300, Arnaldo Carvalho de Melo wrote:
> > Can you try the attached patch?
> >
> I can, only later. But if I underhand your patch correctly perf will
> still crash if there is a comment after the label on the same line.

Possibly, will try that now.

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Arnaldo Carvalho de Melo on 22 Jul 2010 13:00
Em Thu, Jul 22, 2010 at 01:47:11PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, Jul 22, 2010 at 07:38:51PM +0300, Gleb Natapov escreveu:
> > On Thu, Jul 22, 2010 at 11:33:45AM -0300, Arnaldo Carvalho de Melo wrote:
> > > Can you try the attached patch?
> > >
> > I can, only later. But if I underhand your patch correctly perf will
> > still crash if there is a comment after the label on the same line.
>
> Possibly, will try that now.

It could be a comment of play code, like:

[root(a)emilia ~]# cat test.c
int main(int argc, char **argv)
{
int i;

while(1) {
i++;
if (i == 10000000)
goto add;
}
add: return 0;
}
[root(a)emilia ~]#

back to the drawing board :-\

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Arnaldo Carvalho de Melo on 22 Jul 2010 13:10
Em Thu, Jul 22, 2010 at 01:52:22PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, Jul 22, 2010 at 01:47:11PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Thu, Jul 22, 2010 at 07:38:51PM +0300, Gleb Natapov escreveu:
> > > On Thu, Jul 22, 2010 at 11:33:45AM -0300, Arnaldo Carvalho de Melo wrote:
> > > > Can you try the attached patch?
> > > >
> > > I can, only later. But if I underhand your patch correctly perf will
> > > still crash if there is a comment after the label on the same line.
> >
> > Possibly, will try that now.
>
> It could be a comment of play code, like:
>
> [root(a)emilia ~]# cat test.c
> int main(int argc, char **argv)
> {
> int i;
>
> while(1) {
> i++;
> if (i == 10000000)
> goto add;
> }
> add: return 0;
> }
> [root(a)emilia ~]#
>
> back to the drawing board :-\

What about this one?

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 699cf81..96e9044 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -976,13 +976,15 @@ static int hist_entry__parse_objdump_line(struct hist_entry *self, FILE *file,
* Parse hexa addresses followed by ':'
*/
line_ip = strtoull(tmp, &tmp2, 16);
- if (*tmp2 != ':' || tmp == tmp2)
+ if (*tmp2 != ':' || tmp == tmp2 || tmp2[1] == '\0')
line_ip = -1;
}

if (line_ip != -1) {
u64 start = map__rip_2objdump(self->ms.map, sym->start);
offset = line_ip - start;
+ if (offset < 0 || (u64)line_ip > sym->end)
+ offset = -1;
}

objdump_line = objdump_line__new(offset, line);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Arnaldo Carvalho de Melo on 22 Jul 2010 15:20
Em Thu, Jul 22, 2010 at 09:05:38PM +0300, Gleb Natapov escreveu:
> On Thu, Jul 22, 2010 at 02:05:42PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Thu, Jul 22, 2010 at 01:52:22PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Thu, Jul 22, 2010 at 01:47:11PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > It could be a comment of play code, like:

> > > while(1) {
> > > if (++i == 10000000)
> > > goto add;
> > > }
> > > add: return 0;

> > What about this one?

> > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > @@ -976,13 +976,15 @@ static int hist_entry__parse_objdump_line(struct hist_entry *self, FILE *file,
> > if (line_ip != -1) {
> > u64 start = map__rip_2objdump(self->ms.map, sym->start);
> > offset = line_ip - start;
> > + if (offset < 0 || (u64)line_ip > sym->end)
> > + offset = -1;

> This part is good idea anyway. Even if label will be interpreted as ip
> perf at least will not crash. It may miss-report something if check will
> accidentally succeed though.

Yeah, we can possibly find a label which is a valid hex number and that
falls inside the address range, but with what we have in objdump this
seems to be the best we can have, I'll commit this.

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: [PATCH 2/5] Package Level Thermal Control and Power Limit Notification: pkgtemp hwmon driver
Next: xstat: Add a pair of system calls to make extended file stats available [ver #6]