On 03/14/2018 07:39 PM, Masami Hiramatsu wrote:
> Hi Ravi,
>
> This code logic looks good. I just have several small comments for style.
>
> On Tue, 13 Mar 2018 18:26:02 +0530
> Ravi Bangoria <ravi.bangoria@xxxxxxxxxxxxxxxxxx> wrote:
>
>> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>> index e1dbc98..2cbe68a 100644
>> --- a/tools/perf/util/probe-event.c
>> +++ b/tools/perf/util/probe-event.c
>> @@ -1832,6 +1832,12 @@ int parse_probe_trace_command(const char *cmd, struct probe_trace_event *tev)
>> tp->offset = strtoul(fmt2_str, NULL, 10);
>> }
>>
>> + if (tev->uprobes) {
>> + fmt2_str = strchr(p, '(');
>> + if (fmt2_str)
>> + tp->ref_ctr_offset = strtoul(fmt2_str + 1, NULL, 0);
>> + }
>> +
>> tev->nargs = argc - 2;
>> tev->args = zalloc(sizeof(struct probe_trace_arg) * tev->nargs);
>> if (tev->args == NULL) {
>> @@ -2054,15 +2060,22 @@ char *synthesize_probe_trace_command(struct probe_trace_event *tev)
>> }
>>
>> /* Use the tp->address for uprobes */
>> - if (tev->uprobes)
>> - err = strbuf_addf(&buf, "%s:0x%lx", tp->module, tp->address);
>> - else if (!strncmp(tp->symbol, "0x", 2))
>> + if (tev->uprobes) {
>> + if (tp->ref_ctr_offset)
>> + err = strbuf_addf(&buf, "%s:0x%lx(0x%lx)", tp->module,
>> + tp->address, tp->ref_ctr_offset);
>> + else
>> + err = 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() */
>> err = strbuf_addf(&buf, "%s%s0x%lx", tp->module ?: "",
>> tp->module ? ":" : "", tp->address);
>> - else
>> + } else {
>> err = strbuf_addf(&buf, "%s%s%s+%lu", tp->module ?: "",
>> tp->module ? ":" : "", tp->symbol, tp->offset);
>> + }
> What the purpose of this {}?
The starting if has multiple statements and thus it needs braces. So I added
braces is all other conditions.
>> +
>> if (err)
>> goto error;
>>
>> diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
>> index 45b14f0..15a98c3 100644
>> --- a/tools/perf/util/probe-event.h
>> +++ b/tools/perf/util/probe-event.h
>> @@ -27,6 +27,7 @@ struct probe_trace_point {
>> char *symbol; /* Base symbol */
>> char *module; /* Module name */
>> unsigned long offset; /* Offset from symbol */
>> + unsigned long ref_ctr_offset; /* SDT reference counter offset */
>> unsigned long address; /* Actual address of the trace point */
>> bool retprobe; /* Return probe flag */
>> };
>> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
>> index 4ae1123..08ba3a6 100644
>> --- a/tools/perf/util/probe-file.c
>> +++ b/tools/perf/util/probe-file.c
>> @@ -701,6 +701,12 @@ static unsigned long long sdt_note__get_addr(struct sdt_note *note)
>> : (unsigned long long)note->addr.a64[0];
>> }
>>
>> +static unsigned long long sdt_note__get_ref_ctr_offset(struct sdt_note *note)
>> +{
>> + return note->bit32 ? (unsigned long long)note->addr.a32[2]
>> + : (unsigned long long)note->addr.a64[2];
>> +}
> Could you please introduce an enum for specifying the index by name?
>
> e.g.
> enum {
> SDT_NOTE_IDX_ADDR = 0,
> SDT_NOTE_IDX_REFCTR = 2,
> };
That will be good. Will change it.
>> +
>> static const char * const type_to_suffix[] = {
>> ":s64", "", "", "", ":s32", "", ":s16", ":s8",
>> "", ":u8", ":u16", "", ":u32", "", "", "", ":u64"
>> @@ -776,14 +782,24 @@ static char *synthesize_sdt_probe_command(struct sdt_note *note,
>> {
>> struct strbuf buf;
>> char *ret = NULL, **args;
>> - int i, args_count;
>> + int i, args_count, err;
>> + unsigned long long ref_ctr_offset;
>>
>> if (strbuf_init(&buf, 32) < 0)
>> return NULL;
>>
>> - if (strbuf_addf(&buf, "p:%s/%s %s:0x%llx",
>> + ref_ctr_offset = sdt_note__get_ref_ctr_offset(note);
>> +
>> + if (ref_ctr_offset)
>> + err = strbuf_addf(&buf, "p:%s/%s %s:0x%llx(0x%llx)",
>> sdtgrp, note->name, pathname,
>> - sdt_note__get_addr(note)) < 0)
>> + sdt_note__get_addr(note), ref_ctr_offset);
>> + else
>> + err = strbuf_addf(&buf, "p:%s/%s %s:0x%llx",
>> + sdtgrp, note->name, pathname,
>> + sdt_note__get_addr(note));
> This can be minimized (and avoid repeating) by using 2 strbuf_addf()s, like
>
> err = strbuf_addf(&buf, "p:%s/%s %s:0x%llx",
> sdtgrp, note->name, pathname,
> sdt_note__get_addr(note));
> if (ref_ctr_offset && !err < 0)
> err = strbuf_addf("(0x%llx)", ref_ctr_offset);
Sure. Will change it.
>
>> +
>> + if (err < 0)
>> goto error;
>>
>> if (!note->args)
>> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
>> index 2de7705..76c7b54 100644
>> --- a/tools/perf/util/symbol-elf.c
>> +++ b/tools/perf/util/symbol-elf.c
>> @@ -1928,6 +1928,16 @@ static int populate_sdt_note(Elf **elf, const char *data, size_t len,
>> }
>> }
>>
>> + /* Adjust reference counter offset */
>> + if (elf_section_by_name(*elf, &ehdr, &shdr, SDT_PROBES_SCN, NULL)) {
>> + if (shdr.sh_offset) {
>> + if (tmp->bit32)
>> + tmp->addr.a32[2] -= (shdr.sh_addr - shdr.sh_offset);
>> + else
>> + tmp->addr.a64[2] -= (shdr.sh_addr - shdr.sh_offset);
> Here we should use enum above too.
Sure.
Thanks for the review :)
Ravi
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
