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