On Thu, 2 Feb 2017 16:41:41 +0530 Ravi Bangoria <[email protected]> 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 <[email protected]> > --- > 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 <[email protected]>
