Date:	Tue, 26 Apr 2016 11:59:34 -0300
From:	Arnaldo Carvalho de Melo <acme@...nel.org>
To:	Masami Hiramatsu <mhiramat@...nel.org>
Cc:	linux-kernel@...r.kernel.org, Namhyung Kim <namhyung@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>,
	Hemant Kumar <hemant@...ux.vnet.ibm.com>,
	Ananth N Mavinakayanahalli <ananth@...ux.vnet.ibm.com>
Subject: Re: [PATCH perf/core v4 01/19] perf probe: Use strbuf for making
 strings

Em Tue, Apr 26, 2016 at 11:40:47PM +0900, Masami Hiramatsu escreveu:
> On Tue, 26 Apr 2016 10:36:57 -0300
> Arnaldo Carvalho de Melo <acme@...nel.org> wrote:
> 
> > Em Tue, Apr 26, 2016 at 06:02:11PM +0900, Masami Hiramatsu escreveu:
> > > From: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
> > > 
> > > Replace many fixed-length char array with strbuf to
> > > stringify perf_probe_event and probe_trace_event etc.
> > 
> > Sure you want to do that? From time to time I try to reduce the strbuf
> > usage, not grow it :-\
> 
> Aah, I got what you want.
> 
> > 
> > That is one of the last users of xrealloc(), via that ALLOC_GROW()
> > thing.
> 
> Hmm I'm not sure what you considering, memory usage? or code reducing?
> I also would like to remove e_snprintf() use in perf probe by replacing
> with strbuf functions. I'd like to find better way.

What I don't like is that xrealloc ends up calling die() :-\ It should
instead propagate back the error.

In lots of cases I managed to use what is in glibc, asprintf(), that
will allocate space and format in one go.

But strbuf has this addf() one that indeed is handy. So I think I'll
take this patch as it is not for the simpler cases where asprintf() can
be used.

- Arnaldo
 
