| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20170321235539.e0b3e40c2e5ad994356a7ac6@kernel.org>
Date: Tue, 21 Mar 2017 23:55:39 +0900
From: Masami Hiramatsu <mhiramat@...nel.org>
To: Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>
Cc: acme@...hat.com, alexis.berlemont@...il.com,
linux-kernel@...r.kernel.org, peterz@...radead.org,
mingo@...hat.com, alexander.shishkin@...ux.intel.com,
mpe@...erman.id.au, naveen.n.rao@...ux.vnet.ibm.com,
mhiramat@...nel.org, maddy@...ux.vnet.ibm.com
Subject: Re: [PATCH 3/5] perf/sdt/x86: Move OP parser to
tools/perf/arch/x86/
On Thu, 2 Feb 2017 16:41:41 +0530
Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com> wrote:
> SDT marker argument is in N@OP format. N is the size of argument and
> OP is the actual assembly operand. OP is arch dependent component and
> hence it's parsing logic also should be placed under tools/perf/arch/.
>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>
> ---
> tools/perf/arch/x86/util/perf_regs.c | 93 ++++++++++++++++++++++++-
> tools/perf/util/perf_regs.c | 9 ++-
> tools/perf/util/perf_regs.h | 7 +-
> tools/perf/util/probe-file.c | 127 +++++++++--------------------------
> 4 files changed, 134 insertions(+), 102 deletions(-)
>
> diff --git a/tools/perf/arch/x86/util/perf_regs.c b/tools/perf/arch/x86/util/perf_regs.c
> index d8a8dcf..34fcb0d 100644
> --- a/tools/perf/arch/x86/util/perf_regs.c
> +++ b/tools/perf/arch/x86/util/perf_regs.c
> @@ -3,6 +3,7 @@
> #include "../../perf.h"
> #include "../../util/util.h"
> #include "../../util/perf_regs.h"
> +#include "../../util/debug.h"
>
> const struct sample_reg sample_reg_masks[] = {
> SMPL_REG(AX, PERF_REG_X86_AX),
> @@ -87,7 +88,16 @@ static const struct sdt_name_reg sdt_reg_renamings[] = {
> SDT_NAME_REG_END,
> };
>
> -int sdt_rename_register(char **pdesc, char *old_name)
> +bool arch_sdt_probe_arg_supp(void)
> +{
> + return true;
> +}
> +
> +/*
> + * The table sdt_reg_renamings is used for adjusting gcc/gas-generated
> + * registers before filling the uprobe tracer interface.
> + */
> +static int sdt_rename_register(char **pdesc, char *old_name)
> {
> const struct sdt_name_reg *rnames = sdt_reg_renamings;
> char *new_desc, *old_desc = *pdesc;
> @@ -129,3 +139,84 @@ int sdt_rename_register(char **pdesc, char *old_name)
>
> return 0;
> }
> +
> +/*
> + * x86 specific implementation
> + * return value:
> + * <0 : error
> + * 0 : success
> + * >0 : skip
> + */
> +int arch_sdt_probe_parse_op(char **desc, const char **prefix)
Frankly to say, I prefer to see the function interface which has clearly
separated in/out arguments. This function, "desc" is used for both of
input and output.
Could you refactor this to separate args? for example,
int arch_sdt_probe_parse_op(const char *desc, struct strbuf *buf)
I think prefix also integrated with output buffer.
> +{
> + char *tmp;
> + int ret = 0;
> +
> + /*
> + * 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.
> + *
> + * Also it does not support constants; if we find one in the
> + * current argument, let's skip the argument.
> + */
> + if (strchr(*desc, ',') || strchr(*desc, '$')) {
> + pr_debug4("Skipping unsupported SDT argument; %s\n", *desc);
> + return 1;
> + }
> +
> + /*
> + * 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 = ((*desc)[0] == '(') ? "+0" : "+";
> +
> + /*
> + * ...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);
> + return 1;
> + }
> + }
> + }
> +
> + /*
> + * 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)
> + return ret;
> +
> + /*
> + * The *desc pointer might have changed; so, let's not
> + * try to reuse tmp for next lookup
> + */
> + tmp = strchr(*desc + offset + 1, '%');
> + }
> + return 0;
> +}
> diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c
> index a37e593..f2b3d0d 100644
> --- a/tools/perf/util/perf_regs.c
> +++ b/tools/perf/util/perf_regs.c
> @@ -6,8 +6,13 @@ 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)
> +bool __weak arch_sdt_probe_arg_supp(void)
> +{
> + return false;
> +}
> +
> +int __weak arch_sdt_probe_parse_op(char **op_ptr __maybe_unused,
> + const char **prefix __maybe_unused)
> {
> return 0;
> }
Hmm, why we we need both of these functions?
I think you just need to return 1 from arch_sdt_probe_parse_op(), then
all the arguments are skipped.
> diff --git a/tools/perf/util/perf_regs.h b/tools/perf/util/perf_regs.h
> index 7544a15..86a2961 100644
> --- a/tools/perf/util/perf_regs.h
> +++ b/tools/perf/util/perf_regs.h
> @@ -15,11 +15,8 @@ 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);
> +bool arch_sdt_probe_arg_supp(void);
> +int arch_sdt_probe_parse_op(char **op_ptr, const char **prefix);
>
> #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 d8a169e..38eca3c 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -693,6 +693,25 @@ static const char * const type_to_suffix[] = {
> "", ":u8", ":u16", "", ":u32", "", "", "", ":u64"
> };
>
> +/*
> + * Isolate the string number and convert it into a decimal value;
> + * this will be an index to get suffix of the uprobe name (defining
> + * the type)
> + */
> +static int sdt_probe_parse_n(char *n_ptr, const char **suffix)
please make it more meaningful name, sdt_probe_parse_size() etc.
Thank you,
> +{
> + long type_idx;
> +
> + type_idx = strtol(n_ptr, NULL, 10);
> + if (type_idx < -8 || type_idx > 8) {
> + pr_debug4("Failed to get a valid sdt type\n");
> + return -1;
> + }
> +
> + *suffix = type_to_suffix[type_idx + 8];
> + return 0;
> +}
> +
> static int synthesize_sdt_probe_arg(struct strbuf *buf, int i, const char *arg)
> {
> char *tmp, *desc = strdup(arg);
> @@ -704,109 +723,29 @@ static int synthesize_sdt_probe_arg(struct strbuf *buf, int i, const char *arg)
> return ret;
> }
>
> + /*
> + * Argument is in N@OP format. N is size of the argument and OP is
> + * the actual assembly operand. N can be omitted; in that case
> + * argument is just OP(without @).
> + */
> 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);
> - }
>
> - /*
> - * 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)
> + if (sdt_probe_parse_n(desc, &suffix))
> goto error;
> -
> - /*
> - * The desc pointer might have changed; so, let's not
> - * try to reuse tmp for next lookup
> - */
> - tmp = strchr(desc + offset + 1, '%');
> + memmove(desc, tmp, strlen(tmp) + 1);
> }
>
> - if (strbuf_addf(buf, " arg%d=%s%s%s", i + 1, prefix, desc, suffix) < 0)
> + ret = arch_sdt_probe_parse_op(&desc, &prefix);
> + if (ret < 0)
> + goto error;
> +
> + if (ret == 0 &&
> + strbuf_addf(buf, " arg%d=%s%s%s", i + 1, prefix, desc, suffix) < 0)
> goto error;
>
> -out:
> ret = 0;
> error:
> free(desc);
> @@ -829,7 +768,7 @@ static char *synthesize_sdt_probe_command(struct sdt_note *note,
> sdt_note__get_addr(note)) < 0)
> goto error;
>
> - if (!note->args)
> + if (!note->args || !arch_sdt_probe_arg_supp())
> goto out;
>
> if (note->args) {
> --
> 2.9.3
>
--
Masami Hiramatsu <mhiramat@...nel.org>
Powered by blists - more mailing lists