| Message ID | 20190228050643.958685-13-songliubraving@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | perf annotation of BPF programs | expand |
On Wed, Feb 27, 2019 at 09:06:40PM -0800, Song Liu wrote: SNIP > +#ifdef DISASM_FOUR_ARGS_SIGNATURE > + disassemble = disassembler(info.arch, > + bfd_big_endian(bfdf), > + info.mach, > + bfdf); > +#else > + disassemble = disassembler(bfdf); > +#endif > + assert(disassemble); > + > + fflush(s); > + do { > + const struct bpf_line_info *linfo = NULL; > + struct disasm_line *dl; > + size_t prev_buf_size; > + const char *srcline; > + u64 addr; > + > + addr = pc + ((u64 *)(info_linear->info.jited_ksyms))[sub_id]; > + count = disassemble(pc, &info); > + > + if (prog_linfo) > + linfo = bpf_prog_linfo__lfind_addr_func(prog_linfo, > + addr, sub_id, > + nr_skip); > + > + if (linfo && btf) { > + srcline = btf__name_by_offset(btf, linfo->line_off); > + nr_skip++; > + } else > + srcline = NULL; > + > + fprintf(s, "\n"); > + prev_buf_size = buf_size; > + fflush(s); > + > + if (!opts->hide_src_code && srcline) { > + args->offset = -1; > + args->line = strdup(srcline); > + args->line_nr = 0; > + args->ms.sym = sym; > + dl = disasm_line__new(args); > + if (dl) { > + annotation_line__add(&dl->al, > + ¬es->src->source); > + } > + } are you adding the line twice? (code up and down) > + > + args->offset = pc; > + args->line = buf + prev_buf_size; > + args->line_nr = 0; also could you please explain why line_nr is not use thanks, jirka
On Wed, Feb 27, 2019 at 09:06:40PM -0800, Song Liu wrote: SNIP > + > + disassemble_init_for_target(&info); > + > +#ifdef DISASM_FOUR_ARGS_SIGNATURE > + disassemble = disassembler(info.arch, > + bfd_big_endian(bfdf), > + info.mach, > + bfdf); > +#else > + disassemble = disassembler(bfdf); > +#endif > + assert(disassemble); > + > + fflush(s); what's the reason for those fflush calls? thanks, jirka > + do { > + const struct bpf_line_info *linfo = NULL; > + struct disasm_line *dl; > + size_t prev_buf_size; > + const char *srcline; > + u64 addr; > + > + addr = pc + ((u64 *)(info_linear->info.jited_ksyms))[sub_id]; > + count = disassemble(pc, &info); > + > + if (prog_linfo) > + linfo = bpf_prog_linfo__lfind_addr_func(prog_linfo, > + addr, sub_id, > + nr_skip); > + > + if (linfo && btf) { > + srcline = btf__name_by_offset(btf, linfo->line_off); > + nr_skip++; > + } else > + srcline = NULL; > + > + fprintf(s, "\n"); > + prev_buf_size = buf_size; > + fflush(s); SNIP
On Wed, Feb 27, 2019 at 09:06:40PM -0800, Song Liu wrote: SNIP > + fflush(s); > + do { > + const struct bpf_line_info *linfo = NULL; > + struct disasm_line *dl; > + size_t prev_buf_size; > + const char *srcline; > + u64 addr; > + > + addr = pc + ((u64 *)(info_linear->info.jited_ksyms))[sub_id]; > + count = disassemble(pc, &info); > + > + if (prog_linfo) > + linfo = bpf_prog_linfo__lfind_addr_func(prog_linfo, > + addr, sub_id, > + nr_skip); > + > + if (linfo && btf) { > + srcline = btf__name_by_offset(btf, linfo->line_off); > + nr_skip++; > + } else > + srcline = NULL; > + > + fprintf(s, "\n"); > + prev_buf_size = buf_size; > + fflush(s); > + > + if (!opts->hide_src_code && srcline) { > + args->offset = -1; why's the offset not needed in here? jirka > + args->line = strdup(srcline); > + args->line_nr = 0; > + args->ms.sym = sym; > + dl = disasm_line__new(args); > + if (dl) { > + annotation_line__add(&dl->al, > + ¬es->src->source); > + } > + } SNIP
> On Mar 4, 2019, at 5:53 AM, Jiri Olsa <jolsa@redhat.com> wrote: > > On Wed, Feb 27, 2019 at 09:06:40PM -0800, Song Liu wrote: > > SNIP > >> + fflush(s); >> + do { >> + const struct bpf_line_info *linfo = NULL; >> + struct disasm_line *dl; >> + size_t prev_buf_size; >> + const char *srcline; >> + u64 addr; >> + >> + addr = pc + ((u64 *)(info_linear->info.jited_ksyms))[sub_id]; >> + count = disassemble(pc, &info); >> + >> + if (prog_linfo) >> + linfo = bpf_prog_linfo__lfind_addr_func(prog_linfo, >> + addr, sub_id, >> + nr_skip); >> + >> + if (linfo && btf) { >> + srcline = btf__name_by_offset(btf, linfo->line_off); >> + nr_skip++; >> + } else >> + srcline = NULL; >> + >> + fprintf(s, "\n"); >> + prev_buf_size = buf_size; >> + fflush(s); >> + >> + if (!opts->hide_src_code && srcline) { >> + args->offset = -1; > > why's the offset not needed in here? This is the source code line. Based on my understanding of annotation code, we put -1 for source code line. Did I misunderstand something? Thanks, Song > > jirka > >> + args->line = strdup(srcline); >> + args->line_nr = 0; >> + args->ms.sym = sym; >> + dl = disasm_line__new(args); >> + if (dl) { >> + annotation_line__add(&dl->al, >> + ¬es->src->source); >> + } >> + } > > SNIP
On Mon, Mar 04, 2019 at 07:45:00PM +0000, Song Liu wrote: > > > > On Mar 4, 2019, at 5:53 AM, Jiri Olsa <jolsa@redhat.com> wrote: > > > > On Wed, Feb 27, 2019 at 09:06:40PM -0800, Song Liu wrote: > > > > SNIP > > > >> + fflush(s); > >> + do { > >> + const struct bpf_line_info *linfo = NULL; > >> + struct disasm_line *dl; > >> + size_t prev_buf_size; > >> + const char *srcline; > >> + u64 addr; > >> + > >> + addr = pc + ((u64 *)(info_linear->info.jited_ksyms))[sub_id]; > >> + count = disassemble(pc, &info); > >> + > >> + if (prog_linfo) > >> + linfo = bpf_prog_linfo__lfind_addr_func(prog_linfo, > >> + addr, sub_id, > >> + nr_skip); > >> + > >> + if (linfo && btf) { > >> + srcline = btf__name_by_offset(btf, linfo->line_off); > >> + nr_skip++; > >> + } else > >> + srcline = NULL; > >> + > >> + fprintf(s, "\n"); > >> + prev_buf_size = buf_size; > >> + fflush(s); > >> + > >> + if (!opts->hide_src_code && srcline) { > >> + args->offset = -1; > > > > why's the offset not needed in here? > > This is the source code line. Based on my understanding of annotation > code, we put -1 for source code line. Did I misunderstand something? please double check, IIRC annotation_line__print will print just more info if offset is present, together with the source, but I haven't checked that code for some time, so I might be wrong jirka
> On Mar 4, 2019, at 12:37 PM, Jiri Olsa <jolsa@redhat.com> wrote: > > On Mon, Mar 04, 2019 at 07:45:00PM +0000, Song Liu wrote: >> >> >>> On Mar 4, 2019, at 5:53 AM, Jiri Olsa <jolsa@redhat.com> wrote: >>> >>> On Wed, Feb 27, 2019 at 09:06:40PM -0800, Song Liu wrote: >>> >>> SNIP >>> >>>> + fflush(s); >>>> + do { >>>> + const struct bpf_line_info *linfo = NULL; >>>> + struct disasm_line *dl; >>>> + size_t prev_buf_size; >>>> + const char *srcline; >>>> + u64 addr; >>>> + >>>> + addr = pc + ((u64 *)(info_linear->info.jited_ksyms))[sub_id]; >>>> + count = disassemble(pc, &info); >>>> + >>>> + if (prog_linfo) >>>> + linfo = bpf_prog_linfo__lfind_addr_func(prog_linfo, >>>> + addr, sub_id, >>>> + nr_skip); >>>> + >>>> + if (linfo && btf) { >>>> + srcline = btf__name_by_offset(btf, linfo->line_off); >>>> + nr_skip++; >>>> + } else >>>> + srcline = NULL; >>>> + >>>> + fprintf(s, "\n"); >>>> + prev_buf_size = buf_size; >>>> + fflush(s); >>>> + >>>> + if (!opts->hide_src_code && srcline) { >>>> + args->offset = -1; >>> >>> why's the offset not needed in here? >> >> This is the source code line. Based on my understanding of annotation >> code, we put -1 for source code line. Did I misunderstand something? > > please double check, IIRC annotation_line__print will print > just more info if offset is present, together with the source, > but I haven't checked that code for some time, so I might be wrong > > jirka In symbol__parse_objdump_line(), default offset is -1. And it is only updated if line_ip is not -1. And line_ip for src code line is -1, so offset for the source line is -1. It works fine in the tests when I toggle source code on/off with key 's'. Thanks Song
> On Mar 4, 2019, at 5:53 AM, Jiri Olsa <jolsa@redhat.com> wrote: > > On Wed, Feb 27, 2019 at 09:06:40PM -0800, Song Liu wrote: > > SNIP > >> + >> + disassemble_init_for_target(&info); >> + >> +#ifdef DISASM_FOUR_ARGS_SIGNATURE >> + disassemble = disassembler(info.arch, >> + bfd_big_endian(bfdf), >> + info.mach, >> + bfdf); >> +#else >> + disassemble = disassembler(bfdf); >> +#endif >> + assert(disassemble); >> + >> + fflush(s); > > what's the reason for those fflush calls? fflush() updates buf_size for the stream. This one before the loop make sure we read proper buf_size (prev_buf_size = bud_size) in the first iteration. Then, the fflush() in each iteration makes sure the next iteration reads proper prev_buf_size. Thanks, Song > > thanks, > jirka > >> + do { >> + const struct bpf_line_info *linfo = NULL; >> + struct disasm_line *dl; >> + size_t prev_buf_size; >> + const char *srcline; >> + u64 addr; >> + >> + addr = pc + ((u64 *)(info_linear->info.jited_ksyms))[sub_id]; >> + count = disassemble(pc, &info); >> + >> + if (prog_linfo) >> + linfo = bpf_prog_linfo__lfind_addr_func(prog_linfo, >> + addr, sub_id, >> + nr_skip); >> + >> + if (linfo && btf) { >> + srcline = btf__name_by_offset(btf, linfo->line_off); >> + nr_skip++; >> + } else >> + srcline = NULL; >> + >> + fprintf(s, "\n"); >> + prev_buf_size = buf_size; >> + fflush(s); > > SNIP
diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature index 5467c6bf9ceb..4f35e9ff1e00 100644 --- a/tools/build/Makefile.feature +++ b/tools/build/Makefile.feature @@ -71,7 +71,8 @@ FEATURE_TESTS_BASIC := \ sdt \ setns \ libopencsd \ - libaio + libaio \ + disassembler-four-args # FEATURE_TESTS_BASIC + FEATURE_TESTS_EXTRA is the complete list # of all feature tests @@ -118,7 +119,8 @@ FEATURE_DISPLAY ?= \ lzma \ get_cpuid \ bpf \ - libaio + libaio \ + disassembler-four-args # Set FEATURE_CHECK_(C|LD)FLAGS-all for all FEATURE_TESTS features. # If in the future we need per-feature checks/flags for features not diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config index e0bafbc273af..ab223239f1fb 100644 --- a/tools/perf/Makefile.config +++ b/tools/perf/Makefile.config @@ -796,6 +796,10 @@ ifdef HAVE_KVM_STAT_SUPPORT CFLAGS += -DHAVE_KVM_STAT_SUPPORT endif +ifeq ($(feature-disassembler-four-args), 1) + CFLAGS += -DDISASM_FOUR_ARGS_SIGNATURE +endif + ifeq (${IS_64_BIT}, 1) ifndef NO_PERF_READ_VDSO32 $(call feature_check,compile-32) diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c index 70de8f6b3aee..e467aa0ef0c2 100644 --- a/tools/perf/util/annotate.c +++ b/tools/perf/util/annotate.c @@ -9,6 +9,10 @@ #include <errno.h> #include <inttypes.h> +#include <bpf/bpf.h> +#include <bpf/btf.h> +#include <bpf/libbpf.h> +#include <linux/btf.h> #include "util.h" #include "ui/ui.h" #include "sort.h" @@ -22,6 +26,7 @@ #include "annotate.h" #include "evsel.h" #include "evlist.h" +#include "bpf-event.h" #include "block-range.h" #include "string2.h" #include "arch/common.h" @@ -29,6 +34,9 @@ #include <pthread.h> #include <linux/bitops.h> #include <linux/kernel.h> +#include <bfd.h> +#include <dis-asm.h> +#include <bpf/libbpf.h> /* FIXME: For the HE_COLORSET */ #include "ui/browser.h" @@ -1672,6 +1680,144 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil return 0; } +static int symbol__disassemble_bpf(struct symbol *sym, + struct annotate_args *args) +{ + struct annotation *notes = symbol__annotation(sym); + struct annotation_options *opts = args->options; + struct bpf_prog_info_linear *info_linear; + struct bpf_prog_linfo *prog_linfo = NULL; + struct bpf_prog_info_node *info_node; + int len = sym->end - sym->start; + disassembler_ftype disassemble; + struct map *map = args->ms.map; + struct disassemble_info info; + struct dso *dso = map->dso; + int pc = 0, count, sub_id; + struct btf *btf = NULL; + char tpath[PATH_MAX]; + size_t buf_size; + int nr_skip = 0; + int ret = -1; + char *buf; + bfd *bfdf; + FILE *s; + + if (dso->binary_type != DSO_BINARY_TYPE__BPF_PROG_INFO) + return -1; + + pr_debug("%s: handling sym %s addr %lx len %lx\n", __func__, + sym->name, sym->start, sym->end - sym->start); + + memset(tpath, 0, sizeof(tpath)); + perf_exe(tpath, sizeof(tpath)); + + bfdf = bfd_openr(tpath, NULL); + assert(bfdf); + assert(bfd_check_format(bfdf, bfd_object)); + + s = open_memstream(&buf, &buf_size); + if (!s) + goto out; + init_disassemble_info(&info, s, + (fprintf_ftype) fprintf); + + info.arch = bfd_get_arch(bfdf); + info.mach = bfd_get_mach(bfdf); + + info_node = perf_env__find_bpf_prog_info(dso->bpf_prog.env, + dso->bpf_prog.id); + if (!info_node) + goto out; + info_linear = info_node->info_linear; + sub_id = dso->bpf_prog.sub_id; + + info.buffer = (void *)(info_linear->info.jited_prog_insns); + info.buffer_length = info_linear->info.jited_prog_len; + + if (info_linear->info.nr_line_info) + prog_linfo = bpf_prog_linfo__new(&info_linear->info); + + if (info_linear->info.btf_id) { + struct btf_node *node; + + node = perf_env__find_btf(dso->bpf_prog.env, + info_linear->info.btf_id); + if (node) + btf = btf__new((__u8 *)(node->data), + node->data_size); + } + + disassemble_init_for_target(&info); + +#ifdef DISASM_FOUR_ARGS_SIGNATURE + disassemble = disassembler(info.arch, + bfd_big_endian(bfdf), + info.mach, + bfdf); +#else + disassemble = disassembler(bfdf); +#endif + assert(disassemble); + + fflush(s); + do { + const struct bpf_line_info *linfo = NULL; + struct disasm_line *dl; + size_t prev_buf_size; + const char *srcline; + u64 addr; + + addr = pc + ((u64 *)(info_linear->info.jited_ksyms))[sub_id]; + count = disassemble(pc, &info); + + if (prog_linfo) + linfo = bpf_prog_linfo__lfind_addr_func(prog_linfo, + addr, sub_id, + nr_skip); + + if (linfo && btf) { + srcline = btf__name_by_offset(btf, linfo->line_off); + nr_skip++; + } else + srcline = NULL; + + fprintf(s, "\n"); + prev_buf_size = buf_size; + fflush(s); + + if (!opts->hide_src_code && srcline) { + args->offset = -1; + args->line = strdup(srcline); + args->line_nr = 0; + args->ms.sym = sym; + dl = disasm_line__new(args); + if (dl) { + annotation_line__add(&dl->al, + ¬es->src->source); + } + } + + args->offset = pc; + args->line = buf + prev_buf_size; + args->line_nr = 0; + args->ms.sym = sym; + dl = disasm_line__new(args); + if (dl) + annotation_line__add(&dl->al, ¬es->src->source); + + pc += count; + } while (count > 0 && pc < len); + + ret = 0; +out: + free(prog_linfo); + free(btf); + fclose(s); + bfd_close(bfdf); + return ret; +} + static int symbol__disassemble(struct symbol *sym, struct annotate_args *args) { struct annotation_options *opts = args->options; @@ -1699,7 +1845,9 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args) pr_debug("annotating [%p] %30s : [%p] %30s\n", dso, dso->long_name, sym, sym->name); - if (dso__is_kcore(dso)) { + if (dso->binary_type == DSO_BINARY_TYPE__BPF_PROG_INFO) { + return symbol__disassemble_bpf(sym, args); + } else if (dso__is_kcore(dso)) { kce.kcore_filename = symfs_filename; kce.addr = map__rip_2objdump(map, sym->start); kce.offs = sym->start; diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c index 62c8cf622607..1798192bf0f9 100644 --- a/tools/perf/util/dso.c +++ b/tools/perf/util/dso.c @@ -181,6 +181,7 @@ int dso__read_binary_type_filename(const struct dso *dso, case DSO_BINARY_TYPE__KALLSYMS: case DSO_BINARY_TYPE__GUEST_KALLSYMS: case DSO_BINARY_TYPE__JAVA_JIT: + case DSO_BINARY_TYPE__BPF_PROG_INFO: case DSO_BINARY_TYPE__NOT_FOUND: ret = -1; break; diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h index 8c8a7abe809d..f20d319463f1 100644 --- a/tools/perf/util/dso.h +++ b/tools/perf/util/dso.h @@ -14,6 +14,8 @@ #include "namespaces.h" #include "build-id.h" +struct perf_env; + enum dso_binary_type { DSO_BINARY_TYPE__KALLSYMS = 0, DSO_BINARY_TYPE__GUEST_KALLSYMS, @@ -34,6 +36,7 @@ enum dso_binary_type { DSO_BINARY_TYPE__KCORE, DSO_BINARY_TYPE__GUEST_KCORE, DSO_BINARY_TYPE__OPENEMBEDDED_DEBUGINFO, + DSO_BINARY_TYPE__BPF_PROG_INFO, DSO_BINARY_TYPE__NOT_FOUND, }; @@ -177,17 +180,25 @@ struct dso { struct auxtrace_cache *auxtrace_cache; int comp; - /* dso data file */ - struct { - struct rb_root cache; - int fd; - int status; - u32 status_seen; - size_t file_size; - struct list_head open_entry; - u64 debug_frame_offset; - u64 eh_frame_hdr_offset; - } data; + union { + /* dso data file */ + struct { + struct rb_root cache; + int fd; + int status; + u32 status_seen; + size_t file_size; + struct list_head open_entry; + u64 debug_frame_offset; + u64 eh_frame_hdr_offset; + } data; + /* bpf prog information */ + struct { + u32 id; + u32 sub_id; + struct perf_env *env; + } bpf_prog; + }; union { /* Tool specific area */ void *priv; diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c index 48efad6d0f90..33ae59e89da2 100644 --- a/tools/perf/util/symbol.c +++ b/tools/perf/util/symbol.c @@ -1441,6 +1441,7 @@ static bool dso__is_compatible_symtab_type(struct dso *dso, bool kmod, case DSO_BINARY_TYPE__BUILD_ID_CACHE_DEBUGINFO: return true; + case DSO_BINARY_TYPE__BPF_PROG_INFO: case DSO_BINARY_TYPE__NOT_FOUND: default: return false;
This patch enables the annotation of bpf program. A new dso type DSO_BINARY_TYPE__BPF_PROG_INFO is introduced to for BPF programs. In symbol__disassemble(), DSO_BINARY_TYPE__BPF_PROG_INFO dso calls into a new function symbol__disassemble_bpf(), where annotation line information is filled based bpf_prog_info and btf saved in given perf_env. symbol__disassemble_bpf() uses libbfd to disassemble bpf programs. Signed-off-by: Song Liu <songliubraving@fb.com> --- tools/build/Makefile.feature | 6 +- tools/perf/Makefile.config | 4 + tools/perf/util/annotate.c | 150 ++++++++++++++++++++++++++++++++++- tools/perf/util/dso.c | 1 + tools/perf/util/dso.h | 33 +++++--- tools/perf/util/symbol.c | 1 + 6 files changed, 181 insertions(+), 14 deletions(-)