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

From: Jiri Olsa <jolsa@redhat.com>
To: Song Liu <songliubraving@fb.com>
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	ast@kernel.org, daniel@iogearbox.net, kernel-team@fb.com,
	peterz@infradead.org, acme@redhat.com, jolsa@kernel.org,
	namhyung@kernel.org
Subject: Re: [PATCH v3 perf,bpf 10/11] perf, bpf: enable annotation of bpf program
Date: Mon, 18 Feb 2019 00:06:14 +0100	[thread overview]
Message-ID: <20190217230614.GH7443@krava> (raw)
In-Reply-To: <20190215215354.3114006-11-songliubraving@fb.com>

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