> Thank you,
> 
> > 
> > - Arnaldo
> >  
> > > Signed-off-by: Masami Hiramatsu <mhiramat@...nel.org>
> > > Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
> > > 
> > > ---
> > > Changes in v4:
> > >  - Update for the latest kernel.
> > >  - Remove "probe-event.c" from title and body, because
> > >    this also changes probe-finder.c too :)
> > > 
> > > Changes in v3:
> > >  - Remove unneeded strbuf_release(). (Thanks Namhyung!)
> > > 
> > > Changes in v2:
> > >  - Make perf_probe_event__sprintf() simpler.
> > > ---
> > >  tools/perf/util/probe-event.c  |  246 ++++++++++++++--------------------------
> > >  tools/perf/util/probe-event.h  |    2 
> > >  tools/perf/util/probe-finder.c |   14 +-
> > >  3 files changed, 93 insertions(+), 169 deletions(-)
> > > 
> > > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > > index 8319fbb..148af81 100644
> > > --- a/tools/perf/util/probe-event.c
> > > +++ b/tools/perf/util/probe-event.c
> > > @@ -1618,69 +1618,51 @@ out:
> > >  }
> > >  
> > >  /* Compose only probe arg */
> > > -int synthesize_perf_probe_arg(struct perf_probe_arg *pa, char *buf, size_t len)
> > > +char *synthesize_perf_probe_arg(struct perf_probe_arg *pa)
> > >  {
> > >  	struct perf_probe_arg_field *field = pa->field;
> > > -	int ret;
> > > -	char *tmp = buf;
> > > +	struct strbuf buf;
> > > +	char *ret;
> > >  
> > > +	strbuf_init(&buf, 64);
> > >  	if (pa->name && pa->var)
> > > -		ret = e_snprintf(tmp, len, "%s=%s", pa->name, pa->var);
> > > +		strbuf_addf(&buf, "%s=%s", pa->name, pa->var);
> > >  	else
> > > -		ret = e_snprintf(tmp, len, "%s", pa->name ? pa->name : pa->var);
> > > -	if (ret <= 0)
> > > -		goto error;
> > > -	tmp += ret;
> > > -	len -= ret;
> > > +		strbuf_addstr(&buf, pa->name ?: pa->var);
> > >  
> > >  	while (field) {
> > >  		if (field->name[0] == '[')
> > > -			ret = e_snprintf(tmp, len, "%s", field->name);
> > > +			strbuf_addstr(&buf, field->name);
> > >  		else
> > > -			ret = e_snprintf(tmp, len, "%s%s",
> > > -					 field->ref ? "->" : ".", field->name);
> > > -		if (ret <= 0)
> > > -			goto error;
> > > -		tmp += ret;
> > > -		len -= ret;
> > > +			strbuf_addf(&buf, "%s%s", field->ref ? "->" : ".",
> > > +				    field->name);
> > >  		field = field->next;
> > >  	}
> > >  
> > > -	if (pa->type) {
> > > -		ret = e_snprintf(tmp, len, ":%s", pa->type);
> > > -		if (ret <= 0)
> > > -			goto error;
> > > -		tmp += ret;
> > > -		len -= ret;
> > > -	}
> > > +	if (pa->type)
> > > +		strbuf_addf(&buf, ":%s", pa->type);
> > > +
> > > +	ret = strbuf_detach(&buf, NULL);
> > >  
> > > -	return tmp - buf;
> > > -error:
> > > -	pr_debug("Failed to synthesize perf probe argument: %d\n", ret);
> > >  	return ret;
> > >  }
> > >  
> > >  /* Compose only probe point (not argument) */
> > >  static char *synthesize_perf_probe_point(struct perf_probe_point *pp)
> > >  {
> > > -	char *buf, *tmp;
> > > -	char offs[32] = "", line[32] = "", file[32] = "";
> > > -	int ret, len;
> > > -
> > > -	buf = zalloc(MAX_CMDLEN);
> > > -	if (buf == NULL) {
> > > -		ret = -ENOMEM;
> > > -		goto error;
> > > -	}
> > > -	if (pp->offset) {
> > > -		ret = e_snprintf(offs, 32, "+%lu", pp->offset);
> > > -		if (ret <= 0)
> > > -			goto error;
> > > -	}
> > > -	if (pp->line) {
> > > -		ret = e_snprintf(line, 32, ":%d", pp->line);
> > > -		if (ret <= 0)
> > > -			goto error;
> > > +	struct strbuf buf;
> > > +	char *tmp;
> > > +	int len;
> > > +
> > > +	strbuf_init(&buf, 64);
> > > +	if (pp->function) {
> > > +		strbuf_addstr(&buf, pp->function);
> > > +		if (pp->offset)
> > > +			strbuf_addf(&buf, "+%lu", pp->offset);
> > > +		else if (pp->line)
> > > +			strbuf_addf(&buf, ":%d", pp->line);
> > > +		else if (pp->retprobe)
> > > +			strbuf_addstr(&buf, "%return");
> > >  	}
> > >  	if (pp->file) {
> > >  		tmp = pp->file;
> > > @@ -1689,25 +1671,12 @@ static char *synthesize_perf_probe_point(struct perf_probe_point *pp)
> > >  			tmp = strchr(pp->file + len - 30, '/');
> > >  			tmp = tmp ? tmp + 1 : pp->file + len - 30;
> > >  		}
> > > -		ret = e_snprintf(file, 32, "@%s", tmp);
> > > -		if (ret <= 0)
> > > -			goto error;
> > > +		strbuf_addf(&buf, "@%s", tmp);
> > > +		if (!pp->function && pp->line)
> > > +			strbuf_addf(&buf, ":%d", pp->line);
> > >  	}
> > >  
> > > -	if (pp->function)
> > > -		ret = e_snprintf(buf, MAX_CMDLEN, "%s%s%s%s%s", pp->function,
> > > -				 offs, pp->retprobe ? "%return" : "", line,
> > > -				 file);
> > > -	else
> > > -		ret = e_snprintf(buf, MAX_CMDLEN, "%s%s", file, line);
> > > -	if (ret <= 0)
> > > -		goto error;
> > > -
> > > -	return buf;
> > > -error:
> > > -	pr_debug("Failed to synthesize perf probe point: %d\n", ret);
> > > -	free(buf);
> > > -	return NULL;
> > > +	return strbuf_detach(&buf, NULL);
> > >  }
> > >  
> > >  #if 0
> > > @@ -1736,45 +1705,30 @@ char *synthesize_perf_probe_command(struct perf_probe_event *pev)
> > >  #endif
> > >  
> > >  static int __synthesize_probe_trace_arg_ref(struct probe_trace_arg_ref *ref,
> > > -					     char **buf, size_t *buflen,
> > > -					     int depth)
> > > +					    struct strbuf *buf, int depth)
> > >  {
> > > -	int ret;
> > >  	if (ref->next) {
> > >  		depth = __synthesize_probe_trace_arg_ref(ref->next, buf,
> > > -							 buflen, depth + 1);
> > > +							 depth + 1);
> > >  		if (depth < 0)
> > >  			goto out;
> > >  	}
> > > -
> > > -	ret = e_snprintf(*buf, *buflen, "%+ld(", ref->offset);
> > > -	if (ret < 0)
> > > -		depth = ret;
> > > -	else {
> > > -		*buf += ret;
> > > -		*buflen -= ret;
> > > -	}
> > > +	strbuf_addf(buf, "%+ld(", ref->offset);
> > >  out:
> > >  	return depth;
> > > -
> > >  }
> > >  
> > >  static int synthesize_probe_trace_arg(struct probe_trace_arg *arg,
> > > -				       char *buf, size_t buflen)
> > > +				      struct strbuf *buf)
> > >  {
> > >  	struct probe_trace_arg_ref *ref = arg->ref;
> > > -	int ret, depth = 0;
> > > -	char *tmp = buf;
> > > +	int depth = 0;
> > >  
> > >  	/* Argument name or separator */
> > >  	if (arg->name)
> > > -		ret = e_snprintf(buf, buflen, " %s=", arg->name);
> > > +		strbuf_addf(buf, " %s=", arg->name);
> > >  	else
> > > -		ret = e_snprintf(buf, buflen, " ");
> > > -	if (ret < 0)
> > > -		return ret;
> > > -	buf += ret;
> > > -	buflen -= ret;
> > > +		strbuf_addch(buf, ' ');
> > >  
> > >  	/* Special case: @XXX */
> > >  	if (arg->value[0] == '@' && arg->ref)
> > > @@ -1782,60 +1736,41 @@ static int synthesize_probe_trace_arg(struct probe_trace_arg *arg,
> > >  
> > >  	/* Dereferencing arguments */
> > >  	if (ref) {
> > > -		depth = __synthesize_probe_trace_arg_ref(ref, &buf,
> > > -							  &buflen, 1);
> > > +		depth = __synthesize_probe_trace_arg_ref(ref, buf, 1);
> > >  		if (depth < 0)
> > >  			return depth;
> > >  	}
> > >  
> > >  	/* Print argument value */
> > >  	if (arg->value[0] == '@' && arg->ref)
> > > -		ret = e_snprintf(buf, buflen, "%s%+ld", arg->value,
> > > -				 arg->ref->offset);
> > > +		strbuf_addf(buf, "%s%+ld", arg->value, arg->ref->offset);
> > >  	else
> > > -		ret = e_snprintf(buf, buflen, "%s", arg->value);
> > > -	if (ret < 0)
> > > -		return ret;
> > > -	buf += ret;
> > > -	buflen -= ret;
> > > +		strbuf_addstr(buf, arg->value);
> > >  
> > >  	/* Closing */
> > > -	while (depth--) {
> > > -		ret = e_snprintf(buf, buflen, ")");
> > > -		if (ret < 0)
> > > -			return ret;
> > > -		buf += ret;
> > > -		buflen -= ret;
> > > -	}
> > > +	while (depth--)
> > > +		strbuf_addch(buf, ')');
> > >  	/* Print argument type */
> > > -	if (arg->type) {
> > > -		ret = e_snprintf(buf, buflen, ":%s", arg->type);
> > > -		if (ret <= 0)
> > > -			return ret;
> > > -		buf += ret;
> > > -	}
> > > +	if (arg->type)
> > > +		strbuf_addf(buf, ":%s", arg->type);
> > >  
> > > -	return buf - tmp;
> > > +	return 0;
> > >  }
> > >  
> > >  char *synthesize_probe_trace_command(struct probe_trace_event *tev)
> > >  {
> > >  	struct probe_trace_point *tp = &tev->point;
> > > -	char *buf;
> > > -	int i, len, ret;
> > > -
> > > -	buf = zalloc(MAX_CMDLEN);
> > > -	if (buf == NULL)
> > > -		return NULL;
> > > -
> > > -	len = e_snprintf(buf, MAX_CMDLEN, "%c:%s/%s ", tp->retprobe ? 'r' : 'p',
> > > -			 tev->group, tev->event);
> > > -	if (len <= 0)
> > > -		goto error;
> > > +	struct strbuf buf;
> > > +	char *ret = NULL;
> > > +	int i;
> > >  
> > >  	/* Uprobes must have tp->module */
> > >  	if (tev->uprobes && !tp->module)
> > > -		goto error;
> > > +		return NULL;
> > > +
> > > +	strbuf_init(&buf, 32);
> > > +	strbuf_addf(&buf, "%c:%s/%s ", tp->retprobe ? 'r' : 'p',
> > > +		    tev->group, tev->event);
> > >  	/*
> > >  	 * If tp->address == 0, then this point must be a
> > >  	 * absolute address uprobe.
> > > @@ -1849,34 +1784,23 @@ char *synthesize_probe_trace_command(struct probe_trace_event *tev)
> > >  
> > >  	/* Use the tp->address for uprobes */
> > >  	if (tev->uprobes)
> > > -		ret = e_snprintf(buf + len, MAX_CMDLEN - len, "%s:0x%lx",
> > > -				 tp->module, tp->address);
> > > +		strbuf_addf(&buf, "%s:0x%lx", tp->module, tp->address);
> > >  	else if (!strncmp(tp->symbol, "0x", 2))
> > >  		/* Absolute address. See try_to_find_absolute_address() */
> > > -		ret = e_snprintf(buf + len, MAX_CMDLEN - len, "%s%s0x%lx",
> > > -				 tp->module ?: "", tp->module ? ":" : "",
> > > -				 tp->address);
> > > +		strbuf_addf(&buf, "%s%s0x%lx", tp->module ?: "",
> > > +			    tp->module ? ":" : "", tp->address);
> > >  	else
> > > -		ret = e_snprintf(buf + len, MAX_CMDLEN - len, "%s%s%s+%lu",
> > > -				 tp->module ?: "", tp->module ? ":" : "",
> > > -				 tp->symbol, tp->offset);
> > > -
> > > -	if (ret <= 0)
> > > -		goto error;
> > > -	len += ret;
> > > +		strbuf_addf(&buf, "%s%s%s+%lu", tp->module ?: "",
> > > +			    tp->module ? ":" : "", tp->symbol, tp->offset);
> > >  
> > > -	for (i = 0; i < tev->nargs; i++) {
> > > -		ret = synthesize_probe_trace_arg(&tev->args[i], buf + len,
> > > -						  MAX_CMDLEN - len);
> > > -		if (ret <= 0)
> > > +	for (i = 0; i < tev->nargs; i++)
> > > +		if (synthesize_probe_trace_arg(&tev->args[i], &buf) < 0)
> > >  			goto error;
> > > -		len += ret;
> > > -	}
> > >  
> > > -	return buf;
> > > +	ret = strbuf_detach(&buf, NULL);
> > >  error:
> > > -	free(buf);
> > > -	return NULL;
> > > +	strbuf_release(&buf);
> > > +	return ret;
> > >  }
> > >  
> > >  static int find_perf_probe_point_from_map(struct probe_trace_point *tp,
> > > @@ -1958,7 +1882,7 @@ static int convert_to_perf_probe_point(struct probe_trace_point *tp,
> > >  static int convert_to_perf_probe_event(struct probe_trace_event *tev,
> > >  			       struct perf_probe_event *pev, bool is_kprobe)
> > >  {
> > > -	char buf[64] = "";
> > > +	struct strbuf buf = STRBUF_INIT;
> > >  	int i, ret;
> > >  
> > >  	/* Convert event/group name */
> > > @@ -1981,9 +1905,9 @@ static int convert_to_perf_probe_event(struct probe_trace_event *tev,
> > >  		if (tev->args[i].name)
> > >  			pev->args[i].name = strdup(tev->args[i].name);
> > >  		else {
> > > -			ret = synthesize_probe_trace_arg(&tev->args[i],
> > > -							  buf, 64);
> > > -			pev->args[i].name = strdup(buf);
> > > +			strbuf_init(&buf, 32);
> > > +			ret = synthesize_probe_trace_arg(&tev->args[i], &buf);
> > > +			pev->args[i].name = strbuf_detach(&buf, NULL);
> > >  		}
> > >  		if (pev->args[i].name == NULL && ret >= 0)
> > >  			ret = -ENOMEM;
> > > @@ -2161,37 +2085,37 @@ static int perf_probe_event__sprintf(const char *group, const char *event,
> > >  				     const char *module,
> > >  				     struct strbuf *result)
> > >  {
> > > -	int i, ret;
> > > -	char buf[128];
> > > -	char *place;
> > > +	int i;
> > > +	char *buf;
> > >  
> > > -	/* Synthesize only event probe point */
> > > -	place = synthesize_perf_probe_point(&pev->point);
> > > -	if (!place)
> > > -		return -EINVAL;
> > > +	if (asprintf(&buf, "%s:%s", group, event) < 0)
> > > +		return -errno;
> > > +	strbuf_addf(result, "  %-20s (on ", buf);
> > > +	free(buf);
> > >  
> > > -	ret = e_snprintf(buf, 128, "%s:%s", group, event);
> > > -	if (ret < 0)
> > > -		goto out;
> > > +	/* Synthesize only event probe point */
> > > +	buf = synthesize_perf_probe_point(&pev->point);
> > > +	if (!buf)
> > > +		return -ENOMEM;
> > > +	strbuf_addstr(result, buf);
> > > +	free(buf);
> > >  
> > > -	strbuf_addf(result, "  %-20s (on %s", buf, place);
> > >  	if (module)
> > >  		strbuf_addf(result, " in %s", module);
> > >  
> > >  	if (pev->nargs > 0) {
> > >  		strbuf_add(result, " with", 5);
> > >  		for (i = 0; i < pev->nargs; i++) {
> > > -			ret = synthesize_perf_probe_arg(&pev->args[i],
> > > -							buf, 128);
> > > -			if (ret < 0)
> > > -				goto out;
> > > +			buf = synthesize_perf_probe_arg(&pev->args[i]);
> > > +			if (!buf)
> > > +				return -ENOMEM;
> > >  			strbuf_addf(result, " %s", buf);
> > > +			free(buf);
> > >  		}
> > >  	}
> > >  	strbuf_addch(result, ')');
> > > -out:
> > > -	free(place);
> > > -	return ret;
> > > +
> > > +	return 0;
> > >  }
> > >  
> > >  /* Show an event */
> > > diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
> > > index e54e7b0..e220962 100644
> > > --- a/tools/perf/util/probe-event.h
> > > +++ b/tools/perf/util/probe-event.h
> > > @@ -120,7 +120,7 @@ int parse_probe_trace_command(const char *cmd, struct probe_trace_event *tev);
> > >  /* Events to command string */
> > >  char *synthesize_perf_probe_command(struct perf_probe_event *pev);
> > >  char *synthesize_probe_trace_command(struct probe_trace_event *tev);
> > > -int synthesize_perf_probe_arg(struct perf_probe_arg *pa, char *buf, size_t len);
> > > +char *synthesize_perf_probe_arg(struct perf_probe_arg *pa);
> > >  
> > >  /* Check the perf_probe_event needs debuginfo */
> > >  bool perf_probe_event_need_dwarf(struct perf_probe_event *pev);
> > > diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> > > index b3bd0fb..9f68875 100644
> > > --- a/tools/perf/util/probe-finder.c
> > > +++ b/tools/perf/util/probe-finder.c
> > > @@ -553,7 +553,7 @@ static int convert_variable(Dwarf_Die *vr_die, struct probe_finder *pf)
> > >  static int find_variable(Dwarf_Die *sc_die, struct probe_finder *pf)
> > >  {
> > >  	Dwarf_Die vr_die;
> > > -	char buf[32], *ptr;
> > > +	char *buf, *ptr;
> > >  	int ret = 0;
> > >  
> > >  	/* Copy raw parameters */
> > > @@ -563,13 +563,13 @@ static int find_variable(Dwarf_Die *sc_die, struct probe_finder *pf)
> > >  	if (pf->pvar->name)
> > >  		pf->tvar->name = strdup(pf->pvar->name);
> > >  	else {
> > > -		ret = synthesize_perf_probe_arg(pf->pvar, buf, 32);
> > > -		if (ret < 0)
> > > -			return ret;
> > > +		buf = synthesize_perf_probe_arg(pf->pvar);
> > > +		if (!buf)
> > > +			return -ENOMEM;
> > >  		ptr = strchr(buf, ':');	/* Change type separator to _ */
> > >  		if (ptr)
> > >  			*ptr = '_';
> > > -		pf->tvar->name = strdup(buf);
> > > +		pf->tvar->name = buf;
> > >  	}
> > >  	if (pf->tvar->name == NULL)
> > >  		return -ENOMEM;
> > > @@ -1334,8 +1334,8 @@ static int collect_variables_cb(Dwarf_Die *die_mem, void *data)
> > >  			if (ret2 == 0) {
> > >  				strlist__add(vl->vars,
> > >  					strbuf_detach(&buf, NULL));
> > > -			}
> > > -			strbuf_release(&buf);
> > > +			} else
> > > +				strbuf_release(&buf);
> > >  		}
> > >  	}
> > >  
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@...nel.org>

Powered by blists - more mailing lists