perf-probe --cache and SDT support

From: Masami Hiramatsu <[email protected]>

Replace many fixed-length char array with strbuf to stringify perf_probe_event and probe_trace_event etc.

Signed-off-by Masami Hiramatsu <[email protected]>

Signed-off-by Masami Hiramatsu <[email protected]>

---
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(-)

tools/perf/util/probe-event.c +85 -161

index 8319fbb..148af81 100644
@@ -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 */

tools/perf/util/probe-event.h +1 -1

index e54e7b0..e220962 100644
@@ -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);

tools/perf/util/probe-finder.c +7 -7

index b3bd0fb..9f68875 100644
@@ -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);
 		}
 	}
 

Em Tue, Apr 26, 2016 at 06:02:11PM +0900, Masami Hiramatsu escreveu:

Quoted 4 lines

From: Masami Hiramatsu <[email protected]>

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 :-\

That is one of the last users of xrealloc(), via that ALLOC_GROW() thing.

- Arnaldo

Quoted 458 lines

Signed-off-by Masami Hiramatsu <[email protected]>

Signed-off-by Masami Hiramatsu <[email protected]>

---
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(-)

tools/perf/util/probe-event.c +85 -161

index 8319fbb..148af81 100644
@@ -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 */

tools/perf/util/probe-event.h +1 -1

index e54e7b0..e220962 100644
@@ -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);

tools/perf/util/probe-finder.c +7 -7

index b3bd0fb..9f68875 100644
@@ -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);
 		}
 	}
 

Quoted 8 lines · On Tue, 26 Apr 2016 10:36:57 -0300 Arnaldo Carvalho de Melo <[email protected]>…

Em Tue, Apr 26, 2016 at 06:02:11PM +0900, Masami Hiramatsu escreveu:

Quoted 4 lines

From: Masami Hiramatsu <[email protected]>

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.

Quoted 1 line

Quoted 2 lines

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.

Thank you,

Quoted 461 lines

- Arnaldo

Quoted 458 lines

Signed-off-by Masami Hiramatsu <[email protected]>

Signed-off-by Masami Hiramatsu <[email protected]>

---
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(-)

tools/perf/util/probe-event.c +85 -161

index 8319fbb..148af81 100644
@@ -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 */

tools/perf/util/probe-event.h +1 -1

index e54e7b0..e220962 100644
@@ -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);

tools/perf/util/probe-finder.c +7 -7

index b3bd0fb..9f68875 100644
@@ -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);
 		}
 	}
 

Signature · 1 line

Masami Hiramatsu <[email protected]>

Em Tue, Apr 26, 2016 at 11:40:47PM +0900, Masami Hiramatsu escreveu:

Quoted 15 lines

Quoted 8 lines · On Tue, 26 Apr 2016 10:36:57 -0300 Arnaldo Carvalho de Melo <[email protected]>…

Em Tue, Apr 26, 2016 at 06:02:11PM +0900, Masami Hiramatsu escreveu:

Quoted 4 lines

From: Masami Hiramatsu <[email protected]>

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.

Quoted 1 line

Quoted 6 lines

Quoted 2 lines

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.

From: Masami Hiramatsu <[email protected]>

Use path/to/bin/buildid/elf instead of path/to/bin/buildid to store corresponding elf binary.
This also stores vdso in buildid/vdso, kallsyms in buildid/kallsyms. Note that the build-id based symlink changes to point to the path/to/bin/buildid, not path/to/bin/buildid/elf.

Signed-off-by Masami Hiramatsu <[email protected]>

Signed-off-by Masami Hiramatsu <[email protected]>

---
 tools/perf/util/build-id.c |   65 +++++++++++++++++++++++++++++++-------------
 tools/perf/util/dso.h      |    5 +++
 tools/perf/util/symbol.c   |    2 +
 3 files changed, 52 insertions(+), 20 deletions(-)

tools/perf/util/build-id.c +46 -19

index b6ecf87..46a8bcc 100644
@@ -144,7 +144,8 @@ static int asnprintf(char **strp, size_t size, const char *fmt, ...)
 	return ret;
 }
 
