[PATCH v1 0/2] perf arm64: Support SDT

[PATCH v1 0/2] perf arm64: Support SDT

[Leo Yan]

This patch is to enable SDT on Arm64.

Since Arm64 SDT marker in ELF file is different from other archs,
especially for using stack pointer (sp) to retrieve data for local
variables, patch 01 is used to fixup the arguments for this special
case.  Patch 02 is to add argument support for Arm64 SDT.

This patch set has been verified on Arm64/x86_64 platforms with a
testing program usdt_test [1].  The program run the SDT interfaces
one by one for DTRACE_PROBE, DTRACE_PROBE1, ..., DTRACE_PROBE12, so
it tries to verify probe with different count of arguments (the
arguments count is 0 to 12).

The testing flow and result are shown as below:

  # perf buildid-cache --add /root/test/usdt_test
  # perf probe sdt_usdt:test_probe
  # perf probe sdt_usdt:test_probe_param1
  # perf probe sdt_usdt:test_probe_param1x
  # perf probe sdt_usdt:test_probe_param2
  # perf probe sdt_usdt:test_probe_param2x
  # perf probe sdt_usdt:test_probe_param3
  # perf probe sdt_usdt:test_probe_param3x
  # perf probe sdt_usdt:test_probe_param4
  # perf probe sdt_usdt:test_probe_param4x
  # perf probe sdt_usdt:test_probe_param5
  # perf probe sdt_usdt:test_probe_param5x
  # perf probe sdt_usdt:test_probe_param6
  # perf probe sdt_usdt:test_probe_param6x
  # perf probe sdt_usdt:test_probe_param7
  # perf probe sdt_usdt:test_probe_param7x
  # perf probe sdt_usdt:test_probe_param8
  # perf probe sdt_usdt:test_probe_param8x
  # perf probe sdt_usdt:test_probe_param9
  # perf probe sdt_usdt:test_probe_param9x
  # perf probe sdt_usdt:test_probe_param10
  # perf probe sdt_usdt:test_probe_param10x
  # perf probe sdt_usdt:test_probe_param11
  # perf probe sdt_usdt:test_probe_param11x
  # perf probe sdt_usdt:test_probe_param12
  # perf probe sdt_usdt:test_probe_param12x

  # perf record \
        -e sdt_usdt:test_probe_param1 -e sdt_usdt:test_probe_param1x \
        -e sdt_usdt:test_probe_param2 -e sdt_usdt:test_probe_param2x \
        -e sdt_usdt:test_probe_param3 -e sdt_usdt:test_probe_param3x \
        -e sdt_usdt:test_probe_param4 -e sdt_usdt:test_probe_param4x \
        -e sdt_usdt:test_probe_param5 -e sdt_usdt:test_probe_param5x \
        -e sdt_usdt:test_probe_param6 -e sdt_usdt:test_probe_param6x \
        -e sdt_usdt:test_probe_param7 -e sdt_usdt:test_probe_param7x \
        -e sdt_usdt:test_probe_param8 -e sdt_usdt:test_probe_param8x \
        -e sdt_usdt:test_probe_param9 -e sdt_usdt:test_probe_param9x \
        -e sdt_usdt:test_probe_param10 -e sdt_usdt:test_probe_param10x \
        -e sdt_usdt:test_probe_param11 -e sdt_usdt:test_probe_param11x \
        -e sdt_usdt:test_probe_param12 -e sdt_usdt:test_probe_param12x \
        -e sdt_usdt:test_probe  -aR sleep 5

   # ./usdt_test   => Execute in another terminal

   # perf script

       usdt_test  7999 [003] 80493.418276:          sdt_usdt:test_probe: (aaaab0d80714)
       usdt_test  7999 [003] 80493.418352:   sdt_usdt:test_probe_param1: (aaaab0d80728) arg1=1
       usdt_test  7999 [003] 80493.418379:   sdt_usdt:test_probe_param2: (aaaab0d80744) arg1=1 arg2=2
       usdt_test  7999 [003] 80493.418405:   sdt_usdt:test_probe_param3: (aaaab0d80764) arg1=1 arg2=2 arg3=3
       usdt_test  7999 [003] 80493.418432:   sdt_usdt:test_probe_param4: (aaaab0d80788) arg1=1 arg2=2 arg3=3 arg4=4
       usdt_test  7999 [003] 80493.418459:   sdt_usdt:test_probe_param5: (aaaab0d807b0) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5
       usdt_test  7999 [003] 80493.418487:   sdt_usdt:test_probe_param6: (aaaab0d807dc) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6
       usdt_test  7999 [003] 80493.418516:   sdt_usdt:test_probe_param7: (aaaab0d8080c) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6 arg7=7
       usdt_test  7999 [003] 80493.418545:   sdt_usdt:test_probe_param8: (aaaab0d80840) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6 arg7=7 arg8=8
       usdt_test  7999 [003] 80493.418574:   sdt_usdt:test_probe_param9: (aaaab0d80874) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6 arg7=7 arg8=8 arg9=9
       usdt_test  7999 [003] 80493.418603:  sdt_usdt:test_probe_param10: (aaaab0d808a8) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6 arg7=7 arg8=8 arg9=9 arg10=10
       usdt_test  7999 [003] 80493.418632:  sdt_usdt:test_probe_param11: (aaaab0d808dc) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6 arg7=7 arg8=8 arg9=9 arg10=10 arg11=11
       usdt_test  7999 [003] 80493.418662:  sdt_usdt:test_probe_param12: (aaaab0d80910) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6 arg7=7 arg8=8 arg9=9 arg10=10 arg11=11 arg12=12
       usdt_test  7999 [003] 80493.418687:  sdt_usdt:test_probe_param1x: (aaaab0d8092c) arg1=1
       usdt_test  7999 [003] 80493.418713:  sdt_usdt:test_probe_param2x: (aaaab0d80950) arg1=1 arg2=2
       usdt_test  7999 [003] 80493.418739:  sdt_usdt:test_probe_param3x: (aaaab0d8097c) arg1=1 arg2=2 arg3=3
       usdt_test  7999 [003] 80493.418766:  sdt_usdt:test_probe_param4x: (aaaab0d809b0) arg1=1 arg2=2 arg3=3 arg4=4
       usdt_test  7999 [003] 80493.418792:  sdt_usdt:test_probe_param5x: (aaaab0d809ec) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5
       usdt_test  7999 [003] 80493.418820:  sdt_usdt:test_probe_param6x: (aaaab0d80a30) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6
       usdt_test  7999 [003] 80493.418847:  sdt_usdt:test_probe_param7x: (aaaab0d80a7c) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6 arg7=7
       usdt_test  7999 [003] 80493.418875:  sdt_usdt:test_probe_param8x: (aaaab0d80ad0) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6 arg7=7 arg8=8
       usdt_test  7999 [003] 80493.418904:  sdt_usdt:test_probe_param9x: (aaaab0d80b2c) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6 arg7=7 arg8=8 arg9=9
       usdt_test  7999 [003] 80493.418933: sdt_usdt:test_probe_param10x: (aaaab0d80b90) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6 arg7=7 arg8=8 arg9=9 arg10=10
       usdt_test  7999 [003] 80493.418962: sdt_usdt:test_probe_param11x: (aaaab0d80bfc) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6 arg7=7 arg8=8 arg9=9 arg10=10 arg11=11
       usdt_test  7999 [003] 80493.418991: sdt_usdt:test_probe_param12x: (aaaab0d80cb0) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6 arg7=7 arg8=8 arg9=281474762776336 arg10=281474762776340 arg11=281474762776344 arg12=281474762776348

