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 <[email protected]> wrote: > > > Em Tue, Apr 26, 2016 at 06:02:11PM +0900, Masami Hiramatsu escreveu: > > > 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. > > > > > 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 <[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(-) > > > > > > 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 <[email protected]>
