[Resend because of the HTML error]
Hello Arnaldo,
Thanks a lot for the review, I guess you call it v1 for a reason. :)
> > +
> > + id = btf__find_by_name(btf, type);
>
> int id = ...
Do you want me to do the initialization in the middle of the function
body sir? A little reminder, char* pointer 'type' has to be shifted to
the first non-enum-prefix location to do the btf__find_by_name().
>
> > + if (id < 0)
>
> Shouldn't we have some warning here? ok, I see you do it later, in
> trace__read_syscall_info().
I'm sorry, could you be more specific please? To my understanding, it
is syscall__scnprintf_args() who called btf_enum_scnprintf(), and I
did the error handling(or fallback) by calling
syscall_arg_fmt__scnprintf_val(). It's like:
if btf_enum_scnprintf() returns non-0 // success
continue;
else // error
syscall_arg_fmt__scnprintf_val()
So we fall back to just printing the long value.
>
> Also I looked at the btf_enum_scnprintf() caller and if this isn't found
> nothing is printed, it is better to fallback to printing the integer
> value, as done in other parts, see:
Do you think the code below could be seen as a sort of fallback
mechanism? If nothing is printed, btf_enum_scnprintf() returns a 0, we
continue to do a syscall_arg_fmt__scnprintf_val() as the fallback. I
tested it by putting return 0 at the top of btf_enum_scnprintf(), and
it works, although not so straightforward... Maybe I should put the
fallback straight in btf_enum_scnprintf().
> > + if (sc->arg_fmt[arg.idx].is_enum == true && trace->btf) {
> > + p = btf_enum_scnprintf(bf + printed, size - printed, val,
> > + trace->btf, field->type);
> > + if (p) {
> > + printed += p;
> > + continue;
> > + }
> > + }
> > +
> > printed += syscall_arg_fmt__scnprintf_val(&sc->arg_fmt[arg.idx],
> > bf + printed, size - printed, &arg, val);
>
> size_t strarray__scnprintf(struct strarray *sa, char *bf, size_t size, const char *intfmt, bool show_prefix, int val)
>
> That intfmt is configurable to show hex or decimal and is used when the
> 'val' isn't found in the strarray, so we should use the same approach
> with BTF.
>
> > + return 0;
> > +
> > + bt = btf__type_by_id(btf, id);
> > + e = btf_enum(bt);
>
> Declare 'bt' and 'e' here
>
> > +
> > + for (int i = 0; i < btf_vlen(bt); i++, e++) {
> > + if (e->val == val)
> > + return scnprintf(bf, size, "%s",
> > + btf__name_by_offset(btf, e->name_off));
you mean doing it like this?
```
for (bt = btf__type_by_id(btf, id), e = btf_enum(bt), i = 0;
i < btf_vlen(bt); i++, e++) {
if (e->val == val) {
return scnprintf(bf, size, "%s",
btf__name_by_offset(btf, e->name_off));
}
}
```
> This is shaping up super nicely, great!
:) Thank you so much sir.
>
> I'm pushing the simplified first patch to my tmp.perf-tools-next branch
> in my tree at:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git tmp.perf-tools-next
>
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/log/?h=tmp.perf-tools-next
Sure, I'll pull from it and build the enum support on top of that.
>
> Some more comments below.
>
> > if (last_field)
> > sc->args_size = last_field->offset + last_field->size;
> > @@ -1811,6 +1854,7 @@ static int trace__read_syscall_info(struct trace *trace, int id)
> > char tp_name[128];
> > struct syscall *sc;
> > const char *name = syscalltbl__name(trace->sctbl, id);
> > + int err;
> >
> > #ifdef HAVE_SYSCALL_TABLE_SUPPORT
> > if (trace->syscalls.table == NULL) {
> > @@ -1883,7 +1927,17 @@ static int trace__read_syscall_info(struct trace *trace, int id)
> > sc->is_exit = !strcmp(name, "exit_group") || !strcmp(name, "exit");
> > sc->is_open = !strcmp(name, "open") || !strcmp(name, "openat");
> >
> > - return syscall__set_arg_fmts(sc);
> > + err = syscall__set_arg_fmts(sc);
>
> > + /* after calling syscall__set_arg_fmts() we'll know whether use_btf is true */
> > + if (sc->use_btf && trace->btf == NULL) {
> > + trace->btf = btf__load_vmlinux_btf();
> > + if (verbose > 0)
> > + fprintf(trace->output, trace->btf ? "vmlinux BTF loaded\n" :
> > + "Failed to load vmlinux BTF\n");
> > + }
>
> One suggestion here is to get the body of the above if and have it in a
> trace__load_vmlinux_btf(), as that call and the test under verbose will
> be used in other places, for instance, when supporting using BTF to
> pretty print non-syscall tracepoints.
>
> This function probably will grow to support detached BTF, possibly that
> idea about generating BTF from the scrape scripts, etc.
Sure.
Thank you very much for reviewing this patch, v2 is coming up.
Thanks,
Howard
>
> Thanks,
>
> - Arnaldo
>
> > + return err;
> > }
> >
> > static int evsel__init_tp_arg_scnprintf(struct evsel *evsel)
> > @@ -2050,7 +2104,7 @@ static size_t syscall__scnprintf_args(struct syscall *sc, char *bf, size_t size,
> > unsigned char *args, void *augmented_args, int augmented_args_size,
> > struct trace *trace, struct thread *thread)
> > {
> > - size_t printed = 0;
> > + size_t printed = 0, p;
> > unsigned long val;
> > u8 bit = 1;
> > struct syscall_arg arg = {
> > @@ -2103,6 +2157,15 @@ static size_t syscall__scnprintf_args(struct syscall *sc, char *bf, size_t size,
> > if (trace->show_arg_names)
> > printed += scnprintf(bf + printed, size - printed, "%s: ", field->name);
> >
> > }
> > --
> > 2.45.2
