| Message ID | 1441982552-28028-8-git-send-email-tim.gardner@canonical.com |
|---|---|
| State | New |
| Headers | show |
On 11.09.2015 16:42, tim.gardner@canonical.com wrote: > From: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> > > BugLink: http://bugs.launchpad.net/bugs/1485528 > > 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, and > - skipping leading dot if one exists when creating/deleting events. From the whole set I find that one hard to estimate without knowing more about general specs. Will the format on all arches require a ";" or ":" and none of "+@%" to differentiate a file from a function specification? If that is the case then the change looks ok. The remainder of patches always seem to insert new (weak) functions with the non ppc64el path being the same as before. So those seem ok. -Stefan > > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com> > Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> > Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com> > Cc: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> > Cc: linuxppc-dev@lists.ozlabs.org > Link: http://lkml.kernel.org/r/db680f7cb11c4452b632f908e67151f3aa0f4602.1430217967.git.naveen.n.rao@linux.vnet.ibm.com > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> > (cherry picked from commit 3099c026002e97b8c173d9d0bbdfc39257d14402) > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > --- > tools/perf/util/probe-event.c | 29 ++++++++++++++++++++++++++--- > 1 file changed, 26 insertions(+), 3 deletions(-) > > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c > index 5bd5029..69b6f21 100644 > --- a/tools/perf/util/probe-event.c > +++ b/tools/perf/util/probe-event.c > @@ -973,6 +973,7 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev) > struct perf_probe_point *pp = &pev->point; > char *ptr, *tmp; > char c, nc = 0; > + bool file_spec = false; > /* > * <Syntax> > * perf probe [EVENT=]SRC[:LN|;PTN] > @@ -1001,6 +1002,23 @@ 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. > + */ > + if (!strpbrk(arg, "+@%") && (ptr = strpbrk(arg, ";:")) != NULL) { > + /* This is a file spec if it includes a '.' before ; or : */ > + if (memchr(arg, '.', ptr - arg)) > + file_spec = true; > + } > + > ptr = strpbrk(arg, ";:+@%"); > if (ptr) { > nc = *ptr; > @@ -1011,10 +1029,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 (file_spec) > pp->file = tmp; > - else /* Function */ > + else > pp->function = tmp; > > /* Parse other options */ > @@ -2067,6 +2084,9 @@ static int get_new_event_name(char *buf, size_t len, const char *base, > { > int i, ret; > > + if (*base == '.') > + base++; > + > /* Try no suffix */ > ret = e_snprintf(buf, len, "%s", base); > if (ret < 0) { > @@ -2536,6 +2556,9 @@ int del_perf_probe_events(struct strlist *dellist) > event = str; > } > > + if (event && *event == '.') > + event++; > + > ret = e_snprintf(buf, 128, "%s:%s", group, event); > if (ret < 0) { > pr_err("Failed to copy event."); >
On 09/15/2015 02:03 AM, Stefan Bader wrote: > On 11.09.2015 16:42, tim.gardner@canonical.com wrote: >> From: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> >> >> BugLink: http://bugs.launchpad.net/bugs/1485528 >> >> 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, and >> - skipping leading dot if one exists when creating/deleting events. > > From the whole set I find that one hard to estimate without knowing more about > general specs. Will the format on all arches require a ";" or ":" and none of > "+@%" to differentiate a file from a function specification? If that is the case > then the change looks ok. > > The remainder of patches always seem to insert new (weak) functions with the non > ppc64el path being the same as before. So those seem ok. > > -Stefan > According to comments in the code, all arches have the same syntax with the exception of whether a function begins with a '.' (on ppc64el). As far as I can tell its doing the right thing. Also, this commit was merged as of 4.2-rc1 and I don't see any corrections to it up through 4.3-rc1. I'm comfortable that if this had caused a regression then it would have been noticed by now. rtg
On 15.09.2015 18:05, Tim Gardner wrote: > On 09/15/2015 02:03 AM, Stefan Bader wrote: >> On 11.09.2015 16:42, tim.gardner@canonical.com wrote: >>> From: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> >>> >>> BugLink: http://bugs.launchpad.net/bugs/1485528 >>> >>> 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, and >>> - skipping leading dot if one exists when creating/deleting events. >> >> From the whole set I find that one hard to estimate without knowing more about >> general specs. Will the format on all arches require a ";" or ":" and none of >> "+@%" to differentiate a file from a function specification? If that is the case >> then the change looks ok. >> >> The remainder of patches always seem to insert new (weak) functions with the non >> ppc64el path being the same as before. So those seem ok. >> >> -Stefan >> > > According to comments in the code, all arches have the same syntax with the > exception of whether a function begins with a '.' (on ppc64el). As far as I can > tell its doing the right thing. Also, this commit was merged as of 4.2-rc1 and I > don't see any corrections to it up through 4.3-rc1. I'm comfortable that if this > had caused a regression then it would have been noticed by now. So ACK then > > rtg
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c index 5bd5029..69b6f21 100644 --- a/tools/perf/util/probe-event.c +++ b/tools/perf/util/probe-event.c @@ -973,6 +973,7 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev) struct perf_probe_point *pp = &pev->point; char *ptr, *tmp; char c, nc = 0; + bool file_spec = false; /* * <Syntax> * perf probe [EVENT=]SRC[:LN|;PTN] @@ -1001,6 +1002,23 @@ 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. + */ + if (!strpbrk(arg, "+@%") && (ptr = strpbrk(arg, ";:")) != NULL) { + /* This is a file spec if it includes a '.' before ; or : */ + if (memchr(arg, '.', ptr - arg)) + file_spec = true; + } + ptr = strpbrk(arg, ";:+@%"); if (ptr) { nc = *ptr; @@ -1011,10 +1029,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 (file_spec) pp->file = tmp; - else /* Function */ + else pp->function = tmp; /* Parse other options */ @@ -2067,6 +2084,9 @@ static int get_new_event_name(char *buf, size_t len, const char *base, { int i, ret; + if (*base == '.') + base++; + /* Try no suffix */ ret = e_snprintf(buf, len, "%s", base); if (ret < 0) { @@ -2536,6 +2556,9 @@ int del_perf_probe_events(struct strlist *dellist) event = str; } + if (event && *event == '.') + event++; + ret = e_snprintf(buf, 128, "%s:%s", group, event); if (ret < 0) { pr_err("Failed to copy event.");