* [PATCH 1/2] perf probe-event: un-hardcoded sizeof(buf)
2024-04-16 4:55 [PATCH 0/2] Better error messages for 'perf probe --add' Dima Kogan
@ 2024-04-16 4:55 ` Dima Kogan
2024-04-16 4:55 ` [PATCH 2/2] perf probe-event: better error message for a too-long probe name Dima Kogan
2024-04-16 15:46 ` [PATCH 0/2] Better error messages for 'perf probe --add' Arnaldo Carvalho de Melo
2 siblings, 0 replies; 6+ messages in thread
From: Dima Kogan @ 2024-04-16 4:55 UTC (permalink / raw)
To: linux-perf-users; +Cc: Dima Kogan
In several places we had
char buf[64];
...
snprintf(buf, 64, ...);
This patch changes it to
char buf[64];
...
snprintf(buf, sizeof(buf), ...);
so the "64" is only stated once
Signed-off-by: Dima Kogan <dima@secretsauce.net>
---
tools/perf/util/probe-event.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index a1a796043691..b9f678c3be4d 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -235,7 +235,7 @@ static int convert_exec_to_group(const char *exec, char **result)
}
}
- ret = e_snprintf(buf, 64, "%s_%s", PERFPROBE_GROUP, ptr1);
+ ret = e_snprintf(buf, sizeof(buf), "%s_%s", PERFPROBE_GROUP, ptr1);
if (ret < 0)
goto out;
@@ -2867,7 +2867,7 @@ static int probe_trace_event__set_name(struct probe_trace_event *tev,
group = PERFPROBE_GROUP;
/* Get an unused new event name */
- ret = get_new_event_name(buf, 64, event, namelist,
+ ret = get_new_event_name(buf, sizeof(buf), event, namelist,
tev->point.retprobe, allow_suffix);
if (ret < 0)
return ret;
--
2.42.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] perf probe-event: better error message for a too-long probe name
2024-04-16 4:55 [PATCH 0/2] Better error messages for 'perf probe --add' Dima Kogan
2024-04-16 4:55 ` [PATCH 1/2] perf probe-event: un-hardcoded sizeof(buf) Dima Kogan
@ 2024-04-16 4:55 ` Dima Kogan
2024-04-16 15:46 ` [PATCH 0/2] Better error messages for 'perf probe --add' Arnaldo Carvalho de Melo
2 siblings, 0 replies; 6+ messages in thread
From: Dima Kogan @ 2024-04-16 4:55 UTC (permalink / raw)
To: linux-perf-users; +Cc: Dima Kogan
This is a common failure mode when probing userspace C++ code (where the
mangling adds significant length to the symbol names). Prior to this patch, only
a very generic error message is produced, making the user guess at what the
issue is
Signed-off-by: Dima Kogan <dima@secretsauce.net>
---
tools/perf/util/probe-event.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index b9f678c3be4d..84665c56a045 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2758,7 +2758,7 @@ static int get_new_event_name(char *buf, size_t len, const char *base,
/* Try no suffix number */
ret = e_snprintf(buf, len, "%s%s", nbase, ret_event ? "__return" : "");
if (ret < 0) {
- pr_debug("snprintf() failed: %d\n", ret);
+ pr_warning("snprintf() failed: %d; the event name nbase='%s' is too long\n", ret, nbase);
goto out;
}
if (!strlist__has_entry(namelist, buf))
--
2.42.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 0/2] Better error messages for 'perf probe --add'
2024-04-16 4:55 [PATCH 0/2] Better error messages for 'perf probe --add' Dima Kogan
2024-04-16 4:55 ` [PATCH 1/2] perf probe-event: un-hardcoded sizeof(buf) Dima Kogan
2024-04-16 4:55 ` [PATCH 2/2] perf probe-event: better error message for a too-long probe name Dima Kogan
@ 2024-04-16 15:46 ` Arnaldo Carvalho de Melo
2024-04-17 20:35 ` Masami Hiramatsu
2 siblings, 1 reply; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2024-04-16 15:46 UTC (permalink / raw)
To: Dima Kogan, Masami Hiramatsu; +Cc: linux-perf-users
On Mon, Apr 15, 2024 at 09:55:09PM -0700, Dima Kogan wrote:
> Hi.
>
> I tried to add some userspace probes to some C++ code, and got this:
>
> $ sudo perf probe -x tst --add _ZN.....
> Error: Failed to add events.
Both patches look great, Masami, can I have your Acked-by?
If you find other cases where the error messages can be improved, feel
free to send us, please CC Masami, as I'm doing now.
Thanks!
- Rrnaldo
> Note the completely non-useful error message. The issue is that the C++ symbol
> mangling can create very loooong symbol names, causing perf to try to use very
> long probe names by default, causing this problem. It took a little while to
> figure this out, and a better error message would have helped, hence these
> patches.
>
> To reproduce, here's a tst.cc:
>
> #include <stdio.h>
>
> namespace n1111111111111111111111111111111111111111111111111111111111111111 {
> namespace n2222222222222222222222222222222222222222222222222222222222222222 {
>
> void f(void)
> {
> printf("f()\n");
> }
> }
> }
>
> int main(void)
> {
> n1111111111111111111111111111111111111111111111111111111111111111::n2222222222222222222222222222222222222222222222222222222222222222::f();
> n1111111111111111111111111111111111111111111111111111111111111111::n2222222222222222222222222222222222222222222222222222222222222222::f();
> return 0;
> }
>
> I then
>
> g++ -g -o tst tst.cc
>
> and
>
> sudo perf probe -x ~/tmp/tst --add _ZN65n111111111111111111111111111111111111111111111111111111111111111165n22222222222222222222222222222222222222222222222222222222222222221fEv
>
> After the patch a better error message results, and the workaround is clear:
>
> sudo perf probe -x ~/tmp/tst --add probe=_ZN65n111111111111111111111111111111111111111111111111111111111111111165n22222222222222222222222222222222222222222222222222222222222222221fEv
>
> Dima Kogan (2):
> perf probe-event: un-hardcoded sizeof(buf)
> perf probe-event: better error message for a too-long probe name
>
> tools/perf/util/probe-event.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> --
> 2.42.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 0/2] Better error messages for 'perf probe --add'
2024-04-16 15:46 ` [PATCH 0/2] Better error messages for 'perf probe --add' Arnaldo Carvalho de Melo
@ 2024-04-17 20:35 ` Masami Hiramatsu
2024-04-18 14:24 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 6+ messages in thread
From: Masami Hiramatsu @ 2024-04-17 20:35 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo; +Cc: Dima Kogan, linux-perf-users
On Tue, 16 Apr 2024 12:46:23 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> On Mon, Apr 15, 2024 at 09:55:09PM -0700, Dima Kogan wrote:
> > Hi.
> >
> > I tried to add some userspace probes to some C++ code, and got this:
> >
> > $ sudo perf probe -x tst --add _ZN.....
> > Error: Failed to add events.
>
> Both patches look great, Masami, can I have your Acked-by?
>
> If you find other cases where the error messages can be improved, feel
> free to send us, please CC Masami, as I'm doing now.
Yeah, looks good to me.
Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Thank you,
>
> Thanks!
>
> - Rrnaldo
>
> > Note the completely non-useful error message. The issue is that the C++ symbol
> > mangling can create very loooong symbol names, causing perf to try to use very
> > long probe names by default, causing this problem. It took a little while to
> > figure this out, and a better error message would have helped, hence these
> > patches.
> >
> > To reproduce, here's a tst.cc:
> >
> > #include <stdio.h>
> >
> > namespace n1111111111111111111111111111111111111111111111111111111111111111 {
> > namespace n2222222222222222222222222222222222222222222222222222222222222222 {
> >
> > void f(void)
> > {
> > printf("f()\n");
> > }
> > }
> > }
> >
> > int main(void)
> > {
> > n1111111111111111111111111111111111111111111111111111111111111111::n2222222222222222222222222222222222222222222222222222222222222222::f();
> > n1111111111111111111111111111111111111111111111111111111111111111::n2222222222222222222222222222222222222222222222222222222222222222::f();
> > return 0;
> > }
> >
> > I then
> >
> > g++ -g -o tst tst.cc
> >
> > and
> >
> > sudo perf probe -x ~/tmp/tst --add _ZN65n111111111111111111111111111111111111111111111111111111111111111165n22222222222222222222222222222222222222222222222222222222222222221fEv
> >
> > After the patch a better error message results, and the workaround is clear:
> >
> > sudo perf probe -x ~/tmp/tst --add probe=_ZN65n111111111111111111111111111111111111111111111111111111111111111165n22222222222222222222222222222222222222222222222222222222222222221fEv
> >
> > Dima Kogan (2):
> > perf probe-event: un-hardcoded sizeof(buf)
> > perf probe-event: better error message for a too-long probe name
> >
> > tools/perf/util/probe-event.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > --
> > 2.42.0
> >
>
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 0/2] Better error messages for 'perf probe --add'
2024-04-17 20:35 ` Masami Hiramatsu
@ 2024-04-18 14:24 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2024-04-18 14:24 UTC (permalink / raw)
To: Masami Hiramatsu; +Cc: Dima Kogan, linux-perf-users
On Thu, Apr 18, 2024 at 05:35:37AM +0900, Masami Hiramatsu wrote:
> On Tue, 16 Apr 2024 12:46:23 -0300
> Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> > On Mon, Apr 15, 2024 at 09:55:09PM -0700, Dima Kogan wrote:
> > > Hi.
> > >
> > > I tried to add some userspace probes to some C++ code, and got this:
> > >
> > > $ sudo perf probe -x tst --add _ZN.....
> > > Error: Failed to add events.
> >
> > Both patches look great, Masami, can I have your Acked-by?
> >
> > If you find other cases where the error messages can be improved, feel
> > free to send us, please CC Masami, as I'm doing now.
>
> Yeah, looks good to me.
>
> Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Thanks, applied to perf-tools-next,
- Arnaldo
^ permalink raw reply [flat|nested] 6+ messages in thread