-static char *build_id__filename(const char *sbuild_id, char *bf, size_t size)
+static char *build_id_cache__linkname(const char *sbuild_id, char *bf,
+				      size_t size)
 {
 	char *tmp = bf;
 	int ret = asnprintf(&bf, size, "%s/.build-id/%.2s/%s", buildid_dir,

@@ -154,15 +155,35 @@ static char *build_id__filename(const char *sbuild_id, char *bf, size_t size)
 	return bf;
 }
 
+static const char *build_id_cache__basename(bool is_kallsyms, bool is_vdso)
+{
+	return is_kallsyms ? "kallsyms" : (is_vdso ? "vdso" : "elf");
+}
+
 char *dso__build_id_filename(const struct dso *dso, char *bf, size_t size)
 {
-	char build_id_hex[SBUILD_ID_SIZE];
+	bool is_kallsyms = dso__is_kallsyms((struct dso *)dso);
+	bool is_vdso = dso__is_vdso((struct dso *)dso);
+	char sbuild_id[SBUILD_ID_SIZE];
+	char *linkname;
+	bool alloc = (bf == NULL);
+	int ret;
 
 	if (!dso->has_build_id)
 		return NULL;
 
-	build_id__sprintf(dso->build_id, sizeof(dso->build_id), build_id_hex);
-	return build_id__filename(build_id_hex, bf, size);
+	build_id__sprintf(dso->build_id, sizeof(dso->build_id), sbuild_id);
+	linkname = build_id_cache__linkname(sbuild_id, NULL, 0);
+	if (!linkname)
+		return NULL;
+
+	ret = asnprintf(&bf, size, "%s/%s", linkname,
+			 build_id_cache__basename(is_kallsyms, is_vdso));
+	if (ret < 0 || (!alloc && size < (unsigned int)ret))
+		bf = NULL;
+	free(linkname);
+
+	return bf;
 }
 
 bool dso__build_id_is_kmod(const struct dso *dso, char *bf, size_t size)

@@ -341,7 +362,8 @@ void disable_buildid_cache(void)
 }
 
 static char *build_id_cache__dirname_from_path(const char *name,
-					       bool is_kallsyms, bool is_vdso)
+					       bool is_kallsyms, bool is_vdso,
+					       const char *sbuild_id)
 {
 	char *realname = (char *)name, *filename;
 	bool slash = is_kallsyms || is_vdso;

@@ -352,8 +374,9 @@ static char *build_id_cache__dirname_from_path(const char *name,
 			return NULL;
 	}
 
-	if (asprintf(&filename, "%s%s%s", buildid_dir, slash ? "/" : "",
-		     is_vdso ? DSO__NAME_VDSO : realname) < 0)
+	if (asprintf(&filename, "%s%s%s%s%s", buildid_dir, slash ? "/" : "",
+		     is_vdso ? DSO__NAME_VDSO : realname,
+		     sbuild_id ? "/" : "", sbuild_id ?: "") < 0)
 		filename = NULL;
 
 	if (!slash)

@@ -372,7 +395,8 @@ int build_id_cache__list_build_ids(const char *pathname,
 	int ret = 0;
 
 	list = strlist__new(NULL, NULL);
-	dir_name = build_id_cache__dirname_from_path(pathname, false, false);
+	dir_name = build_id_cache__dirname_from_path(pathname, false, false,
+						     NULL);
 	if (!list || !dir_name) {
 		ret = -ENOMEM;
 		goto out;

@@ -407,7 +431,7 @@ int build_id_cache__add_s(const char *sbuild_id, const char *name,
 {
 	const size_t size = PATH_MAX;
 	char *realname = NULL, *filename = NULL, *dir_name = NULL,
-	     *linkname = zalloc(size), *targetname, *tmp;
+	     *linkname = zalloc(size), *tmp;
 	int err = -1;
 
 	if (!is_kallsyms) {

@@ -416,14 +440,17 @@ int build_id_cache__add_s(const char *sbuild_id, const char *name,
 			goto out_free;
 	}
 
-	dir_name = build_id_cache__dirname_from_path(name, is_kallsyms, is_vdso);
+	dir_name = build_id_cache__dirname_from_path(name, is_kallsyms,
+						     is_vdso, sbuild_id);
 	if (!dir_name)
 		goto out_free;
 
 	if (mkdir_p(dir_name, 0755))
 		goto out_free;
 
-	if (asprintf(&filename, "%s/%s", dir_name, sbuild_id) < 0) {
+	/* Save the allocated buildid dirname */
+	if (asprintf(&filename, "%s/%s", dir_name,
+		     build_id_cache__basename(is_kallsyms, is_vdso)) < 0) {
 		filename = NULL;
 		goto out_free;
 	}

@@ -437,7 +464,7 @@ int build_id_cache__add_s(const char *sbuild_id, const char *name,
 			goto out_free;
 	}
 
-	if (!build_id__filename(sbuild_id, linkname, size))
+	if (!build_id_cache__linkname(sbuild_id, linkname, size))
 		goto out_free;
 	tmp = strrchr(linkname, '/');
 	*tmp = '\0';

@@ -446,10 +473,10 @@ int build_id_cache__add_s(const char *sbuild_id, const char *name,
 		goto out_free;
 
 	*tmp = '/';
-	targetname = filename + strlen(buildid_dir) - 5;
-	memcpy(targetname, "../..", 5);
+	tmp = dir_name + strlen(buildid_dir) - 5;
+	memcpy(tmp, "../..", 5);
 
-	if (symlink(targetname, linkname) == 0)
+	if (symlink(tmp, linkname) == 0)
 		err = 0;
 out_free:
 	if (!is_kallsyms)

@@ -474,7 +501,7 @@ static int build_id_cache__add_b(const u8 *build_id, size_t build_id_size,
 bool build_id_cache__cached(const char *sbuild_id)
 {
 	bool ret = false;
-	char *filename = build_id__filename(sbuild_id, NULL, 0);
+	char *filename = build_id_cache__linkname(sbuild_id, NULL, 0);
 
 	if (filename && !access(filename, F_OK))
 		ret = true;

@@ -493,7 +520,7 @@ int build_id_cache__remove_s(const char *sbuild_id)
 	if (filename == NULL || linkname == NULL)
 		goto out_free;
 
-	if (!build_id__filename(sbuild_id, linkname, size))
+	if (!build_id_cache__linkname(sbuild_id, linkname, size))
 		goto out_free;
 
 	if (access(linkname, F_OK))

@@ -511,7 +538,7 @@ int build_id_cache__remove_s(const char *sbuild_id)
 	tmp = strrchr(linkname, '/') + 1;
 	snprintf(tmp, size - (tmp - linkname), "%s", filename);
 
-	if (unlink(linkname))
+	if (rm_rf(linkname))
 		goto out_free;
 
 	err = 0;

@@ -523,7 +550,7 @@ out_free:
 
 static int dso__cache_build_id(struct dso *dso, struct machine *machine)
 {
-	bool is_kallsyms = dso->kernel && dso->long_name[0] != '/';
+	bool is_kallsyms = dso__is_kallsyms(dso);
 	bool is_vdso = dso__is_vdso(dso);
 	const char *name = dso->long_name;
 	char nm[PATH_MAX];

tools/perf/util/dso.h +5 -0

index 0953280..76d79d0 100644
@@ -349,6 +349,11 @@ static inline bool dso__is_kcore(struct dso *dso)
 	       dso->binary_type == DSO_BINARY_TYPE__GUEST_KCORE;
 }
 
+static inline bool dso__is_kallsyms(struct dso *dso)
+{
+	return dso->kernel && dso->long_name[0] != '/';
+}
+
 void dso__free_a2l(struct dso *dso);
 
 enum dso_type dso__type(struct dso *dso, struct machine *machine);

tools/perf/util/symbol.c +1 -1

index 415c4f6..c57cb47 100644
@@ -1685,7 +1685,7 @@ static char *dso__find_kallsyms(struct dso *dso, struct map *map)
 	if (!find_matching_kcore(map, path, sizeof(path)))
 		return strdup(path);
 
-	scnprintf(path, sizeof(path), "%s/[kernel.kallsyms]/%s",
+	scnprintf(path, sizeof(path), "%s/[kernel.kallsyms]/%s/kallsyms",
 		  buildid_dir, sbuild_id);
 
 	if (access(path, F_OK)) {

Em Tue, Apr 26, 2016 at 06:02:22PM +0900, Masami Hiramatsu escreveu:

Quoted 7 lines

From: Masami Hiramatsu <[email protected]>

Use path/to/bin/buildid/elf instead of path/to/bin/buildid to store corresponding elf binary.
This also stores vdso in buildid/vdso, kallsyms in buildid/kallsyms. Note that the build-id based symlink changes to point to the path/to/bin/buildid, not path/to/bin/buildid/elf.

Ok, but what happens to existing caches? Not a problem? I.e. the lookups are made from ~/.debug/.build-id/, so it will just follow that symlink and if the file was inserted in the cache before this change, it will find it there, ok.

If inserted later, ditto.

Now what do you mean by "Note that the build-id based symlink changes to point to the path/to/bin/buildid, not path/to/bin/buildid/elf."?

Probably the 'perf buildid-cache --purge" will just follow the symlink, so it'll work.

- Arnaldo

Quoted 210 lines

Signed-off-by Masami Hiramatsu <[email protected]>

Signed-off-by Masami Hiramatsu <[email protected]>

---
 tools/perf/util/build-id.c |   65 +++++++++++++++++++++++++++++++-------------
 tools/perf/util/dso.h      |    5 +++
 tools/perf/util/symbol.c   |    2 +
 3 files changed, 52 insertions(+), 20 deletions(-)

tools/perf/util/build-id.c +46 -19

index b6ecf87..46a8bcc 100644
@@ -144,7 +144,8 @@ static int asnprintf(char **strp, size_t size, const char *fmt, ...)
 	return ret;
 }
 
-static char *build_id__filename(const char *sbuild_id, char *bf, size_t size)
+static char *build_id_cache__linkname(const char *sbuild_id, char *bf,
+				      size_t size)
 {
 	char *tmp = bf;
 	int ret = asnprintf(&bf, size, "%s/.build-id/%.2s/%s", buildid_dir,

@@ -154,15 +155,35 @@ static char *build_id__filename(const char *sbuild_id, char *bf, size_t size)
 	return bf;
 }
 
+static const char *build_id_cache__basename(bool is_kallsyms, bool is_vdso)
+{
+	return is_kallsyms ? "kallsyms" : (is_vdso ? "vdso" : "elf");
+}
+
 char *dso__build_id_filename(const struct dso *dso, char *bf, size_t size)
 {
-	char build_id_hex[SBUILD_ID_SIZE];
+	bool is_kallsyms = dso__is_kallsyms((struct dso *)dso);
+	bool is_vdso = dso__is_vdso((struct dso *)dso);
+	char sbuild_id[SBUILD_ID_SIZE];
+	char *linkname;
+	bool alloc = (bf == NULL);
+	int ret;
 
 	if (!dso->has_build_id)
 		return NULL;
 
-	build_id__sprintf(dso->build_id, sizeof(dso->build_id), build_id_hex);
-	return build_id__filename(build_id_hex, bf, size);
+	build_id__sprintf(dso->build_id, sizeof(dso->build_id), sbuild_id);
+	linkname = build_id_cache__linkname(sbuild_id, NULL, 0);
+	if (!linkname)
+		return NULL;
+
+	ret = asnprintf(&bf, size, "%s/%s", linkname,
+			 build_id_cache__basename(is_kallsyms, is_vdso));
+	if (ret < 0 || (!alloc && size < (unsigned int)ret))
+		bf = NULL;
+	free(linkname);
+
+	return bf;
 }
 
 bool dso__build_id_is_kmod(const struct dso *dso, char *bf, size_t size)

@@ -341,7 +362,8 @@ void disable_buildid_cache(void)
 }
 
 static char *build_id_cache__dirname_from_path(const char *name,
-					       bool is_kallsyms, bool is_vdso)
+					       bool is_kallsyms, bool is_vdso,
+					       const char *sbuild_id)
 {
 	char *realname = (char *)name, *filename;
 	bool slash = is_kallsyms || is_vdso;

@@ -352,8 +374,9 @@ static char *build_id_cache__dirname_from_path(const char *name,
 			return NULL;
 	}
 
-	if (asprintf(&filename, "%s%s%s", buildid_dir, slash ? "/" : "",
-		     is_vdso ? DSO__NAME_VDSO : realname) < 0)
+	if (asprintf(&filename, "%s%s%s%s%s", buildid_dir, slash ? "/" : "",
+		     is_vdso ? DSO__NAME_VDSO : realname,
+		     sbuild_id ? "/" : "", sbuild_id ?: "") < 0)
 		filename = NULL;
 
 	if (!slash)

@@ -372,7 +395,8 @@ int build_id_cache__list_build_ids(const char *pathname,
 	int ret = 0;
 
 	list = strlist__new(NULL, NULL);
-	dir_name = build_id_cache__dirname_from_path(pathname, false, false);
+	dir_name = build_id_cache__dirname_from_path(pathname, false, false,
+						     NULL);
 	if (!list || !dir_name) {
 		ret = -ENOMEM;
 		goto out;

@@ -407,7 +431,7 @@ int build_id_cache__add_s(const char *sbuild_id, const char *name,
 {
 	const size_t size = PATH_MAX;
 	char *realname = NULL, *filename = NULL, *dir_name = NULL,
-	     *linkname = zalloc(size), *targetname, *tmp;
+	     *linkname = zalloc(size), *tmp;
 	int err = -1;
 
 	if (!is_kallsyms) {

@@ -416,14 +440,17 @@ int build_id_cache__add_s(const char *sbuild_id, const char *name,
 			goto out_free;
 	}
 
-	dir_name = build_id_cache__dirname_from_path(name, is_kallsyms, is_vdso);
+	dir_name = build_id_cache__dirname_from_path(name, is_kallsyms,
+						     is_vdso, sbuild_id);
 	if (!dir_name)
 		goto out_free;
 
 	if (mkdir_p(dir_name, 0755))
 		goto out_free;
 
-	if (asprintf(&filename, "%s/%s", dir_name, sbuild_id) < 0) {
+	/* Save the allocated buildid dirname */
+	if (asprintf(&filename, "%s/%s", dir_name,
+		     build_id_cache__basename(is_kallsyms, is_vdso)) < 0) {
 		filename = NULL;
 		goto out_free;
 	}

@@ -437,7 +464,7 @@ int build_id_cache__add_s(const char *sbuild_id, const char *name,
 			goto out_free;
 	}
 
-	if (!build_id__filename(sbuild_id, linkname, size))
+	if (!build_id_cache__linkname(sbuild_id, linkname, size))
 		goto out_free;
 	tmp = strrchr(linkname, '/');
 	*tmp = '\0';

@@ -446,10 +473,10 @@ int build_id_cache__add_s(const char *sbuild_id, const char *name,
 		goto out_free;
 
 	*tmp = '/';
-	targetname = filename + strlen(buildid_dir) - 5;
-	memcpy(targetname, "../..", 5);
+	tmp = dir_name + strlen(buildid_dir) - 5;
+	memcpy(tmp, "../..", 5);
 
-	if (symlink(targetname, linkname) == 0)
+	if (symlink(tmp, linkname) == 0)
 		err = 0;
 out_free:
 	if (!is_kallsyms)

@@ -474,7 +501,7 @@ static int build_id_cache__add_b(const u8 *build_id, size_t build_id_size,
 bool build_id_cache__cached(const char *sbuild_id)
 {
 	bool ret = false;
-	char *filename = build_id__filename(sbuild_id, NULL, 0);
+	char *filename = build_id_cache__linkname(sbuild_id, NULL, 0);
 
 	if (filename && !access(filename, F_OK))
 		ret = true;

@@ -493,7 +520,7 @@ int build_id_cache__remove_s(const char *sbuild_id)
 	if (filename == NULL || linkname == NULL)
 		goto out_free;
 
-	if (!build_id__filename(sbuild_id, linkname, size))
+	if (!build_id_cache__linkname(sbuild_id, linkname, size))
 		goto out_free;
 
 	if (access(linkname, F_OK))

@@ -511,7 +538,7 @@ int build_id_cache__remove_s(const char *sbuild_id)
 	tmp = strrchr(linkname, '/') + 1;
 	snprintf(tmp, size - (tmp - linkname), "%s", filename);
 
-	if (unlink(linkname))
+	if (rm_rf(linkname))
 		goto out_free;
 
 	err = 0;

@@ -523,7 +550,7 @@ out_free:
 
 static int dso__cache_build_id(struct dso *dso, struct machine *machine)
 {
-	bool is_kallsyms = dso->kernel && dso->long_name[0] != '/';
+	bool is_kallsyms = dso__is_kallsyms(dso);
 	bool is_vdso = dso__is_vdso(dso);
 	const char *name = dso->long_name;
 	char nm[PATH_MAX];

tools/perf/util/dso.h +5 -0

index 0953280..76d79d0 100644
@@ -349,6 +349,11 @@ static inline bool dso__is_kcore(struct dso *dso)
 	       dso->binary_type == DSO_BINARY_TYPE__GUEST_KCORE;
 }
 
+static inline bool dso__is_kallsyms(struct dso *dso)
+{
+	return dso->kernel && dso->long_name[0] != '/';
+}
+
 void dso__free_a2l(struct dso *dso);
 
 enum dso_type dso__type(struct dso *dso, struct machine *machine);

tools/perf/util/symbol.c +1 -1

index 415c4f6..c57cb47 100644
@@ -1685,7 +1685,7 @@ static char *dso__find_kallsyms(struct dso *dso, struct map *map)
 	if (!find_matching_kcore(map, path, sizeof(path)))
 		return strdup(path);
 
-	scnprintf(path, sizeof(path), "%s/[kernel.kallsyms]/%s",
+	scnprintf(path, sizeof(path), "%s/[kernel.kallsyms]/%s/kallsyms",
 		  buildid_dir, sbuild_id);
 
 	if (access(path, F_OK)) {

Fall back to the old style build-id cache (~/.debug/<path-to-bin>/<build-id>) if possible. Because there is old build-id cache under ~/.debug, perf has to check if the old entry is there instead of returning an error. Or, user have to remove ~/.debug completely.

This patch introduces fallback code to try using old-style buildid caches. To update the cached entry, you just need to add a cache. It automatically removes old cache entry and make a new one in new style (~/.debug/<path-to-bin>/<build-id>/[elf|vdso|kallsyms]).

IOW, without this patch, "perf buildid-cache --add" always gets errors when adding existing binaries.
----

  # perf buildid-cache -a /usr/lib64/libc-2.20.so,/usr/bin/gcc
  Couldn't add /usr/bin/gcc: Not a directory
  Couldn't add /usr/lib64/libc-2.20.so: Not a directory
  ----
Moreover, --update option removes only symlink, thus the
build-id cache is corrupted.

This fixes above issues.

Signed-off-by Masami Hiramatsu <[email protected]>

---
 tools/perf/util/build-id.c |   17 ++++++++++++++++-
 tools/perf/util/symbol.c   |   13 +++++++++----
 2 files changed, 25 insertions(+), 5 deletions(-)

tools/perf/util/build-id.c +16 -1

index 46a8bcc..b035483 100644
@@ -155,6 +155,12 @@ static char *build_id_cache__linkname(const char *sbuild_id, char *bf,
 	return bf;
 }
 
+static bool __is_regular_file(const char *pathname)
+{
+	struct stat sb;
+	return stat(pathname, &sb) == 0 && S_ISREG(sb.st_mode);
+}
+
 static const char *build_id_cache__basename(bool is_kallsyms, bool is_vdso)
 {
 	return is_kallsyms ? "kallsyms" : (is_vdso ? "vdso" : "elf");

@@ -177,7 +183,11 @@ char *dso__build_id_filename(const struct dso *dso, char *bf, size_t size)
 	if (!linkname)
 		return NULL;
 
-	ret = asnprintf(&bf, size, "%s/%s", linkname,
+	/* Check if old style build_id cache */
+	if (__is_regular_file(linkname))
+		ret = asnprintf(&bf, size, "%s", linkname);
+	else
+		ret = asnprintf(&bf, size, "%s/%s", linkname,
 			 build_id_cache__basename(is_kallsyms, is_vdso));
 	if (ret < 0 || (!alloc && size < (unsigned int)ret))
 		bf = NULL;

@@ -445,6 +455,11 @@ int build_id_cache__add_s(const char *sbuild_id, const char *name,
 	if (!dir_name)
 		goto out_free;
 
+	/* Remove old style build-id cache */
+	if (__is_regular_file(dir_name))
+		if (unlink(dir_name))
+			goto out_free;
+
 	if (mkdir_p(dir_name, 0755))
 		goto out_free;
 

tools/perf/util/symbol.c +9 -4

index c57cb47..9463c7d 100644
@@ -1687,11 +1687,16 @@ static char *dso__find_kallsyms(struct dso *dso, struct map *map)
 
 	scnprintf(path, sizeof(path), "%s/[kernel.kallsyms]/%s/kallsyms",
 		  buildid_dir, sbuild_id);
-
+	/* Try old style kallsyms cache */
 	if (access(path, F_OK)) {
-		pr_err("No kallsyms or vmlinux with build-id %s was found\n",
-		       sbuild_id);
-		return NULL;
+		scnprintf(path, sizeof(path), "%s/[kernel.kallsyms]/%s",
+			  buildid_dir, sbuild_id);
+
+		if (access(path, F_OK)) {
+			pr_err("No kallsyms or vmlinux with build-id %s was found\n",
+			       sbuild_id);
+			return NULL;
+		}
 	}
 
 	return strdup(path);

Em Tue, Apr 26, 2016 at 06:02:33PM +0900, Masami Hiramatsu escreveu:

Quoted 5 lines

Fall back to the old style build-id cache (~/.debug/<path-to-bin>/<build-id>) if possible. Because there is old build-id cache under ~/.debug, perf has to check if the old entry is there instead of returning an error. Or, user have to remove ~/.debug completely.

So, from the description this seems to address the issues I mentioned in the previous patch.

Probably it is a good idea not to introduce the problem in the first place? I.e. combine this and the previous patch, so that we don't have this bisection break?

- Arnaldo

Quoted 91 lines

This patch introduces fallback code to try using old-style buildid caches. To update the cached entry, you just need to add a cache. It automatically removes old cache entry and make a new one in new style (~/.debug/<path-to-bin>/<build-id>/[elf|vdso|kallsyms]).

IOW, without this patch, "perf buildid-cache --add" always gets errors when adding existing binaries.
----

  # perf buildid-cache -a /usr/lib64/libc-2.20.so,/usr/bin/gcc
  Couldn't add /usr/bin/gcc: Not a directory
  Couldn't add /usr/lib64/libc-2.20.so: Not a directory
  ----
Moreover, --update option removes only symlink, thus the
build-id cache is corrupted.

This fixes above issues.

Signed-off-by Masami Hiramatsu <[email protected]>

---
 tools/perf/util/build-id.c |   17 ++++++++++++++++-
 tools/perf/util/symbol.c   |   13 +++++++++----
 2 files changed, 25 insertions(+), 5 deletions(-)

tools/perf/util/build-id.c +16 -1

index 46a8bcc..b035483 100644
@@ -155,6 +155,12 @@ static char *build_id_cache__linkname(const char *sbuild_id, char *bf,
 	return bf;
 }
 
+static bool __is_regular_file(const char *pathname)
+{
+	struct stat sb;
+	return stat(pathname, &sb) == 0 && S_ISREG(sb.st_mode);
+}
+
 static const char *build_id_cache__basename(bool is_kallsyms, bool is_vdso)
 {
 	return is_kallsyms ? "kallsyms" : (is_vdso ? "vdso" : "elf");

@@ -177,7 +183,11 @@ char *dso__build_id_filename(const struct dso *dso, char *bf, size_t size)
 	if (!linkname)
 		return NULL;
 
-	ret = asnprintf(&bf, size, "%s/%s", linkname,
+	/* Check if old style build_id cache */
+	if (__is_regular_file(linkname))
+		ret = asnprintf(&bf, size, "%s", linkname);
+	else
+		ret = asnprintf(&bf, size, "%s/%s", linkname,
 			 build_id_cache__basename(is_kallsyms, is_vdso));
 	if (ret < 0 || (!alloc && size < (unsigned int)ret))
 		bf = NULL;

@@ -445,6 +455,11 @@ int build_id_cache__add_s(const char *sbuild_id, const char *name,
 	if (!dir_name)
 		goto out_free;
 
+	/* Remove old style build-id cache */
+	if (__is_regular_file(dir_name))
+		if (unlink(dir_name))
+			goto out_free;
+
 	if (mkdir_p(dir_name, 0755))
 		goto out_free;
 

tools/perf/util/symbol.c +9 -4

index c57cb47..9463c7d 100644
@@ -1687,11 +1687,16 @@ static char *dso__find_kallsyms(struct dso *dso, struct map *map)
 
 	scnprintf(path, sizeof(path), "%s/[kernel.kallsyms]/%s/kallsyms",
 		  buildid_dir, sbuild_id);
-
+	/* Try old style kallsyms cache */
 	if (access(path, F_OK)) {
-		pr_err("No kallsyms or vmlinux with build-id %s was found\n",
-		       sbuild_id);
-		return NULL;
+		scnprintf(path, sizeof(path), "%s/[kernel.kallsyms]/%s",
+			  buildid_dir, sbuild_id);
+
+		if (access(path, F_OK)) {
+			pr_err("No kallsyms or vmlinux with build-id %s was found\n",
+			       sbuild_id);
+			return NULL;
+		}
 	}
 
 	return strdup(path);

From: Masami Hiramatsu <[email protected]>

As a utility function, add lsdir() which reads given directory and store entry name into a strlist. lsdir accepts a filter function so that user can filter out unneeded entries.

Signed-off-by Masami Hiramatsu <[email protected]>

Signed-off-by Masami Hiramatsu <[email protected]>

Em Tue, Apr 26, 2016 at 06:02:42PM +0900, Masami Hiramatsu escreveu:

Quoted 7 lines

From: Masami Hiramatsu <[email protected]>

As a utility function, add lsdir() which reads given directory and store entry name into a strlist. lsdir accepts a filter function so that user can filter out unneeded entries.

Quoted 2 lines

Signed-off-by Masami Hiramatsu <[email protected]>

Signed-off-by Masami Hiramatsu <[email protected]>

Thanks, applied.

Em Tue, Apr 26, 2016 at 10:40:20AM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 8 lines

Em Tue, Apr 26, 2016 at 06:02:42PM +0900, Masami Hiramatsu escreveu:

Quoted 7 lines

From: Masami Hiramatsu <[email protected]>

As a utility function, add lsdir() which reads given directory and store entry name into a strlist. lsdir accepts a filter function so that user can filter out unneeded entries.

Quoted 4 lines

Quoted 2 lines

Signed-off-by Masami Hiramatsu <[email protected]>

Signed-off-by Masami Hiramatsu <[email protected]>

Thanks, applied.

Had to fix it to build on some distros, like ubuntu 12.04:

alldeps-ubuntu-12.04: FAIL
util/util.h:228:38: error: declaration of 'dirname' shadows a global declaration [-Werror=shadow]
cc1: all warnings being treated as errors
mv: cannot stat `/tmp/build/perf/util/.db-export.o.tmp': No such file or directory make[3]: *** [/tmp/build/perf/util/db-export.o] Error 1
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [util] Error 2
make[2]: *** [ui] Error 2
make[1]: *** [/tmp/build/perf/libperf-in.o] Error
make: *** [install-bin] Error 2

Quoted 16 lines · On Tue, 26 Apr 2016 11:07:42 -0300 Arnaldo Carvalho de Melo <[email protected]>…

Em Tue, Apr 26, 2016 at 10:40:20AM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 8 lines

Em Tue, Apr 26, 2016 at 06:02:42PM +0900, Masami Hiramatsu escreveu:

Quoted 7 lines

From: Masami Hiramatsu <[email protected]>

As a utility function, add lsdir() which reads given directory and store entry name into a strlist. lsdir accepts a filter function so that user can filter out unneeded entries.

Quoted 4 lines

Quoted 2 lines

Signed-off-by Masami Hiramatsu <[email protected]>

Signed-off-by Masami Hiramatsu <[email protected]>

Thanks, applied.

Had to fix it to build on some distros, like ubuntu 12.04:

Quoted 11 lines

alldeps-ubuntu-12.04: FAIL
util/util.h:228:38: error: declaration of 'dirname' shadows a global declaration [-Werror=shadow]
cc1: all warnings being treated as errors
mv: cannot stat `/tmp/build/perf/util/.db-export.o.tmp': No such file or directory make[3]: *** [/tmp/build/perf/util/db-export.o] Error 1
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [util] Error 2
make[2]: *** [ui] Error 2
make[1]: *** [/tmp/build/perf/libperf-in.o] Error
make: *** [install-bin] Error 2

Hmm, I didn't know that the dummy argument can shadow a global declaration. I might be better to rename "dirname" to "dir_name".

Since other methods return 0 if succeeded (or filedesc), let probe_file__add_event() return 0 instead of the length of written bytes.

Signed-off-by Masami Hiramatsu <[email protected]>

---
 tools/perf/util/probe-file.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

tools/perf/util/probe-file.c +1 -2

index e3b3b92..3fe6214 100644
@@ -220,8 +220,7 @@ int probe_file__add_event(int fd, struct probe_trace_event *tev)
 
 	pr_debug("Writing event: %s\n", buf);
 	if (!probe_event_dry_run) {
-		ret = write(fd, buf, strlen(buf));
-		if (ret <= 0) {
+		if (write(fd, buf, strlen(buf)) < (int)strlen(buf)) {
 			ret = -errno;
 			pr_warning("Failed to write event: %s\n",
 				   strerror_r(errno, sbuf, sizeof(sbuf)));