lkml.org 
[lkml]   [2017]   [Mar]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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@linux.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@linux.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@kernel.org>

    \
     
     \ /
      Last update: 2017-03-21 16:07    [W:4.061 / U:0.156 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site