[PATCH v1 0/3] perf probe: Support long symbol

[PATCH v1 0/3] perf probe: Support long symbol

[Leo Yan]

Currently, a probe supports event name maximum to a 64-byte length. The
event name comes from the probed symbol name, otherwise, user can
specify an event name.

In the case when user tries to inject a probe for a long symbol, e.g.
mangled symbol name in a C++ program, the kernel buffer cannot
accommodate it if the symbol name is longer than 64 bytes.

On the other hand, this series relies on the perf tool to resolve the
issue. When the tool detects user doesn't specify event name and the
probed symbol is longer than 64 bytes, it will generate a hashed event
name with 64-byte length to avoid failure.


Leo Yan (3):
  perf: Dynamically allocate buffer for event string
  perf probe: Check group string length
  perf probe: Generate hash event for long symbol

 tools/perf/util/probe-event.c | 71 ++++++++++++++++++++++++++++++++---
 1 file changed, 65 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH v1 1/3] perf: Dynamically allocate buffer for event string

[Leo Yan]

Dynamically allocate and free buffer to support event name.

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 tools/perf/util/probe-event.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index a17c9b8a7a79..cad11d95af4f 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2834,6 +2834,9 @@ static void warn_uprobe_event_compat(struct probe_trace_event *tev)
 	free(buf);
 }
 