[1] https://people.linaro.org/~leo.yan/debug/perf/usdt_test.c


Leo Yan (2):
  perf probe: Fixup Arm64 SDT arguments
  perf arm64: Add argument support for SDT

 tools/perf/arch/arm64/util/perf_regs.c | 94 ++++++++++++++++++++++++++
 tools/perf/util/probe-file.c           | 32 ++++++++-
 2 files changed, 124 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH v1 1/2] perf probe: Fixup Arm64 SDT arguments

[Leo Yan]

Arm64 ELF section '.note.stapsdt' uses string format "-4@[sp, NUM]" if
the probe is to access data in stack, e.g. below is an example for
dumping Arm64 ELF file and shows the argument format:

  Arguments: -4@[sp, 12] -4@[sp, 8] -4@[sp, 4]

Comparing against other archs' argument format, Arm64's argument
introduces an extra space character in the middle of square brackets,
due to argv_split() uses space as splitter, the argument is wrongly
divided into two items.

To support Arm64 SDT, this patch fixes up for this case, if any item
contains sub string "[sp", concatenates the two continuous items.  And
adds the detailed explaination in comment.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/util/probe-file.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index 064b63a6a3f3..60878c859e60 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -794,6 +794,8 @@ static char *synthesize_sdt_probe_command(struct sdt_note *note,
 	char *ret = NULL, **args;
 	int i, args_count, err;
 	unsigned long long ref_ctr_offset;
+	char *arg;
+	int arg_idx = 0;
 
 	if (strbuf_init(&buf, 32) < 0)
 		return NULL;
@@ -815,8 +817,34 @@ static char *synthesize_sdt_probe_command(struct sdt_note *note,
 	if (note->args) {
 		args = argv_split(note->args, &args_count);
 
-		for (i = 0; i < args_count; ++i) {
-			if (synthesize_sdt_probe_arg(&buf, i, args[i]) < 0)
+		for (i = 0; i < args_count; ) {
+			/*
+			 * FIXUP: Arm64 ELF section '.note.stapsdt' uses string
+			 * format "-4@[sp, NUM]" if a probe is to access data in
+			 * the stack, e.g. below is an example for the SDT
+			 * Arguments:
+			 *
+			 *   Arguments: -4@[sp, 12] -4@[sp, 8] -4@[sp, 4]
+			 *
+			 * Since the string introduces an extra space character
+			 * in the middle of square brackets, the argument is
+			 * divided into two items.  Fixup for this case, if an
+			 * item contains sub string "[sp,", need to concatenate
+			 * the two items.
+			 */
+			if (strstr(args[i], "[sp,") && (i+1) < args_count) {
+				arg = strcat(args[i], args[i+1]);
+				i += 2;
+			} else {
+				arg = strdup(args[i]);
+				i += 1;
+			}
+
+			err = synthesize_sdt_probe_arg(&buf, arg_idx, arg);
+			free(arg);
+			arg_idx++;
+
+			if (err < 0)
 				goto error;
 		}
 	}
-- 
2.17.1

[PATCH v1 2/2] perf arm64: Add argument support for SDT

[Leo Yan]

Now the two OP formats are used for SDT marker argument in Arm64 ELF,
one format is genreal register xNUM (e.g. x1, x2, etc), another is for
using stack pointer to access local variables (e.g. [sp], [sp, 8]).

This patch adds support SDT marker argument for Arm64, it parses OP and
converts to uprobe compatible format.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 tools/perf/arch/arm64/util/perf_regs.c | 94 ++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/tools/perf/arch/arm64/util/perf_regs.c b/tools/perf/arch/arm64/util/perf_regs.c
index 54efa12fdbea..6b4b18283041 100644
--- a/tools/perf/arch/arm64/util/perf_regs.c
+++ b/tools/perf/arch/arm64/util/perf_regs.c
@@ -1,4 +1,12 @@
 // SPDX-License-Identifier: GPL-2.0
+#include <errno.h>
+#include <regex.h>
+#include <string.h>
+#include <linux/kernel.h>
+#include <linux/zalloc.h>
+
+#include "../../../util/debug.h"
+#include "../../../util/event.h"
 #include "../../../util/perf_regs.h"
 
 const struct sample_reg sample_reg_masks[] = {
@@ -37,3 +45,89 @@ const struct sample_reg sample_reg_masks[] = {
 	SMPL_REG(pc, PERF_REG_ARM64_PC),
 	SMPL_REG_END
 };
+
+/* %xNUM */
+#define SDT_OP_REGEX1  "^(x[1-2]?[0-9]|3[0-1])$"
+
+/* [sp], [sp, NUM] or [sp,NUM] */
+#define SDT_OP_REGEX2  "^\\[sp(, *)?([0-9]+)?\\]$"
+
+static regex_t sdt_op_regex1, sdt_op_regex2;
+
+static int sdt_init_op_regex(void)
+{
+	static int initialized;
+	int ret = 0;
+
+	if (initialized)
+		return 0;
+
+	ret = regcomp(&sdt_op_regex1, SDT_OP_REGEX1, REG_EXTENDED);
+	if (ret)
+		goto error;
+
+	ret = regcomp(&sdt_op_regex2, SDT_OP_REGEX2, REG_EXTENDED);
+	if (ret)
+		goto free_regex1;
+
+	initialized = 1;
+	return 0;
+
+free_regex1:
+	regfree(&sdt_op_regex1);
+error:
+	pr_debug4("Regex compilation error.\n");
+	return ret;
+}
+
+/*
+ * SDT marker arguments on Arm64 uses %xREG or [sp, NUM], currently
+ * support these two formats.
+ */
+int arch_sdt_arg_parse_op(char *old_op, char **new_op)
+{
+	int ret, new_len;
+	regmatch_t rm[5];
+
+	ret = sdt_init_op_regex();
+	if (ret < 0)
+		return ret;
+
+	if (!regexec(&sdt_op_regex1, old_op, 3, rm, 0)) {
+		/* Extract xNUM */
+		new_len = 2;	/* % NULL */
+		new_len += (int)(rm[1].rm_eo - rm[1].rm_so);
+
+		*new_op = zalloc(new_len);
+		if (!*new_op)
+			return -ENOMEM;
+
+		scnprintf(*new_op, new_len, "%%%.*s",
+			(int)(rm[1].rm_eo - rm[1].rm_so), old_op + rm[1].rm_so);
+	} else if (!regexec(&sdt_op_regex2, old_op, 5, rm, 0)) {
+		/* [sp], [sp, NUM] or [sp,NUM] */
+		new_len = 7;	/* + ( % s p ) NULL */
+
+		/* If the arugment is [sp], need to fill offset '0' */
+		if (rm[2].rm_so == -1)
+			new_len += 1;
+		else
+			new_len += (int)(rm[2].rm_eo - rm[2].rm_so);
+
+		*new_op = zalloc(new_len);
+		if (!*new_op)
+			return -ENOMEM;
+
+		if (rm[2].rm_so == -1)
+			scnprintf(*new_op, new_len, "+0(%%sp)");
+		else
+			scnprintf(*new_op, new_len, "+%.*s(%%sp)",
+				  (int)(rm[2].rm_eo - rm[2].rm_so),
+				  old_op + rm[2].rm_so);
+	} else {
+		pr_debug4("Skipping unsupported SDT argument: %s\n", old_op);
+		return SDT_ARG_SKIP;
+	}
+
+	return SDT_ARG_VALID;
+}
-- 
2.17.1

Re: [PATCH v1 0/2] perf arm64: Support SDT

[Masami Hiramatsu]

Hi Leo,

On Wed, 23 Dec 2020 14:39:03 +0800
Leo Yan <leo.yan@linaro.org> wrote:

> This patch is to enable SDT on Arm64.
> 
> Since Arm64 SDT marker in ELF file is different from other archs,
> especially for using stack pointer (sp) to retrieve data for local
> variables, patch 01 is used to fixup the arguments for this special
> case.  Patch 02 is to add argument support for Arm64 SDT.

Both patches look good to me.

Acked-by: Masami Hiramatsu <mhiramat@kernel.org> for the seires.

Thank you!

> 
> This patch set has been verified on Arm64/x86_64 platforms with a
> testing program usdt_test [1].  The program run the SDT interfaces
> one by one for DTRACE_PROBE, DTRACE_PROBE1, ..., DTRACE_PROBE12, so
> it tries to verify probe with different count of arguments (the
> arguments count is 0 to 12).
> 
> The testing flow and result are shown as below:
> 
>   # perf buildid-cache --add /root/test/usdt_test
>   # perf probe sdt_usdt:test_probe
>   # perf probe sdt_usdt:test_probe_param1
>   # perf probe sdt_usdt:test_probe_param1x
>   # perf probe sdt_usdt:test_probe_param2
>   # perf probe sdt_usdt:test_probe_param2x
>   # perf probe sdt_usdt:test_probe_param3
>   # perf probe sdt_usdt:test_probe_param3x
>   # perf probe sdt_usdt:test_probe_param4
>   # perf probe sdt_usdt:test_probe_param4x
>   # perf probe sdt_usdt:test_probe_param5
>   # perf probe sdt_usdt:test_probe_param5x
>   # perf probe sdt_usdt:test_probe_param6
>   # perf probe sdt_usdt:test_probe_param6x
>   # perf probe sdt_usdt:test_probe_param7
>   # perf probe sdt_usdt:test_probe_param7x
>   # perf probe sdt_usdt:test_probe_param8
>   # perf probe sdt_usdt:test_probe_param8x
>   # perf probe sdt_usdt:test_probe_param9
>   # perf probe sdt_usdt:test_probe_param9x
>   # perf probe sdt_usdt:test_probe_param10
>   # perf probe sdt_usdt:test_probe_param10x
>   # perf probe sdt_usdt:test_probe_param11
>   # perf probe sdt_usdt:test_probe_param11x
>   # perf probe sdt_usdt:test_probe_param12
>   # perf probe sdt_usdt:test_probe_param12x
> 
>   # perf record \
>         -e sdt_usdt:test_probe_param1 -e sdt_usdt:test_probe_param1x \
>         -e sdt_usdt:test_probe_param2 -e sdt_usdt:test_probe_param2x \
>         -e sdt_usdt:test_probe_param3 -e sdt_usdt:test_probe_param3x \
>         -e sdt_usdt:test_probe_param4 -e sdt_usdt:test_probe_param4x \
>         -e sdt_usdt:test_probe_param5 -e sdt_usdt:test_probe_param5x \
>         -e sdt_usdt:test_probe_param6 -e sdt_usdt:test_probe_param6x \
>         -e sdt_usdt:test_probe_param7 -e sdt_usdt:test_probe_param7x \
>         -e sdt_usdt:test_probe_param8 -e sdt_usdt:test_probe_param8x \
>         -e sdt_usdt:test_probe_param9 -e sdt_usdt:test_probe_param9x \
>         -e sdt_usdt:test_probe_param10 -e sdt_usdt:test_probe_param10x \
>         -e sdt_usdt:test_probe_param11 -e sdt_usdt:test_probe_param11x \
>         -e sdt_usdt:test_probe_param12 -e sdt_usdt:test_probe_param12x \
>         -e sdt_usdt:test_probe  -aR sleep 5
> 
>    # ./usdt_test   => Execute in another terminal
> 
>    # perf script
> 
>        usdt_test  7999 [003] 80493.418276:          sdt_usdt:test_probe: (aaaab0d80714)
>        usdt_test  7999 [003] 80493.418352:   sdt_usdt:test_probe_param1: (aaaab0d80728) arg1=1
>        usdt_test  7999 [003] 80493.418379:   sdt_usdt:test_probe_param2: (aaaab0d80744) arg1=1 arg2=2
>        usdt_test  7999 [003] 80493.418405:   sdt_usdt:test_probe_param3: (aaaab0d80764) arg1=1 arg2=2 arg3=3
>        usdt_test  7999 [003] 80493.418432:   sdt_usdt:test_probe_param4: (aaaab0d80788) arg1=1 arg2=2 arg3=3 arg4=4
>        usdt_test  7999 [003] 80493.418459:   sdt_usdt:test_probe_param5: (aaaab0d807b0) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5
>        usdt_test  7999 [003] 80493.418487:   sdt_usdt:test_probe_param6: (aaaab0d807dc) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6
>        usdt_test  7999 [003] 80493.418516:   sdt_usdt:test_probe_param7: (aaaab0d8080c) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6 arg7=7
>        usdt_test  7999 [003] 80493.418545:   sdt_usdt:test_probe_param8: (aaaab0d80840) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6 arg7=7 arg8=8
>        usdt_test  7999 [003] 80493.418574:   sdt_usdt:test_probe_param9: (aaaab0d80874) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6 arg7=7 arg8=8 arg9=9
>        usdt_test  7999 [003] 80493.418603:  sdt_usdt:test_probe_param10: (aaaab0d808a8) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6 arg7=7 arg8=8 arg9=9 arg10=10
>        usdt_test  7999 [003] 80493.418632:  sdt_usdt:test_probe_param11: (aaaab0d808dc) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6 arg7=7 arg8=8 arg9=9 arg10=10 arg11=11
>        usdt_test  7999 [003] 80493.418662:  sdt_usdt:test_probe_param12: (aaaab0d80910) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6 arg7=7 arg8=8 arg9=9 arg10=10 arg11=11 arg12=12
>        usdt_test  7999 [003] 80493.418687:  sdt_usdt:test_probe_param1x: (aaaab0d8092c) arg1=1
>        usdt_test  7999 [003] 80493.418713:  sdt_usdt:test_probe_param2x: (aaaab0d80950) arg1=1 arg2=2
>        usdt_test  7999 [003] 80493.418739:  sdt_usdt:test_probe_param3x: (aaaab0d8097c) arg1=1 arg2=2 arg3=3
>        usdt_test  7999 [003] 80493.418766:  sdt_usdt:test_probe_param4x: (aaaab0d809b0) arg1=1 arg2=2 arg3=3 arg4=4
>        usdt_test  7999 [003] 80493.418792:  sdt_usdt:test_probe_param5x: (aaaab0d809ec) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5
>        usdt_test  7999 [003] 80493.418820:  sdt_usdt:test_probe_param6x: (aaaab0d80a30) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6
>        usdt_test  7999 [003] 80493.418847:  sdt_usdt:test_probe_param7x: (aaaab0d80a7c) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6 arg7=7
>        usdt_test  7999 [003] 80493.418875:  sdt_usdt:test_probe_param8x: (aaaab0d80ad0) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6 arg7=7 arg8=8
>        usdt_test  7999 [003] 80493.418904:  sdt_usdt:test_probe_param9x: (aaaab0d80b2c) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6 arg7=7 arg8=8 arg9=9
>        usdt_test  7999 [003] 80493.418933: sdt_usdt:test_probe_param10x: (aaaab0d80b90) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6 arg7=7 arg8=8 arg9=9 arg10=10
>        usdt_test  7999 [003] 80493.418962: sdt_usdt:test_probe_param11x: (aaaab0d80bfc) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6 arg7=7 arg8=8 arg9=9 arg10=10 arg11=11
>        usdt_test  7999 [003] 80493.418991: sdt_usdt:test_probe_param12x: (aaaab0d80cb0) arg1=1 arg2=2 arg3=3 arg4=4 arg5=5 arg6=6 arg7=7 arg8=8 arg9=281474762776336 arg10=281474762776340 arg11=281474762776344 arg12=281474762776348
> 
> [1] https://people.linaro.org/~leo.yan/debug/perf/usdt_test.c
> 
> 
> Leo Yan (2):
>   perf probe: Fixup Arm64 SDT arguments
>   perf arm64: Add argument support for SDT
> 
>  tools/perf/arch/arm64/util/perf_regs.c | 94 ++++++++++++++++++++++++++
>  tools/perf/util/probe-file.c           | 32 ++++++++-
>  2 files changed, 124 insertions(+), 2 deletions(-)
> 
> -- 
> 2.17.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v1 1/2] perf probe: Fixup Arm64 SDT arguments

[Arnaldo Carvalho de Melo]

Em Wed, Dec 23, 2020 at 02:39:04PM +0800, Leo Yan escreveu:
> Arm64 ELF section '.note.stapsdt' uses string format "-4@[sp, NUM]" if
> the probe is to access data in stack, e.g. below is an example for
> dumping Arm64 ELF file and shows the argument format:
> 
>   Arguments: -4@[sp, 12] -4@[sp, 8] -4@[sp, 4]
> 
> Comparing against other archs' argument format, Arm64's argument
> introduces an extra space character in the middle of square brackets,
> due to argv_split() uses space as splitter, the argument is wrongly
> divided into two items.
> 
> To support Arm64 SDT, this patch fixes up for this case, if any item
> contains sub string "[sp", concatenates the two continuous items.  And
> adds the detailed explaination in comment.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  tools/perf/util/probe-file.c | 32 ++++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 064b63a6a3f3..60878c859e60 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -794,6 +794,8 @@ static char *synthesize_sdt_probe_command(struct sdt_note *note,
>  	char *ret = NULL, **args;
>  	int i, args_count, err;
>  	unsigned long long ref_ctr_offset;
> +	char *arg;
> +	int arg_idx = 0;
>  
>  	if (strbuf_init(&buf, 32) < 0)
>  		return NULL;
> @@ -815,8 +817,34 @@ static char *synthesize_sdt_probe_command(struct sdt_note *note,
>  	if (note->args) {
>  		args = argv_split(note->args, &args_count);
>  
> -		for (i = 0; i < args_count; ++i) {
> -			if (synthesize_sdt_probe_arg(&buf, i, args[i]) < 0)
> +		for (i = 0; i < args_count; ) {
> +			/*
> +			 * FIXUP: Arm64 ELF section '.note.stapsdt' uses string
> +			 * format "-4@[sp, NUM]" if a probe is to access data in
> +			 * the stack, e.g. below is an example for the SDT
> +			 * Arguments:
> +			 *
> +			 *   Arguments: -4@[sp, 12] -4@[sp, 8] -4@[sp, 4]
> +			 *
> +			 * Since the string introduces an extra space character
> +			 * in the middle of square brackets, the argument is
> +			 * divided into two items.  Fixup for this case, if an
> +			 * item contains sub string "[sp,", need to concatenate
> +			 * the two items.
> +			 */
> +			if (strstr(args[i], "[sp,") && (i+1) < args_count) {
> +				arg = strcat(args[i], args[i+1]);
> +				i += 2;
> +			} else {
> +				arg = strdup(args[i]);
> +				i += 1;
> +			}
> +
> +			err = synthesize_sdt_probe_arg(&buf, arg_idx, arg);
> +			free(arg);

So you free here unconditionally because either you used something you
got from argv_split() that strdup'ed all the entries in the array it
returns, or that you strdup'ed in the else branch.

But then you may not free all the things argv_split() returned, right?
Also, that strcat(args[i], args[i+1]), are you sure that is safe? strcat
expects dest to have enough space for the concatenation, I don't see
argv_split[] adding extra bytes, just a strdup().

So probably you need asprintf() where you use strcat() and then, at the
end of the loop, you need to free what argv_split() returned, using
argv_free(), no?

Also please check strdup() (and then asprintf) managed to allocate, else
synthesize_sdt_probe_arg() will receive its 'desc' argument as NULL, do
_another_ strdup on it and boom.

Or am I missing something? :)

I just looked ant synthesize_sdt_probe_command() is leaking the args it
gets from argv_split()

So this patch is needed, ack?

diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index 064b63a6a3f311cd..bbecb449ea944395 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -791,7 +791,7 @@ static char *synthesize_sdt_probe_command(struct sdt_note *note,
 					const char *sdtgrp)
 {
 	struct strbuf buf;
-	char *ret = NULL, **args;
+	char *ret = NULL;
 	int i, args_count, err;
 	unsigned long long ref_ctr_offset;
 
@@ -813,12 +813,19 @@ static char *synthesize_sdt_probe_command(struct sdt_note *note,
 		goto out;
 
 	if (note->args) {
-		args = argv_split(note->args, &args_count);
+		char **args = argv_split(note->args, &args_count);
+
+		if (args == NULL)
+			goto error;
 
 		for (i = 0; i < args_count; ++i) {
-			if (synthesize_sdt_probe_arg(&buf, i, args[i]) < 0)
+			if (synthesize_sdt_probe_arg(&buf, i, args[i]) < 0) {
+				argv_free(args);
 				goto error;
+			}
 		}
+
+		argv_free(args);
 	}
 
 out:

Re: [PATCH v1 1/2] perf probe: Fixup Arm64 SDT arguments

[Leo Yan]

On Thu, Dec 24, 2020 at 10:51:39AM -0300, Arnaldo Carvalho de Melo wrote:

> Em Wed, Dec 23, 2020 at 02:39:04PM +0800, Leo Yan escreveu:
> > Arm64 ELF section '.note.stapsdt' uses string format "-4@[sp, NUM]" if
> > the probe is to access data in stack, e.g. below is an example for
> > dumping Arm64 ELF file and shows the argument format:
> > 
> >   Arguments: -4@[sp, 12] -4@[sp, 8] -4@[sp, 4]
> > 
> > Comparing against other archs' argument format, Arm64's argument
> > introduces an extra space character in the middle of square brackets,
> > due to argv_split() uses space as splitter, the argument is wrongly
> > divided into two items.
> > 
> > To support Arm64 SDT, this patch fixes up for this case, if any item
> > contains sub string "[sp", concatenates the two continuous items.  And
> > adds the detailed explaination in comment.
> > 
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  tools/perf/util/probe-file.c | 32 ++++++++++++++++++++++++++++++--
> >  1 file changed, 30 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> > index 064b63a6a3f3..60878c859e60 100644
> > --- a/tools/perf/util/probe-file.c
> > +++ b/tools/perf/util/probe-file.c
> > @@ -794,6 +794,8 @@ static char *synthesize_sdt_probe_command(struct sdt_note *note,
> >  	char *ret = NULL, **args;
> >  	int i, args_count, err;
> >  	unsigned long long ref_ctr_offset;
> > +	char *arg;
> > +	int arg_idx = 0;
> >  
> >  	if (strbuf_init(&buf, 32) < 0)
> >  		return NULL;
> > @@ -815,8 +817,34 @@ static char *synthesize_sdt_probe_command(struct sdt_note *note,
> >  	if (note->args) {
> >  		args = argv_split(note->args, &args_count);
> >  
> > -		for (i = 0; i < args_count; ++i) {
> > -			if (synthesize_sdt_probe_arg(&buf, i, args[i]) < 0)
> > +		for (i = 0; i < args_count; ) {
> > +			/*
> > +			 * FIXUP: Arm64 ELF section '.note.stapsdt' uses string
> > +			 * format "-4@[sp, NUM]" if a probe is to access data in
> > +			 * the stack, e.g. below is an example for the SDT
> > +			 * Arguments:
> > +			 *
> > +			 *   Arguments: -4@[sp, 12] -4@[sp, 8] -4@[sp, 4]
> > +			 *
> > +			 * Since the string introduces an extra space character
> > +			 * in the middle of square brackets, the argument is
> > +			 * divided into two items.  Fixup for this case, if an
> > +			 * item contains sub string "[sp,", need to concatenate
> > +			 * the two items.
> > +			 */
> > +			if (strstr(args[i], "[sp,") && (i+1) < args_count) {
> > +				arg = strcat(args[i], args[i+1]);
> > +				i += 2;
> > +			} else {
> > +				arg = strdup(args[i]);
> > +				i += 1;
> > +			}
> > +
> > +			err = synthesize_sdt_probe_arg(&buf, arg_idx, arg);
> > +			free(arg);
> 
> So you free here unconditionally because either you used something you
> got from argv_split() that strdup'ed all the entries in the array it
> returns, or that you strdup'ed in the else branch.


> But then you may not free all the things argv_split() returned, right?

Yes.

> Also, that strcat(args[i], args[i+1]), are you sure that is safe? strcat
> expects dest to have enough space for the concatenation, I don't see
> argv_split[] adding extra bytes, just a strdup().

Correct, will change to use asprintf().

> So probably you need asprintf() where you use strcat() and then, at the
> end of the loop, you need to free what argv_split() returned, using
> argv_free(), no?
> 
> Also please check strdup() (and then asprintf) managed to allocate, else
> synthesize_sdt_probe_arg() will receive its 'desc' argument as NULL, do
> _another_ strdup on it and boom.

Will add checking for the pointer from strdup()/asprintf().

> Or am I missing something? :)
> 
> I just looked ant synthesize_sdt_probe_command() is leaking the args it
> gets from argv_split()
> 
> So this patch is needed, ack?

Below change is good for me.  In the next respin, I will add this new
patch with your author name and send out.

Thanks a lot for the review, Masami & Arnaldo!

> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 064b63a6a3f311cd..bbecb449ea944395 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -791,7 +791,7 @@ static char *synthesize_sdt_probe_command(struct sdt_note *note,
>  					const char *sdtgrp)
>  {
>  	struct strbuf buf;
> -	char *ret = NULL, **args;
> +	char *ret = NULL;
>  	int i, args_count, err;
>  	unsigned long long ref_ctr_offset;
>  
> @@ -813,12 +813,19 @@ static char *synthesize_sdt_probe_command(struct sdt_note *note,
>  		goto out;
>  
>  	if (note->args) {
> -		args = argv_split(note->args, &args_count);
> +		char **args = argv_split(note->args, &args_count);
> +
> +		if (args == NULL)
> +			goto error;
>  
>  		for (i = 0; i < args_count; ++i) {
> -			if (synthesize_sdt_probe_arg(&buf, i, args[i]) < 0)
> +			if (synthesize_sdt_probe_arg(&buf, i, args[i]) < 0) {
> +				argv_free(args);
>  				goto error;
> +			}
>  		}
> +
> +		argv_free(args);
>  	}
>  
>  out: