Date:   Tue, 24 Jan 2017 14:20:21 +0530
From:   Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>
To:     Alexis Berlemont <alexis.berlemont@...il.com>
Cc:     linux-kernel@...r.kernel.org, peterz@...radead.org,
        mingo@...hat.com, acme@...nel.org,
        alexander.shishkin@...ux.intel.com,
        Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>
Subject: Re: [PATCH v5 2/2] perf probe: add sdt probes arguments into the
 uprobe cmd string

Hi Alexis,

On Wednesday 14 December 2016 05:37 AM, Alexis Berlemont wrote:
> An sdt probe can be associated with arguments but they were not passed
> to the user probe tracing interface (uprobe_events); this patch adapts
> the sdt argument descriptors according to the uprobe input format.
>
> As the uprobe parser does not support scaled address mode, perf will
> skip arguments which cannot be adapted to the uprobe format.
>
> Here are the results:
>
> $ perf buildid-cache -v --add test_sdt
> $ perf probe -x test_sdt sdt_libfoo:table_frob
> $ perf probe -x test_sdt sdt_libfoo:table_diddle
> $ perf record -e sdt_libfoo:table_frob -e sdt_libfoo:table_diddle test_sdt
> $ perf script
> test_sdt  ...   666.255678:   sdt_libfoo:table_frob: (4004d7) arg0=0 arg1=0
> test_sdt  ...   666.255683: sdt_libfoo:table_diddle: (40051a) arg0=0 arg1=0
> test_sdt  ...   666.255686:   sdt_libfoo:table_frob: (4004d7) arg0=1 arg1=2
> test_sdt  ...   666.255689: sdt_libfoo:table_diddle: (40051a) arg0=3 arg1=4
> test_sdt  ...   666.255692:   sdt_libfoo:table_frob: (4004d7) arg0=2 arg1=4
> test_sdt  ...   666.255694: sdt_libfoo:table_diddle: (40051a) arg0=6 arg1=8
>
> Signed-off-by: Alexis Berlemont <alexis.berlemont@...il.com>
> ---
>  tools/perf/arch/x86/util/perf_regs.c |  83 +++++++++++++++++
>  tools/perf/util/perf_regs.c          |   6 ++
>  tools/perf/util/perf_regs.h          |   6 ++
>  tools/perf/util/probe-file.c         | 170 ++++++++++++++++++++++++++++++++++-
>  4 files changed, 261 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/arch/x86/util/perf_regs.c b/tools/perf/arch/x86/util/perf_regs.c
> index c5db14f..09a7f55 100644
> --- a/tools/perf/arch/x86/util/perf_regs.c
> +++ b/tools/perf/arch/x86/util/perf_regs.c
> @@ -1,4 +1,7 @@
> +#include <string.h>
> +
>  #include "../../perf.h"
> +#include "../../util/util.h"
>  #include "../../util/perf_regs.h"
>
>  const struct sample_reg sample_reg_masks[] = {
> @@ -26,3 +29,83 @@ const struct sample_reg sample_reg_masks[] = {
>  #endif
>  	SMPL_REG_END
>  };
> +
> +struct sdt_name_reg {
> +	const char *sdt_name;
> +	const char *uprobe_name;
> +};
> +#define SDT_NAME_REG(n, m) {.sdt_name = "%" #n, .uprobe_name = "%" #m}
> +#define SDT_NAME_REG_END {.sdt_name = NULL, .uprobe_name = NULL}
> +
> +static const struct sdt_name_reg sdt_reg_renamings[] = {
> +	SDT_NAME_REG(eax, ax),
> +	SDT_NAME_REG(rax, ax),
> +	SDT_NAME_REG(ebx, bx),
> +	SDT_NAME_REG(rbx, bx),
> +	SDT_NAME_REG(ecx, cx),
> +	SDT_NAME_REG(rcx, cx),
> +	SDT_NAME_REG(edx, dx),
> +	SDT_NAME_REG(rdx, dx),
> +	SDT_NAME_REG(esi, si),
> +	SDT_NAME_REG(rsi, si),
> +	SDT_NAME_REG(edi, di),
> +	SDT_NAME_REG(rdi, di),
> +	SDT_NAME_REG(ebp, bp),
> +	SDT_NAME_REG(rbp, bp),
> +	SDT_NAME_REG_END,
> +};

I see many markers uses %rsp. Such markers are failing at perf probe.
Please add renaming entry for %rsp and %esp. For example:

$ readelf -n /usr/lib64/libpython3.5m.so.1.0
  ...
  Name: function__entry
  Arguments: 8@224(%rsp) 8@232(%rsp) -4@240(%rsp) 8@...x

$ sudo ./perf probe sdt_python:function__entry
  Failed to write event: Invalid argument
  Please upgrade your kernel to at least 3.14 to have access to feature +224(%rsp):u64
    Error: Failed to add events.


This code does not handle rNN registers with postfix('b', 'w', 'd').
Such markers are failing at perf probe. For example:

$ readelf -n /usr/lib64/libperl.so.5.24.0
  ...
  Name: sub__return
  Arguments: 8@...x 8@%r8 4@...d 8@...i

$ sudo ./perf probe -v sdt_perl:sub__entry
  ...
  Opening /sys/kernel/debug/tracing//uprobe_events write=1
  Writing event: p:sdt_perl/sub__entry /usr/lib64/libperl.so.5.24.0:0xbb780 arg1=%ax:u64 arg2=%r8:u64 arg3=%r9d:u32 arg4=%si:u64
  Failed to write event: Invalid argument
    Error: Failed to add events. Reason: Invalid argument (Code: -22)

 Can we add them like:

    /* rNN registers */
    SDT_NAME_REG(r8b,  r8),
    SDT_NAME_REG(r8w,  r8),
    SDT_NAME_REG(r8d,  r8),
    SDT_NAME_REG(r9b,  r9),
    ...
    SDT_NAME_REG(r14d, r14),
    SDT_NAME_REG(r15b, r15),
    SDT_NAME_REG(r15w, r15),
    SDT_NAME_REG(r15d, r15),

and ...

> +
> +int sdt_rename_register(char **pdesc, char *old_name)
> +{
> +	const struct sdt_name_reg *rnames = sdt_reg_renamings;
> +	char *new_desc, *old_desc = *pdesc;
> +	size_t prefix_len, sdt_len, uprobe_len, old_desc_len, offset;
> +	int ret = -1;
> +
> +	while (ret != 0 && rnames->sdt_name != NULL) {
> +		sdt_len = strlen(rnames->sdt_name);
> +		ret = strncmp(old_name, rnames->sdt_name, sdt_len);
> +		rnames += !!ret;
> +	}
> +
> +	if (rnames->sdt_name == NULL)
> +		return 0;
> +
> +	sdt_len = strlen(rnames->sdt_name);
> +	uprobe_len = strlen(rnames->uprobe_name);
> +	old_desc_len = strlen(old_desc) + 1;
> +
> +	new_desc = zalloc(old_desc_len + uprobe_len - sdt_len);
> +	if (new_desc == NULL)
> +		return -1;
> +
> +	/* Copy the chars before the register name (at least '%') */
> +	prefix_len = old_name - old_desc;
> +	memcpy(new_desc, old_desc, prefix_len);
> +
> +	/* Copy the new register name */
> +	memcpy(new_desc + prefix_len, rnames->uprobe_name, uprobe_len);
> +
> +	/* Copy the chars after the register name (if need be) */
> +	offset = prefix_len + sdt_len;

... remove below if(){} block.

> +	if (offset < old_desc_len) {
> +		/*
> +		 * The orginal register name can be suffixed by 'b',
> +		 * 'w' or 'd' to indicate its size; so, we need to
> +		 * skip this char if we met one.
> +		 */
> +		char sfx = old_desc[offset];
> +
> +		if (sfx == 'b' || sfx == 'w'  || sfx == 'd')
> +			offset++;
> +	}
> +
> +	if (offset < old_desc_len)
> +		memcpy(new_desc + prefix_len + uprobe_len,
> +			old_desc + offset, old_desc_len - offset);
> +
> +	free(old_desc);
> +	*pdesc = new_desc;
> +
> +	return 0;
> +}
> diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c
> index c4023f2..a37e593 100644
> --- a/tools/perf/util/perf_regs.c
> +++ b/tools/perf/util/perf_regs.c
> @@ -6,6 +6,12 @@ const struct sample_reg __weak sample_reg_masks[] = {
>  	SMPL_REG_END
>  };
>
> +int __weak sdt_rename_register(char **pdesc __maybe_unused,
> +			char *old_name __maybe_unused)
> +{
> +	return 0;
> +}
> +
>  #ifdef HAVE_PERF_REGS_SUPPORT
>  int perf_reg_value(u64 *valp, struct regs_dump *regs, int id)
>  {
> diff --git a/tools/perf/util/perf_regs.h b/tools/perf/util/perf_regs.h
> index 679d6e4..7544a15 100644
> --- a/tools/perf/util/perf_regs.h
> +++ b/tools/perf/util/perf_regs.h
> @@ -15,6 +15,12 @@ struct sample_reg {
>
>  extern const struct sample_reg sample_reg_masks[];
>
> +/*
> + * The table sdt_reg_renamings is used for adjusting gcc/gas-generated
> + * registers before filling the uprobe tracer interface.
> + */
> +int sdt_rename_register(char **pdesc, char *old_name);
> +
>  #ifdef HAVE_PERF_REGS_SUPPORT
>  #include <perf_regs.h>
>
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 436b647..d8a169e 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -27,6 +27,7 @@
>  #include "probe-event.h"
>  #include "probe-file.h"
>  #include "session.h"
> +#include "perf_regs.h"
>
>  #define MAX_CMDLEN 256
>
> @@ -687,6 +688,166 @@ static unsigned long long sdt_note__get_addr(struct sdt_note *note)
>  		 : (unsigned long long)note->addr.a64[0];
>  }
>
> +static const char * const type_to_suffix[] = {
> +	":s64", "", "", "", ":s32", "", ":s16", ":s8",
> +	"", ":u8", ":u16", "", ":u32", "", "", "", ":u64"
> +};
> +
> +static int synthesize_sdt_probe_arg(struct strbuf *buf, int i, const char *arg)
> +{
> +	char *tmp, *desc = strdup(arg);
> +	const char *prefix = "", *suffix = "";
> +	int ret = -1;
> +
> +	if (desc == NULL) {
> +		pr_debug4("Allocation error\n");
> +		return ret;
> +	}
> +
> +	tmp = strchr(desc, '@');
> +	if (tmp) {
> +		long type_idx;
> +		/*
> +		 * Isolate the string number and convert it into a
> +		 * binary value; this will be an index to get suffix
> +		 * of the uprobe name (defining the type)
> +		 */
> +		tmp[0] = '\0';
> +		type_idx = strtol(desc, NULL, 10);
> +		/* Check that the conversion went OK */
> +		if (type_idx == LONG_MIN || type_idx == LONG_MAX) {
> +			pr_debug4("Failed to parse sdt type\n");
> +			goto error;
> +		}
> +		/* Check that the converted value is OK */
> +		if (type_idx < -8 || type_idx > 8) {
> +			pr_debug4("Failed to get a valid sdt type\n");
> +			goto error;
> +		}
> +		suffix = type_to_suffix[type_idx + 8];
> +		/* Get rid of the sdt prefix which is now useless */
> +		tmp++;
> +		memmove(desc, tmp, strlen(tmp) + 1);
> +	}
> +

Below code is x86 specific. For example, Powerpc uses "i" for constant
arguments not "$". Powerpc does not use symbol as offset etc.

SDT marker argument is in N@OP format. Can we divide this function into
two components. One parses N and other parses OP. Parsing OP will be
arch specific code and should be moved under /tools/perf/arch/.

> +	/*
> +	 * The uprobe tracer format does not support all the
> +	 * addressing modes (notably: in x86 the scaled mode); so, we
> +	 * detect ',' characters, if there is just one, there is no
> +	 * use converting the sdt arg into a uprobe one.
> +	 */
> +	if (strchr(desc, ',')) {
> +		pr_debug4("Skipping unsupported SDT argument; %s\n", desc);
> +		goto out;
> +	}
> +
> +	/*
> +	 * If the argument addressing mode is indirect, we must check
> +	 * a few things...
> +	 */
> +	tmp = strchr(desc, '(');
> +	if (tmp) {
> +		int j;
> +
> +		/*
> +		 * ...if the addressing mode is indirect with a
> +		 * positive offset (ex.: "1608(%ax)"), we need to add
> +		 * a '+' prefix so as to be compliant with uprobe
> +		 * format.
> +		 */
> +		if (desc[0] != '+' && desc[0] != '-')
> +			prefix = "+";
> +
> +		/*
> +		 * ...or if the addressing mode is indirect with a symbol
> +		 * as offset, the argument will not be supported by
> +		 * the uprobe tracer format; so, let's skip this one.
> +		 */
> +		for (j = 0; j < tmp - desc; j++) {
> +			if (desc[j] != '+' && desc[j] != '-' &&
> +				!isdigit(desc[j])) {
> +				pr_debug4("Skipping unsupported SDT argument; "
> +					"%s\n", desc);
> +				goto out;
> +			}
> +		}
> +	}
> +
> +	/*
> +	 * The uprobe tracer format does not support constants; if we
> +	 * find one in the current argument, let's skip the argument.
> +	 */
> +	if (strchr(desc, '$')) {
> +		pr_debug4("Skipping unsupported SDT argument; %s\n", desc);
> +		goto out;
> +	}
> +
> +	/*
> +	 * The uprobe parser does not support all gas register names;
> +	 * so, we have to replace them (ex. for x86_64: %rax -> %ax);
> +	 * the loop below looks for the register names (starting with
> +	 * a '%' and tries to perform the needed renamings.
> +	 */
> +	tmp = strchr(desc, '%');
> +	while (tmp) {
> +		size_t offset = tmp - desc;
> +
> +		ret = sdt_rename_register(&desc, desc + offset);
> +		if (ret < 0)
> +			goto error;
> +
> +		/*
> +		 * The desc pointer might have changed; so, let's not
> +		 * try to reuse tmp for next lookup
> +		 */
> +		tmp = strchr(desc + offset + 1, '%');
> +	}
> +
> +	if (strbuf_addf(buf, " arg%d=%s%s%s", i + 1, prefix, desc, suffix) < 0)
> +		goto error;
> +
> +out:
> +	ret = 0;
> +error:
> +	free(desc);
> +	return ret;
> +}

Thanks,
Ravi

Powered by blists - more mailing lists