Re: [PATCHv2 2/8] perf probe: Improve detection of file/function name in the probe pattern
On 2015/03/13 08:20PM, Masami Hiramatsu wrote: > (2015/03/13 5:24), Arnaldo Carvalho de Melo wrote: > > Em Mon, Dec 15, 2014 at 08:20:32PM +0530, Naveen N. Rao escreveu: > >> Currently, perf probe considers patterns including a '.' to be a file. > >> However, this causes problems on powerpc ABIv1 where all functions have > >> a leading '.': > >> > >> $ perf probe -F | grep schedule_timeout_interruptible > >> .schedule_timeout_interruptible > >> $ perf probe .schedule_timeout_interruptible > >> Semantic error :File always requires line number or lazy pattern. > >> Error: Command Parse Error. > >> > >> Fix this by checking the probe pattern in more detail. > > > > Masami, can I have your Acked-by or Reviewed-by? > > As far as I can see, this is not enough for fixing that issue. > Could you fold the first half of [4/8] to this patch? > I also have some comments on it. See below. Masami, Arnaldo, Thanks for the review. v3 patches forthcoming... - Naveen -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 2/8] perf probe: Improve detection of file/function name in the probe pattern
On 2015/03/13 08:20PM, Masami Hiramatsu wrote:
> (2015/03/13 5:24), Arnaldo Carvalho de Melo wrote:
> > Em Mon, Dec 15, 2014 at 08:20:32PM +0530, Naveen N. Rao escreveu:
> >> Currently, perf probe considers patterns including a '.' to be a file.
> >> However, this causes problems on powerpc ABIv1 where all functions have
> >> a leading '.':
> >>
> >> $ perf probe -F | grep schedule_timeout_interruptible
> >> .schedule_timeout_interruptible
> >> $ perf probe .schedule_timeout_interruptible
> >> Semantic error :File always requires line number or lazy pattern.
> >> Error: Command Parse Error.
> >>
> >> Fix this by checking the probe pattern in more detail.
> >
> > Masami, can I have your Acked-by or Reviewed-by?
>
> As far as I can see, this is not enough for fixing that issue.
> Could you fold the first half of [4/8] to this patch?
Sure.
> I also have some comments on it. See below.
>
> >
> >> Signed-off-by: Naveen N. Rao
> >> ---
> >> tools/perf/util/probe-event.c | 23 ---
> >> 1 file changed, 20 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> >> index 28eb141..9943ff3 100644
> >> --- a/tools/perf/util/probe-event.c
> >> +++ b/tools/perf/util/probe-event.c
> >> @@ -999,6 +999,24 @@ static int parse_perf_probe_point(char *arg, struct
> >> perf_probe_event *pev)
> >>arg = tmp;
> >>}
> >>
> >> + /*
> >> + * Check arg is function or file name and copy it.
> >> + *
> >> + * We consider arg to be a file spec if and only if it satisfies
> >> + * all of the below criteria::
> >> + * - it does not include any of "+@%",
> >> + * - it includes one of ":;", and
> >> + * - it has a period '.' in the name.
> >> + *
> >> + * Otherwise, we consider arg to be a function specification.
> >> + */
> >> + c = 0;
>
> Oh please, don't reuse 'char c' for a boolean flag, you should
> introduce new 'bool file_loc' etc.
>
> >> + if (!strpbrk(arg, "+@%") && (ptr = strpbrk(arg, ";:")) != NULL) {
> >> + /* This is a file spec if it includes a '.' before ; or : */
> >> + if (memchr(arg, '.', ptr-arg))
> ^^ add spaces around '-'.
Sure.
- Naveen
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Re: [PATCHv2 2/8] perf probe: Improve detection of file/function name in the probe pattern
(2015/03/13 5:24), Arnaldo Carvalho de Melo wrote:
> Em Mon, Dec 15, 2014 at 08:20:32PM +0530, Naveen N. Rao escreveu:
>> Currently, perf probe considers patterns including a '.' to be a file.
>> However, this causes problems on powerpc ABIv1 where all functions have
>> a leading '.':
>>
>> $ perf probe -F | grep schedule_timeout_interruptible
>> .schedule_timeout_interruptible
>> $ perf probe .schedule_timeout_interruptible
>> Semantic error :File always requires line number or lazy pattern.
>> Error: Command Parse Error.
>>
>> Fix this by checking the probe pattern in more detail.
>
> Masami, can I have your Acked-by or Reviewed-by?
As far as I can see, this is not enough for fixing that issue.
Could you fold the first half of [4/8] to this patch?
I also have some comments on it. See below.
>
>> Signed-off-by: Naveen N. Rao
>> ---
>> tools/perf/util/probe-event.c | 23 ---
>> 1 file changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>> index 28eb141..9943ff3 100644
>> --- a/tools/perf/util/probe-event.c
>> +++ b/tools/perf/util/probe-event.c
>> @@ -999,6 +999,24 @@ static int parse_perf_probe_point(char *arg, struct
>> perf_probe_event *pev)
>> arg = tmp;
>> }
>>
>> +/*
>> + * Check arg is function or file name and copy it.
>> + *
>> + * We consider arg to be a file spec if and only if it satisfies
>> + * all of the below criteria::
>> + * - it does not include any of "+@%",
>> + * - it includes one of ":;", and
>> + * - it has a period '.' in the name.
>> + *
>> + * Otherwise, we consider arg to be a function specification.
>> + */
>> +c = 0;
Oh please, don't reuse 'char c' for a boolean flag, you should
introduce new 'bool file_loc' etc.
>> +if (!strpbrk(arg, "+@%") && (ptr = strpbrk(arg, ";:")) != NULL) {
>> +/* This is a file spec if it includes a '.' before ; or : */
>> +if (memchr(arg, '.', ptr-arg))
^^ add spaces around '-'.
Thank you,
>> +c = 1;
>> +}
>> +
>> ptr = strpbrk(arg, ";:+@%");
>> if (ptr) {
>> nc = *ptr;
>> @@ -1009,10 +1027,9 @@ static int parse_perf_probe_point(char *arg, struct
>> perf_probe_event *pev)
>> if (tmp == NULL)
>> return -ENOMEM;
>>
>> -/* Check arg is function or file and copy it */
>> -if (strchr(tmp, '.')) /* File */
>> +if (c == 1)
>> pp->file = tmp;
>> -else/* Function */
>> +else
>> pp->function = tmp;
>>
>> /* Parse other options */
>> --
>> 2.1.3
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu...@hitachi.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 2/8] perf probe: Improve detection of file/function name in the probe pattern
On Thu, Mar 12, 2015 at 05:24:14PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Dec 15, 2014 at 08:20:32PM +0530, Naveen N. Rao escreveu:
> > Currently, perf probe considers patterns including a '.' to be a file.
> > However, this causes problems on powerpc ABIv1 where all functions have
> > a leading '.':
> >
> > $ perf probe -F | grep schedule_timeout_interruptible
> > .schedule_timeout_interruptible
> > $ perf probe .schedule_timeout_interruptible
> > Semantic error :File always requires line number or lazy pattern.
> > Error: Command Parse Error.
> >
> > Fix this by checking the probe pattern in more detail.
>
> Masami, can I have your Acked-by or Reviewed-by?
Arnaldo,
FWIW, I have reviewed this code...
Reviewed-by: Ananth N Mavinakayanahalli
>
>
> - Arnaldo
>
> > Signed-off-by: Naveen N. Rao
> > ---
> > tools/perf/util/probe-event.c | 23 ---
> > 1 file changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index 28eb141..9943ff3 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -999,6 +999,24 @@ static int parse_perf_probe_point(char *arg, struct
> > perf_probe_event *pev)
> > arg = tmp;
> > }
> >
> > + /*
> > +* Check arg is function or file name and copy it.
> > +*
> > +* We consider arg to be a file spec if and only if it satisfies
> > +* all of the below criteria::
> > +* - it does not include any of "+@%",
> > +* - it includes one of ":;", and
> > +* - it has a period '.' in the name.
> > +*
> > +* Otherwise, we consider arg to be a function specification.
> > +*/
> > + c = 0;
> > + if (!strpbrk(arg, "+@%") && (ptr = strpbrk(arg, ";:")) != NULL) {
> > + /* This is a file spec if it includes a '.' before ; or : */
> > + if (memchr(arg, '.', ptr-arg))
> > + c = 1;
> > + }
> > +
> > ptr = strpbrk(arg, ";:+@%");
> > if (ptr) {
> > nc = *ptr;
> > @@ -1009,10 +1027,9 @@ static int parse_perf_probe_point(char *arg, struct
> > perf_probe_event *pev)
> > if (tmp == NULL)
> > return -ENOMEM;
> >
> > - /* Check arg is function or file and copy it */
> > - if (strchr(tmp, '.')) /* File */
> > + if (c == 1)
> > pp->file = tmp;
> > - else/* Function */
> > + else
> > pp->function = tmp;
> >
> > /* Parse other options */
> > --
> > 2.1.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 2/8] perf probe: Improve detection of file/function name in the probe pattern
Em Thu, Mar 12, 2015 at 05:24:14PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Dec 15, 2014 at 08:20:32PM +0530, Naveen N. Rao escreveu:
> > Currently, perf probe considers patterns including a '.' to be a file.
> > However, this causes problems on powerpc ABIv1 where all functions have
> > a leading '.':
> >
> > $ perf probe -F | grep schedule_timeout_interruptible
> > .schedule_timeout_interruptible
> > $ perf probe .schedule_timeout_interruptible
> > Semantic error :File always requires line number or lazy pattern.
> > Error: Command Parse Error.
> >
> > Fix this by checking the probe pattern in more detail.
>
> Masami, can I have your Acked-by or Reviewed-by?
It is limited to powerpc, but I would even so be happy if you could look
at it,
Thanks,
- Arnaldo
> > Signed-off-by: Naveen N. Rao
> > ---
> > tools/perf/util/probe-event.c | 23 ---
> > 1 file changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index 28eb141..9943ff3 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -999,6 +999,24 @@ static int parse_perf_probe_point(char *arg, struct
> > perf_probe_event *pev)
> > arg = tmp;
> > }
> >
> > + /*
> > +* Check arg is function or file name and copy it.
> > +*
> > +* We consider arg to be a file spec if and only if it satisfies
> > +* all of the below criteria::
> > +* - it does not include any of "+@%",
> > +* - it includes one of ":;", and
> > +* - it has a period '.' in the name.
> > +*
> > +* Otherwise, we consider arg to be a function specification.
> > +*/
> > + c = 0;
> > + if (!strpbrk(arg, "+@%") && (ptr = strpbrk(arg, ";:")) != NULL) {
> > + /* This is a file spec if it includes a '.' before ; or : */
> > + if (memchr(arg, '.', ptr-arg))
> > + c = 1;
> > + }
> > +
> > ptr = strpbrk(arg, ";:+@%");
> > if (ptr) {
> > nc = *ptr;
> > @@ -1009,10 +1027,9 @@ static int parse_perf_probe_point(char *arg, struct
> > perf_probe_event *pev)
> > if (tmp == NULL)
> > return -ENOMEM;
> >
> > - /* Check arg is function or file and copy it */
> > - if (strchr(tmp, '.')) /* File */
> > + if (c == 1)
> > pp->file = tmp;
> > - else/* Function */
> > + else
> > pp->function = tmp;
> >
> > /* Parse other options */
> > --
> > 2.1.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 2/8] perf probe: Improve detection of file/function name in the probe pattern
Em Mon, Dec 15, 2014 at 08:20:32PM +0530, Naveen N. Rao escreveu:
> Currently, perf probe considers patterns including a '.' to be a file.
> However, this causes problems on powerpc ABIv1 where all functions have
> a leading '.':
>
> $ perf probe -F | grep schedule_timeout_interruptible
> .schedule_timeout_interruptible
> $ perf probe .schedule_timeout_interruptible
> Semantic error :File always requires line number or lazy pattern.
> Error: Command Parse Error.
>
> Fix this by checking the probe pattern in more detail.
Masami, can I have your Acked-by or Reviewed-by?
- Arnaldo
> Signed-off-by: Naveen N. Rao
> ---
> tools/perf/util/probe-event.c | 23 ---
> 1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 28eb141..9943ff3 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -999,6 +999,24 @@ static int parse_perf_probe_point(char *arg, struct
> perf_probe_event *pev)
> arg = tmp;
> }
>
> + /*
> + * Check arg is function or file name and copy it.
> + *
> + * We consider arg to be a file spec if and only if it satisfies
> + * all of the below criteria::
> + * - it does not include any of "+@%",
> + * - it includes one of ":;", and
> + * - it has a period '.' in the name.
> + *
> + * Otherwise, we consider arg to be a function specification.
> + */
> + c = 0;
> + if (!strpbrk(arg, "+@%") && (ptr = strpbrk(arg, ";:")) != NULL) {
> + /* This is a file spec if it includes a '.' before ; or : */
> + if (memchr(arg, '.', ptr-arg))
> + c = 1;
> + }
> +
> ptr = strpbrk(arg, ";:+@%");
> if (ptr) {
> nc = *ptr;
> @@ -1009,10 +1027,9 @@ static int parse_perf_probe_point(char *arg, struct
> perf_probe_event *pev)
> if (tmp == NULL)
> return -ENOMEM;
>
> - /* Check arg is function or file and copy it */
> - if (strchr(tmp, '.')) /* File */
> + if (c == 1)
> pp->file = tmp;
> - else/* Function */
> + else
> pp->function = tmp;
>
> /* Parse other options */
> --
> 2.1.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCHv2 2/8] perf probe: Improve detection of file/function name in the probe pattern
Currently, perf probe considers patterns including a '.' to be a file.
However, this causes problems on powerpc ABIv1 where all functions have
a leading '.':
$ perf probe -F | grep schedule_timeout_interruptible
.schedule_timeout_interruptible
$ perf probe .schedule_timeout_interruptible
Semantic error :File always requires line number or lazy pattern.
Error: Command Parse Error.
Fix this by checking the probe pattern in more detail.
Signed-off-by: Naveen N. Rao
---
tools/perf/util/probe-event.c | 23 ---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 28eb141..9943ff3 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -999,6 +999,24 @@ static int parse_perf_probe_point(char *arg, struct
perf_probe_event *pev)
arg = tmp;
}
+ /*
+* Check arg is function or file name and copy it.
+*
+* We consider arg to be a file spec if and only if it satisfies
+* all of the below criteria::
+* - it does not include any of "+@%",
+* - it includes one of ":;", and
+* - it has a period '.' in the name.
+*
+* Otherwise, we consider arg to be a function specification.
+*/
+ c = 0;
+ if (!strpbrk(arg, "+@%") && (ptr = strpbrk(arg, ";:")) != NULL) {
+ /* This is a file spec if it includes a '.' before ; or : */
+ if (memchr(arg, '.', ptr-arg))
+ c = 1;
+ }
+
ptr = strpbrk(arg, ";:+@%");
if (ptr) {
nc = *ptr;
@@ -1009,10 +1027,9 @@ static int parse_perf_probe_point(char *arg, struct
perf_probe_event *pev)
if (tmp == NULL)
return -ENOMEM;
- /* Check arg is function or file and copy it */
- if (strchr(tmp, '.')) /* File */
+ if (c == 1)
pp->file = tmp;
- else/* Function */
+ else
pp->function = tmp;
/* Parse other options */
--
2.1.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