+/* Defined in kernel/trace/trace.h */
+#define MAX_EVENT_NAME_LEN	64
+
 /* Set new name from original perf_probe_event and namelist */
 static int probe_trace_event__set_name(struct probe_trace_event *tev,
 				       struct perf_probe_event *pev,
@@ -2841,9 +2844,13 @@ static int probe_trace_event__set_name(struct probe_trace_event *tev,
 				       bool allow_suffix)
 {
 	const char *event, *group;
-	char buf[64];
+	char *buf;
 	int ret;
 
+	buf = malloc(MAX_EVENT_NAME_LEN);
+	if (!buf)
+		return -ENOMEM;
+
 	/* If probe_event or trace_event already have the name, reuse it */
 	if (pev->event && !pev->sdt)
 		event = pev->event;
@@ -2866,17 +2873,19 @@ 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, sizeof(buf), event, namelist,
+	ret = get_new_event_name(buf, MAX_EVENT_NAME_LEN, event, namelist,
 				 tev->point.retprobe, allow_suffix);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	event = buf;
 
 	tev->event = strdup(event);
 	tev->group = strdup(group);
-	if (tev->event == NULL || tev->group == NULL)
-		return -ENOMEM;
+	if (tev->event == NULL || tev->group == NULL) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	/*
 	 * Add new event name to namelist if multiprobe event is NOT
@@ -2885,7 +2894,10 @@ static int probe_trace_event__set_name(struct probe_trace_event *tev,
 	 */
 	if (!multiprobe_event_is_supported())
 		strlist__add(namelist, event);
-	return 0;
+
+out:
+	free(buf);
+	return ret < 0 ? ret : 0;
 }
 
 static int __open_probe_file_and_namelist(bool uprobe,
-- 
2.34.1

[PATCH v1 2/3] perf probe: Check group string length

[Leo Yan]

In the kernel, the probe group string length is limited up to
MAX_EVENT_NAME_LEN (including the NULL terminator).

Check for this limitation and report an error if it is exceeded.

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 tools/perf/util/probe-event.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index cad11d95af4f..71acea07cb46 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2872,6 +2872,13 @@ static int probe_trace_event__set_name(struct probe_trace_event *tev,
 	else
 		group = PERFPROBE_GROUP;
 
+	if (strlen(group) >= MAX_EVENT_NAME_LEN) {
+		pr_err("Probe group string='%s' is too long (>= %d bytes)\n",
+			group, MAX_EVENT_NAME_LEN);
+		ret = -ENOMEM;
+		goto out;
+	}
+
 	/* Get an unused new event name */
 	ret = get_new_event_name(buf, MAX_EVENT_NAME_LEN, event, namelist,
 				 tev->point.retprobe, allow_suffix);
-- 
2.34.1

[PATCH v1 3/3] perf probe: Generate hash event for long symbol

[Leo Yan]

If a symbol name is longer than the maximum event length (64 bytes),
generate an new event name with below combination:

  TruncatedSymbol + '_' + HashString + '__return' + '\0'
    `> 46B        + 1B  +   8B       +    8B      + 1B   = 64 Bytes.

With this change, a probe can be injected for long symbol.

Before:

  # nm test_cpp_mangle | grep -E "print_data|Point"
  0000000000000cac t _GLOBAL__sub_I__Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi
  0000000000000b50 T _Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzR5Point
  0000000000000b14 T _Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi

  # perf probe -x test_cpp_mangle --add \
        "_Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi"
  snprintf() failed: -7; the event name nbase='_Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi' is too long
  Error: Failed to add events.

After:

  # perf probe -x test_cpp_mangle --add \
	"_Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi"

  Probe event='_Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi' is too long (>= 64 bytes).
  Generate hashed event name='_Z62this_is_a_very_very_long_print_data_abcdef_91f40679'

  Added new event:
    probe_test_cpp_mangle: _Z62this_is_a_very_very_long_print_data_abcdef_91f40679
    (on _Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi in /mnt/test_cpp_mangle)

  You can now use it in all perf tools, such as:

      perf record -e probe_test_cpp_mangle: _Z62this_is_a_very_very_long_print_data_abcdef_91f40679 -aR sleep 1

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 tools/perf/util/probe-event.c | 42 ++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 71acea07cb46..bacd29b95c75 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2837,6 +2837,32 @@ static void warn_uprobe_event_compat(struct probe_trace_event *tev)
 /* Defined in kernel/trace/trace.h */
 #define MAX_EVENT_NAME_LEN	64
 
+static char *probe_trace_event__hash_event(const char *event)
+{
+	char *str = NULL;
+	size_t hash;
+
+	str = malloc(MAX_EVENT_NAME_LEN);
+	if (!str)
+		return NULL;
+
+	hash = str_hash(event);
+
+	/*
+	 * Reserve characters for the "__return" suffix for the return probe.
+	 * Thus the string buffer (64 bytes) are used for:
+	 *   Truncated event:  46 bytes
+	 *   '_'            :   1 byte
+	 *   hash string    :   8 bytes
+	 *   reserved       :   8 bytes (for suffix "__return")
+	 *   '\0'           :   1 byte
+	 */
+	strncpy(str, event, 46);
+	/* '_' + hash string + '\0' */
+	snprintf(str + 46, 10, "_%lx", hash);
+	return str;
+}
+
 /* Set new name from original perf_probe_event and namelist */
 static int probe_trace_event__set_name(struct probe_trace_event *tev,
 				       struct perf_probe_event *pev,
@@ -2844,7 +2870,7 @@ static int probe_trace_event__set_name(struct probe_trace_event *tev,
 				       bool allow_suffix)
 {
 	const char *event, *group;
-	char *buf;
+	char *buf, *hash_event = NULL;
 	int ret;
 
 	buf = malloc(MAX_EVENT_NAME_LEN);
@@ -2864,6 +2890,19 @@ static int probe_trace_event__set_name(struct probe_trace_event *tev,
 			event = pev->point.function;
 		else
 			event = tev->point.realname;
+
+		if (strlen(event) >= MAX_EVENT_NAME_LEN) {
+			pr_warning("Probe event='%s' is too long (>= %d bytes).\n",
+				   event, MAX_EVENT_NAME_LEN);
+
+			hash_event = probe_trace_event__hash_event(event);
+			if (!hash_event) {
+				ret = -ENOMEM;
+				goto out;
+			}
+			pr_warning("Generate hashed event name='%s'\n", hash_event);
+			event = hash_event;
+		}
 	}
 	if (pev->group && !pev->sdt)
 		group = pev->group;
@@ -2903,6 +2942,7 @@ static int probe_trace_event__set_name(struct probe_trace_event *tev,
 		strlist__add(namelist, event);
 
 out:
+	free(hash_event);
 	free(buf);
 	return ret < 0 ? ret : 0;
 }
-- 
2.34.1

Re: [PATCH v1 0/3] perf probe: Support long symbol

[Namhyung Kim]

Hello Leo,

On Mon, Oct 07, 2024 at 03:11:13PM +0100, Leo Yan wrote:
> Currently, a probe supports event name maximum to a 64-byte length. The
> event name comes from the probed symbol name, otherwise, user can
> specify an event name.
> 
> In the case when user tries to inject a probe for a long symbol, e.g.
> mangled symbol name in a C++ program, the kernel buffer cannot
> accommodate it if the symbol name is longer than 64 bytes.
> 
> On the other hand, this series relies on the perf tool to resolve the
> issue. When the tool detects user doesn't specify event name and the
> probed symbol is longer than 64 bytes, it will generate a hashed event
> name with 64-byte length to avoid failure.

Please CC Masami for probe related changes in the future.

Thanks,
Namhyung

> 
> 
> Leo Yan (3):
>   perf: Dynamically allocate buffer for event string
>   perf probe: Check group string length
>   perf probe: Generate hash event for long symbol
> 
>  tools/perf/util/probe-event.c | 71 ++++++++++++++++++++++++++++++++---
>  1 file changed, 65 insertions(+), 6 deletions(-)
> 
> -- 
> 2.34.1
> 

Re: [PATCH v1 0/3] perf probe: Support long symbol

[Masami Hiramatsu]

On Wed, 9 Oct 2024 18:12:52 -0700
Namhyung Kim <namhyung@kernel.org> wrote:

> Hello Leo,
> 
> On Mon, Oct 07, 2024 at 03:11:13PM +0100, Leo Yan wrote:
> > Currently, a probe supports event name maximum to a 64-byte length. The
> > event name comes from the probed symbol name, otherwise, user can
> > specify an event name.
> > 
> > In the case when user tries to inject a probe for a long symbol, e.g.
> > mangled symbol name in a C++ program, the kernel buffer cannot
> > accommodate it if the symbol name is longer than 64 bytes.
> > 
> > On the other hand, this series relies on the perf tool to resolve the
> > issue. When the tool detects user doesn't specify event name and the
> > probed symbol is longer than 64 bytes, it will generate a hashed event
> > name with 64-byte length to avoid failure.
> 
> Please CC Masami for probe related changes in the future.

Thanks Namhyung,

Let me find the patches.

> 
> Thanks,
> Namhyung
> 
> > 
> > 
> > Leo Yan (3):
> >   perf: Dynamically allocate buffer for event string
> >   perf probe: Check group string length
> >   perf probe: Generate hash event for long symbol
> > 
> >  tools/perf/util/probe-event.c | 71 ++++++++++++++++++++++++++++++++---
> >  1 file changed, 65 insertions(+), 6 deletions(-)
> > 
> > -- 
> > 2.34.1
> > 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v1 1/3] perf: Dynamically allocate buffer for event string

[Masami Hiramatsu]

On Mon,  7 Oct 2024 15:11:14 +0100
Leo Yan <leo.yan@arm.com> wrote:

> Dynamically allocate and free buffer to support event name.

Sorry, but I don't see any benefit from this patch. This looks making
code more complex.

Thank you,

> 
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>  tools/perf/util/probe-event.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index a17c9b8a7a79..cad11d95af4f 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -2834,6 +2834,9 @@ static void warn_uprobe_event_compat(struct probe_trace_event *tev)
>  	free(buf);
>  }
>  
> +/* Defined in kernel/trace/trace.h */
> +#define MAX_EVENT_NAME_LEN	64
> +
>  /* Set new name from original perf_probe_event and namelist */
>  static int probe_trace_event__set_name(struct probe_trace_event *tev,
>  				       struct perf_probe_event *pev,
> @@ -2841,9 +2844,13 @@ static int probe_trace_event__set_name(struct probe_trace_event *tev,
>  				       bool allow_suffix)
>  {
>  	const char *event, *group;
> -	char buf[64];
> +	char *buf;
>  	int ret;
>  
> +	buf = malloc(MAX_EVENT_NAME_LEN);
> +	if (!buf)
> +		return -ENOMEM;
> +
>  	/* If probe_event or trace_event already have the name, reuse it */
>  	if (pev->event && !pev->sdt)
>  		event = pev->event;
> @@ -2866,17 +2873,19 @@ 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, sizeof(buf), event, namelist,
> +	ret = get_new_event_name(buf, MAX_EVENT_NAME_LEN, event, namelist,
>  				 tev->point.retprobe, allow_suffix);
>  	if (ret < 0)
> -		return ret;
> +		goto out;
>  
>  	event = buf;
>  
>  	tev->event = strdup(event);
>  	tev->group = strdup(group);
> -	if (tev->event == NULL || tev->group == NULL)
> -		return -ENOMEM;
> +	if (tev->event == NULL || tev->group == NULL) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
>  
>  	/*
>  	 * Add new event name to namelist if multiprobe event is NOT
> @@ -2885,7 +2894,10 @@ static int probe_trace_event__set_name(struct probe_trace_event *tev,
>  	 */
>  	if (!multiprobe_event_is_supported())
>  		strlist__add(namelist, event);
> -	return 0;
> +
> +out:
> +	free(buf);
> +	return ret < 0 ? ret : 0;
>  }
>  
>  static int __open_probe_file_and_namelist(bool uprobe,
> -- 
> 2.34.1
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v1 2/3] perf probe: Check group string length

[Masami Hiramatsu]

On Mon,  7 Oct 2024 15:11:15 +0100
Leo Yan <leo.yan@arm.com> wrote:

> In the kernel, the probe group string length is limited up to
> MAX_EVENT_NAME_LEN (including the NULL terminator).

This looks good to me.

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thank you,

> 
> Check for this limitation and report an error if it is exceeded.
> 
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>  tools/perf/util/probe-event.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index cad11d95af4f..71acea07cb46 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -2872,6 +2872,13 @@ static int probe_trace_event__set_name(struct probe_trace_event *tev,
>  	else
>  		group = PERFPROBE_GROUP;
>  
> +	if (strlen(group) >= MAX_EVENT_NAME_LEN) {
> +		pr_err("Probe group string='%s' is too long (>= %d bytes)\n",
> +			group, MAX_EVENT_NAME_LEN);
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
>  	/* Get an unused new event name */
>  	ret = get_new_event_name(buf, MAX_EVENT_NAME_LEN, event, namelist,
>  				 tev->point.retprobe, allow_suffix);
> -- 
> 2.34.1
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v1 3/3] perf probe: Generate hash event for long symbol

[Masami Hiramatsu]

On Mon,  7 Oct 2024 15:11:16 +0100
Leo Yan <leo.yan@arm.com> wrote:

> If a symbol name is longer than the maximum event length (64 bytes),
> generate an new event name with below combination:
> 
>   TruncatedSymbol + '_' + HashString + '__return' + '\0'
>     `> 46B        + 1B  +   8B       +    8B      + 1B   = 64 Bytes.
> 
> With this change, a probe can be injected for long symbol.
> 
> Before:
> 
>   # nm test_cpp_mangle | grep -E "print_data|Point"
>   0000000000000cac t _GLOBAL__sub_I__Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi
>   0000000000000b50 T _Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzR5Point
>   0000000000000b14 T _Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi
> 
>   # perf probe -x test_cpp_mangle --add \
>         "_Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi"
>   snprintf() failed: -7; the event name nbase='_Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi' is too long
>   Error: Failed to add events.
> 
> After:
> 
>   # perf probe -x test_cpp_mangle --add \
> 	"_Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi"
> 
>   Probe event='_Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi' is too long (>= 64 bytes).
>   Generate hashed event name='_Z62this_is_a_very_very_long_print_data_abcdef_91f40679'
> 
>   Added new event:
>     probe_test_cpp_mangle: _Z62this_is_a_very_very_long_print_data_abcdef_91f40679
>     (on _Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi in /mnt/test_cpp_mangle)
> 
>   You can now use it in all perf tools, such as:
> 
>       perf record -e probe_test_cpp_mangle: _Z62this_is_a_very_very_long_print_data_abcdef_91f40679 -aR sleep 1

OK, personally, I recommend you to specify event name instead of generating
long event name in this case. But I understand sometimes this kind of feature
is good for someone.

BTW, I would like to confirm. Can't we demangle the symbol name and parse it?

Thank you,

> 
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>  tools/perf/util/probe-event.c | 42 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 71acea07cb46..bacd29b95c75 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -2837,6 +2837,32 @@ static void warn_uprobe_event_compat(struct probe_trace_event *tev)
>  /* Defined in kernel/trace/trace.h */
>  #define MAX_EVENT_NAME_LEN	64
>  
> +static char *probe_trace_event__hash_event(const char *event)
> +{
> +	char *str = NULL;
> +	size_t hash;
> +
> +	str = malloc(MAX_EVENT_NAME_LEN);
> +	if (!str)
> +		return NULL;
> +
> +	hash = str_hash(event);
> +
> +	/*
> +	 * Reserve characters for the "__return" suffix for the return probe.
> +	 * Thus the string buffer (64 bytes) are used for:
> +	 *   Truncated event:  46 bytes
> +	 *   '_'            :   1 byte
> +	 *   hash string    :   8 bytes
> +	 *   reserved       :   8 bytes (for suffix "__return")
> +	 *   '\0'           :   1 byte
> +	 */
> +	strncpy(str, event, 46);
> +	/* '_' + hash string + '\0' */
> +	snprintf(str + 46, 10, "_%lx", hash);
> +	return str;
> +}
> +
>  /* Set new name from original perf_probe_event and namelist */
>  static int probe_trace_event__set_name(struct probe_trace_event *tev,
>  				       struct perf_probe_event *pev,
> @@ -2844,7 +2870,7 @@ static int probe_trace_event__set_name(struct probe_trace_event *tev,
>  				       bool allow_suffix)
>  {
>  	const char *event, *group;
> -	char *buf;
> +	char *buf, *hash_event = NULL;
>  	int ret;
>  
>  	buf = malloc(MAX_EVENT_NAME_LEN);
> @@ -2864,6 +2890,19 @@ static int probe_trace_event__set_name(struct probe_trace_event *tev,
>  			event = pev->point.function;
>  		else
>  			event = tev->point.realname;
> +
> +		if (strlen(event) >= MAX_EVENT_NAME_LEN) {
> +			pr_warning("Probe event='%s' is too long (>= %d bytes).\n",
> +				   event, MAX_EVENT_NAME_LEN);
> +
> +			hash_event = probe_trace_event__hash_event(event);
> +			if (!hash_event) {
> +				ret = -ENOMEM;
> +				goto out;
> +			}
> +			pr_warning("Generate hashed event name='%s'\n", hash_event);
> +			event = hash_event;
> +		}
>  	}
>  	if (pev->group && !pev->sdt)
>  		group = pev->group;
> @@ -2903,6 +2942,7 @@ static int probe_trace_event__set_name(struct probe_trace_event *tev,
>  		strlist__add(namelist, event);
>  
>  out:
> +	free(hash_event);
>  	free(buf);
>  	return ret < 0 ? ret : 0;
>  }
> -- 
> 2.34.1
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v1 3/3] perf probe: Generate hash event for long symbol

[Leo Yan]

Hi Masami,

On 10/10/24 16:34, Masami Hiramatsu (Google) wrote:
> 
> 
> On Mon,  7 Oct 2024 15:11:16 +0100
> Leo Yan <leo.yan@arm.com> wrote:
> 
>> If a symbol name is longer than the maximum event length (64 bytes),
>> generate an new event name with below combination:
>>
>>    TruncatedSymbol + '_' + HashString + '__return' + '\0'
>>      `> 46B        + 1B  +   8B       +    8B      + 1B   = 64 Bytes.
>>
>> With this change, a probe can be injected for long symbol.
>>
>> Before:
>>
>>    # nm test_cpp_mangle | grep -E "print_data|Point"
>>    0000000000000cac t _GLOBAL__sub_I__Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi
>>    0000000000000b50 T _Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzR5Point
>>    0000000000000b14 T _Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi
>>
>>    # perf probe -x test_cpp_mangle --add \
>>          "_Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi"
>>    snprintf() failed: -7; the event name nbase='_Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi' is too long
>>    Error: Failed to add events.
>>
>> After:
>>
>>    # perf probe -x test_cpp_mangle --add \
>>        "_Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi"
>>
>>    Probe event='_Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi' is too long (>= 64 bytes).
>>    Generate hashed event name='_Z62this_is_a_very_very_long_print_data_abcdef_91f40679'
>>
>>    Added new event:
>>      probe_test_cpp_mangle: _Z62this_is_a_very_very_long_print_data_abcdef_91f40679
>>      (on _Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi in /mnt/test_cpp_mangle)
>>
>>    You can now use it in all perf tools, such as:
>>
>>        perf record -e probe_test_cpp_mangle: _Z62this_is_a_very_very_long_print_data_abcdef_91f40679 -aR sleep 1
> 
> OK, personally, I recommend you to specify event name instead of generating
> long event name in this case. But I understand sometimes this kind of feature
> is good for someone.

Sometimes, users try to add probe for long symbol and returns error, but there 
  have no clue for proceeding.

Either we automatically generate a hashed name, or a guidance in the failure 
log for setting event name would be helpful. If you have concern for hashed 
name, maybe we can refine the log to give info for setting event name?

> BTW, I would like to confirm. Can't we demangle the symbol name and parse it?

I did test for C++ demangle symbols with the command:

   perf probe -x /mnt/test_cpp_mangle -F --demangle

The command doesn't work as I cannot see it output correctly for demangled 
symbols. But I don't look into details why this does not work at my side.

Thanks for review.

Leo

Re: [PATCH v1 3/3] perf probe: Generate hash event for long symbol

[Masami Hiramatsu]

On Thu, 10 Oct 2024 16:53:05 +0100
Leo Yan <leo.yan@arm.com> wrote:

> Hi Masami,
> 
> On 10/10/24 16:34, Masami Hiramatsu (Google) wrote:
> > 
> > 
> > On Mon,  7 Oct 2024 15:11:16 +0100
> > Leo Yan <leo.yan@arm.com> wrote:
> > 
> >> If a symbol name is longer than the maximum event length (64 bytes),
> >> generate an new event name with below combination:
> >>
> >>    TruncatedSymbol + '_' + HashString + '__return' + '\0'
> >>      `> 46B        + 1B  +   8B       +    8B      + 1B   = 64 Bytes.
> >>
> >> With this change, a probe can be injected for long symbol.
> >>
> >> Before:
> >>
> >>    # nm test_cpp_mangle | grep -E "print_data|Point"
> >>    0000000000000cac t _GLOBAL__sub_I__Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi
> >>    0000000000000b50 T _Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzR5Point
> >>    0000000000000b14 T _Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi
> >>
> >>    # perf probe -x test_cpp_mangle --add \
> >>          "_Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi"
> >>    snprintf() failed: -7; the event name nbase='_Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi' is too long
> >>    Error: Failed to add events.
> >>
> >> After:
> >>
> >>    # perf probe -x test_cpp_mangle --add \
> >>        "_Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi"
> >>
> >>    Probe event='_Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi' is too long (>= 64 bytes).
> >>    Generate hashed event name='_Z62this_is_a_very_very_long_print_data_abcdef_91f40679'
> >>
> >>    Added new event:
> >>      probe_test_cpp_mangle: _Z62this_is_a_very_very_long_print_data_abcdef_91f40679
> >>      (on _Z62this_is_a_very_very_long_print_data_abcdefghijklmnopqrstuvwxyzi in /mnt/test_cpp_mangle)
> >>
> >>    You can now use it in all perf tools, such as:
> >>
> >>        perf record -e probe_test_cpp_mangle: _Z62this_is_a_very_very_long_print_data_abcdef_91f40679 -aR sleep 1
> > 
> > OK, personally, I recommend you to specify event name instead of generating
> > long event name in this case. But I understand sometimes this kind of feature
> > is good for someone.
> 
> Sometimes, users try to add probe for long symbol and returns error, but there 
>   have no clue for proceeding.

OK, no warning messsage is not good.
It should warn them to recommend adding it with their own event name too.

> Either we automatically generate a hashed name, or a guidance in the failure 
> log for setting event name would be helpful. If you have concern for hashed 
> name, maybe we can refine the log to give info for setting event name?

Yeah, I think this long event name is not useful for user to type.

> > BTW, I would like to confirm. Can't we demangle the symbol name and parse it?
> 
> I did test for C++ demangle symbols with the command:
> 
>    perf probe -x /mnt/test_cpp_mangle -F --demangle
> 
> The command doesn't work as I cannot see it output correctly for demangled 
> symbols. But I don't look into details why this does not work at my side.

Oops, that is another issue to be solved.

Thank you,

> 
> Thanks for review.
> 
> Leo


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v1 3/3] perf probe: Generate hash event for long symbol

[Leo Yan]

On 10/11/2024 4:07 AM, Masami Hiramatsu (Google) wrote:

[...]

>>> OK, personally, I recommend you to specify event name instead of generating
>>> long event name in this case. But I understand sometimes this kind of feature
>>> is good for someone.
>>
>> Sometimes, users try to add probe for long symbol and returns error, but there
>>   have no clue for proceeding.
> 
> OK, no warning messsage is not good.
> It should warn them to recommend adding it with their own event name too.

Okay, will do this in next spin.

>> Either we automatically generate a hashed name, or a guidance in the failure
>> log for setting event name would be helpful. If you have concern for hashed
>> name, maybe we can refine the log to give info for setting event name?
> 
> Yeah, I think this long event name is not useful for user to type.

Agreed.

>>> BTW, I would like to confirm. Can't we demangle the symbol name and parse it?
>>
>> I did test for C++ demangle symbols with the command:
>>
>>    perf probe -x /mnt/test_cpp_mangle -F --demangle
>>
>> The command doesn't work as I cannot see it output correctly for demangled
>> symbols. But I don't look into details why this does not work at my side.
> 
> Oops, that is another issue to be solved.

After install libiberty, then I can see the tool can show demangled symbols.
However, I found another issue for probing demangle symbol, please see details
in below patch and let me know if makes sense.

---8<---

From 3b09a6f89c7e383c6b1d2b7e6bd80c6bfa658d5b Mon Sep 17 00:00:00 2001
From: Leo Yan <leo.yan@arm.com>
Date: Fri, 11 Oct 2024 07:58:08 +0000
Subject: [PATCH] perf probe: Correct demangled symbols in C++ program

An issue can be observed when probe C++ demangled symbol with steps:

  # nm test_cpp_mangle | grep print_data
    0000000000000c94 t _GLOBAL__sub_I__Z10print_datai
    0000000000000afc T _Z10print_datai
    0000000000000b38 T _Z10print_dataR5Point

  # ./perf probe -x /home/niayan01/test_cpp_mangle -F --demangle
    ...
    print_data(Point&)
    print_data(int)
    ...

  # ./perf --debug verbose=3 probe -x test_cpp_mangle --add "test=print_data(int)"
    probe-definition(0): test=print_data(int)
    symbol:print_data(int) file:(null) line:0 offset:0 return:0 lazy:(null)
    0 arguments
    Open Debuginfo file: /home/niayan01/test_cpp_mangle
    Try to find probe point from debuginfo.
    Symbol print_data(int) address found : afc
    Matched function: print_data [2ccf]
    Probe point found: print_data+0
    Found 1 probe_trace_events.
    Opening /sys/kernel/tracing//uprobe_events write=1
    Opening /sys/kernel/tracing//README write=0
    Writing event: p:probe_test_cpp_mangle/test /home/niayan01/test_cpp_mangle:0xb38
    ...

When tried to probe symbol "print_data(int)", the log show:

    Symbol print_data(int) address found : afc

The found address is 0xafc - which is right if we connect with the
output result from nm. Afterwards when write event, the command uses
offset 0xb38 in the last log, which is a wrong address.

The dwarf_diename() gets a common function name, in above case, it
returns string "print_data". As a result, the tool parses the offset
based on the common name. This leads to probe at the wrong symbol
"print_data(Point&)".

To fix the issue, use the die_get_linkage_name() function to retrieve
the distinct linkage name - this is the mangled name for the C++ case.
Based on this unique name, the tool can get a correct offset for
probing. Based on DWARF doc, it is possible the linkage name is missed
in the DIE, it rolls back to use dwarf_diename().

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 tools/perf/util/probe-finder.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index 630e16c54ed5..28a14005fc1f 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -1583,8 +1583,21 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,

        /* Find a corresponding function (name, baseline and baseaddr) */
        if (die_find_realfunc(&cudie, (Dwarf_Addr)addr, &spdie)) {
-               /* Get function entry information */
-               func = basefunc = dwarf_diename(&spdie);
+               /*
+                * Get function entry information.
+                *
+                * As described in the document DWARF Debugging Information
+                * Format Version 5, section 2.22 Linkage Names, "mangled names,
+                * are used in various ways, ... to distinguish multiple
+                * entities that have the same name".
+                *
+                * Firstly try to get distinct linkage name, if fail then
+                * rollback to get associated name in DIE.
+                */
+               func = basefunc = die_get_linkage_name(&spdie);
+               if (!func)
+                        func = basefunc = dwarf_diename(&spdie);
+
                if (!func ||
                    die_entrypc(&spdie, &baseaddr) != 0 ||
                    dwarf_decl_line(&spdie, &baseline) != 0) {
--
2.25.1

Re: [PATCH v1 3/3] perf probe: Generate hash event for long symbol

[Masami Hiramatsu]

On Fri, 11 Oct 2024 09:41:39 +0100
Leo Yan <leo.yan@arm.com> wrote:

> On 10/11/2024 4:07 AM, Masami Hiramatsu (Google) wrote:
> 
> [...]
> 
> >>> OK, personally, I recommend you to specify event name instead of generating
> >>> long event name in this case. But I understand sometimes this kind of feature
> >>> is good for someone.
> >>
> >> Sometimes, users try to add probe for long symbol and returns error, but there
> >>   have no clue for proceeding.
> > 
> > OK, no warning messsage is not good.
> > It should warn them to recommend adding it with their own event name too.
> 
> Okay, will do this in next spin.
> 
> >> Either we automatically generate a hashed name, or a guidance in the failure
> >> log for setting event name would be helpful. If you have concern for hashed
> >> name, maybe we can refine the log to give info for setting event name?
> > 
> > Yeah, I think this long event name is not useful for user to type.
> 
> Agreed.
> 
> >>> BTW, I would like to confirm. Can't we demangle the symbol name and parse it?
> >>
> >> I did test for C++ demangle symbols with the command:
> >>
> >>    perf probe -x /mnt/test_cpp_mangle -F --demangle
> >>
> >> The command doesn't work as I cannot see it output correctly for demangled
> >> symbols. But I don't look into details why this does not work at my side.
> > 
> > Oops, that is another issue to be solved.
> 
> After install libiberty, then I can see the tool can show demangled symbols.
> However, I found another issue for probing demangle symbol, please see details
> in below patch and let me know if makes sense.

Yeah, I think that will fixes a mangled symbol problem.
But I have one comment below.

> 
> ---8<---
> 
> From 3b09a6f89c7e383c6b1d2b7e6bd80c6bfa658d5b Mon Sep 17 00:00:00 2001
> From: Leo Yan <leo.yan@arm.com>
> Date: Fri, 11 Oct 2024 07:58:08 +0000
> Subject: [PATCH] perf probe: Correct demangled symbols in C++ program
> 
> An issue can be observed when probe C++ demangled symbol with steps:
> 
>   # nm test_cpp_mangle | grep print_data
>     0000000000000c94 t _GLOBAL__sub_I__Z10print_datai
>     0000000000000afc T _Z10print_datai
>     0000000000000b38 T _Z10print_dataR5Point
> 
>   # ./perf probe -x /home/niayan01/test_cpp_mangle -F --demangle
>     ...
>     print_data(Point&)
>     print_data(int)
>     ...
> 
>   # ./perf --debug verbose=3 probe -x test_cpp_mangle --add "test=print_data(int)"
>     probe-definition(0): test=print_data(int)
>     symbol:print_data(int) file:(null) line:0 offset:0 return:0 lazy:(null)
>     0 arguments
>     Open Debuginfo file: /home/niayan01/test_cpp_mangle
>     Try to find probe point from debuginfo.
>     Symbol print_data(int) address found : afc
>     Matched function: print_data [2ccf]
>     Probe point found: print_data+0
>     Found 1 probe_trace_events.
>     Opening /sys/kernel/tracing//uprobe_events write=1
>     Opening /sys/kernel/tracing//README write=0
>     Writing event: p:probe_test_cpp_mangle/test /home/niayan01/test_cpp_mangle:0xb38
>     ...
> 
> When tried to probe symbol "print_data(int)", the log show:
> 
>     Symbol print_data(int) address found : afc
> 
> The found address is 0xafc - which is right if we connect with the
> output result from nm. Afterwards when write event, the command uses
> offset 0xb38 in the last log, which is a wrong address.
> 
> The dwarf_diename() gets a common function name, in above case, it
> returns string "print_data". As a result, the tool parses the offset
> based on the common name. This leads to probe at the wrong symbol
> "print_data(Point&)".
> 
> To fix the issue, use the die_get_linkage_name() function to retrieve
> the distinct linkage name - this is the mangled name for the C++ case.
> Based on this unique name, the tool can get a correct offset for
> probing. Based on DWARF doc, it is possible the linkage name is missed
> in the DIE, it rolls back to use dwarf_diename().

Can you add the result after applying this patch here?

Thank you,

> 
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>  tools/perf/util/probe-finder.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> index 630e16c54ed5..28a14005fc1f 100644
> --- a/tools/perf/util/probe-finder.c
> +++ b/tools/perf/util/probe-finder.c
> @@ -1583,8 +1583,21 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
> 
>         /* Find a corresponding function (name, baseline and baseaddr) */
>         if (die_find_realfunc(&cudie, (Dwarf_Addr)addr, &spdie)) {
> -               /* Get function entry information */
> -               func = basefunc = dwarf_diename(&spdie);
> +               /*
> +                * Get function entry information.
> +                *
> +                * As described in the document DWARF Debugging Information
> +                * Format Version 5, section 2.22 Linkage Names, "mangled names,
> +                * are used in various ways, ... to distinguish multiple
> +                * entities that have the same name".
> +                *
> +                * Firstly try to get distinct linkage name, if fail then
> +                * rollback to get associated name in DIE.
> +                */
> +               func = basefunc = die_get_linkage_name(&spdie);
> +               if (!func)
> +                        func = basefunc = dwarf_diename(&spdie);
> +
>                 if (!func ||
>                     die_entrypc(&spdie, &baseaddr) != 0 ||
>                     dwarf_decl_line(&spdie, &baseline) != 0) {
> --
> 2.25.1
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v1 3/3] perf probe: Generate hash event for long symbol

[Leo Yan]

On 10/12/24 06:30, Masami Hiramatsu (Google) wrote:

[...]

>> ---8<---
>>
>>  From 3b09a6f89c7e383c6b1d2b7e6bd80c6bfa658d5b Mon Sep 17 00:00:00 2001
>> From: Leo Yan <leo.yan@arm.com>
>> Date: Fri, 11 Oct 2024 07:58:08 +0000
>> Subject: [PATCH] perf probe: Correct demangled symbols in C++ program
>>
>> An issue can be observed when probe C++ demangled symbol with steps:
>>
>>    # nm test_cpp_mangle | grep print_data
>>      0000000000000c94 t _GLOBAL__sub_I__Z10print_datai
>>      0000000000000afc T _Z10print_datai
>>      0000000000000b38 T _Z10print_dataR5Point
>>
>>    # ./perf probe -x /home/niayan01/test_cpp_mangle -F --demangle
>>      ...
>>      print_data(Point&)
>>      print_data(int)
>>      ...
>>
>>    # ./perf --debug verbose=3 probe -x test_cpp_mangle --add "test=print_data(int)"
>>      probe-definition(0): test=print_data(int)
>>      symbol:print_data(int) file:(null) line:0 offset:0 return:0 lazy:(null)
>>      0 arguments
>>      Open Debuginfo file: /home/niayan01/test_cpp_mangle
>>      Try to find probe point from debuginfo.
>>      Symbol print_data(int) address found : afc
>>      Matched function: print_data [2ccf]
>>      Probe point found: print_data+0
>>      Found 1 probe_trace_events.
>>      Opening /sys/kernel/tracing//uprobe_events write=1
>>      Opening /sys/kernel/tracing//README write=0
>>      Writing event: p:probe_test_cpp_mangle/test /home/niayan01/test_cpp_mangle:0xb38
>>      ...
>>
>> When tried to probe symbol "print_data(int)", the log show:
>>
>>      Symbol print_data(int) address found : afc
>>
>> The found address is 0xafc - which is right if we connect with the
>> output result from nm. Afterwards when write event, the command uses
>> offset 0xb38 in the last log, which is a wrong address.
>>
>> The dwarf_diename() gets a common function name, in above case, it
>> returns string "print_data". As a result, the tool parses the offset
>> based on the common name. This leads to probe at the wrong symbol
>> "print_data(Point&)".
>>
>> To fix the issue, use the die_get_linkage_name() function to retrieve
>> the distinct linkage name - this is the mangled name for the C++ case.
>> Based on this unique name, the tool can get a correct offset for
>> probing. Based on DWARF doc, it is possible the linkage name is missed
>> in the DIE, it rolls back to use dwarf_diename().
> 
> Can you add the result after applying this patch here?

Sure. The result with this patch is:

   # perf --debug verbose=3 probe -x test_cpp_mangle --add "test=print_data(int)"
     probe-definition(0): test=print_data(int)
     symbol:print_data(int) file:(null) line:0 offset:0 return:0 lazy:(null)
     0 arguments
     Open Debuginfo file: /home/niayan01/test_cpp_mangle
     Try to find probe point from debuginfo.
     Symbol print_data(int) address found : afc
     Matched function: print_data [2d06]
     Probe point found: print_data+0
     Found 1 probe_trace_events.
     Opening /sys/kernel/tracing//uprobe_events write=1
     Opening /sys/kernel/tracing//README write=0
     Writing event: p:probe_test_cpp_mangle/test /home/niayan01/test_cpp_mangle:0xafc
     Added new event:
       probe_test_cpp_mangle:test (on print_data(int) in /home/niayan01/test_cpp_mangle)
                                                                                                                                                                                                                                                        
     You can now use it in all perf tools, such as:
                                                                                                                                                                                                                                                        
             perf record -e probe_test_cpp_mangle:test -aR sleep 1
                                                                                                                                                                                                                                                        
   # perf --debug verbose=3 probe -x test_cpp_mangle --add "test2=print_data(Point&)"
     probe-definition(0): test2=print_data(Point&)
     symbol:print_data(Point&) file:(null) line:0 offset:0 return:0 lazy:(null)
     0 arguments
     Open Debuginfo file: /home/niayan01/test_cpp_mangle
     Try to find probe point from debuginfo.
     Symbol print_data(Point&) address found : b38
     Matched function: print_data [2ccf]
     Probe point found: print_data+0
     Found 1 probe_trace_events.
     Opening /sys/kernel/tracing//uprobe_events write=1
     Parsing probe_events: p:probe_test_cpp_mangle/test /home/niayan01/test_cpp_mangle:0x0000000000000afc
     Group:probe_test_cpp_mangle Event:test probe:p
     Opening /sys/kernel/tracing//README write=0
     Writing event: p:probe_test_cpp_mangle/test2 /home/niayan01/test_cpp_mangle:0xb38
     Added new event:
       probe_test_cpp_mangle:test2 (on print_data(Point&) in /home/niayan01/test_cpp_mangle)
                                                                                                                                                                                                                                                        
     You can now use it in all perf tools, such as:
                                                                                                                                                                                                                                                        
             perf record -e probe_test_cpp_mangle:test2 -aR sleep 1

I have sent out a formal patch (with fixed tag):
https://lore.kernel.org/linux-perf-users/20241012141432.877894-1-leo.yan@arm.com/T/#u

Thanks for review!

Leo