[PATCH 0/2] Better error messages for 'perf probe --add'

[PATCH 0/2] Better error messages for 'perf probe --add'

[Dima Kogan]

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.

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

[PATCH 1/2] perf probe-event: un-hardcoded sizeof(buf)

[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

[PATCH 2/2] perf probe-event: better error message for a too-long probe name

[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

Re: [PATCH 0/2] Better error messages for 'perf probe --add'

[Arnaldo Carvalho de Melo]

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
> 

Re: [PATCH 0/2] Better error messages for 'perf probe --add'

[Masami Hiramatsu]

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>

Re: [PATCH 0/2] Better error messages for 'perf probe --add'

[Arnaldo Carvalho de Melo]

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