Re: [PATCH v3 perf,bpf 10/11] perf, bpf: enable annotation of bpf program

Jiri Olsa
2019-02-17T23:06:20Z

On Fri, Feb 15, 2019 at 01:53:53PM -0800, Song Liu wrote:

SNIP

> +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;
> + __u64 arrays;
> + 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));
> + get_exec_path(tpath, sizeof(tpath));
> +
> + bfdf = bfd_openr(tpath, NULL);
> + assert(bfdf);
> + assert(bfd_check_format(bfdf, bfd_object));
> +
> + s = open_memstream(&buf, &buf_size);

what if open_memstream fails?

> + init_disassemble_info(&info, s,
> + (fprintf_ftype) fprintf);
> +
> + info.arch = bfd_get_arch(bfdf);
> + info.mach = bfd_get_mach(bfdf);
> +
> + arrays = 1UL << BPF_PROG_INFO_JITED_INSNS;
> + arrays |= 1UL << BPF_PROG_INFO_JITED_KSYMS;
> + arrays |= 1UL << BPF_PROG_INFO_LINE_INFO;
> + arrays |= 1UL << BPF_PROG_INFO_FUNC_INFO;

what's the arrays for?

> +
> + info_node = perf_env__find_bpf_prog_info(dso->bpf_prog.env,
> + dso->bpf_prog.id);
> + if (!info_node)
> + return -1;
> + 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);
> + prog_linfo = prog_linfo;
> +
> + 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);
> + }

what if btf__new fails? the btf__name_by_offset
does not check btf != NULL

> +
> + 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);

any chance this function could be split into some logical
pieces/fucntions?

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);
> +
> + linfo = bpf_prog_linfo__lfind_addr_func(prog_linfo, addr, sub_id,
> + nr_skip);
> +
> + if (linfo) {
> + 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);
> + annotation_line__add(&dl->al, &notes->src->source);
> + }
> +
> + args->offset = pc;
> + args->line = buf + prev_buf_size;
> + args->line_nr = 0;
> + args->ms.sym = sym;
> + dl = disasm_line__new(args);
> + annotation_line__add(&dl->al, &notes->src->source);
> +
> + pc += count;
> + } while (count > 0 && pc < len);
> +
> + bfd_close(bfdf);
> + return 0;
> +}

SNIP