Re: [PATCH bpf-next 3/5] bpftool: Support inline annotations when dumping the CFG of a program

[Stanislav Fomichev <sdf@google.com>]

On 03/24, Quentin Monnet wrote:
> We support dumping the control flow graph of loaded programs to the DOT
> format with bpftool, but so far this feature wouldn't display the source
> code lines available through BTF along with the eBPF bytecode. Let's add
> support for these annotations, to make it easier to read the graph.

> In prog.c, we move the call to dump_xlated_cfg() in order to pass and
> use the full struct dump_data, instead of creating a minimal one in
> draw_bb_node().

> We pass the pointer to this struct down to dump_xlated_for_graph() in
> xlated_dumper.c, where most of the logics is added. We deal with BTF
> mostly like we do for plain or JSON output, except that we cannot use a
> "nr_skip" value to skip a given number of linfo records (we don't
> process the BPF instructions linearly, and apart from the root of the
> graph we don't know how many records we should skip, so we just store
> the last linfo and make sure the new one we find is different before
> printing it).

> When printing the source instructions to the label of a DOT graph node,
> there are a few subtleties to address. We want some special newline
> markers, and there are some characters that we must escape. To deal with
> them, we introduce a new dedicated function btf_dump_linfo_dotlabel() in
> btf_dumper.c. We'll reuse this function in a later commit to format the
> filepath, line, and column references as well.

> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> ---
>   tools/bpf/bpftool/btf_dumper.c    | 34 +++++++++++++++++++++++++++++++
>   tools/bpf/bpftool/cfg.c           | 23 +++++++++------------
>   tools/bpf/bpftool/cfg.h           |  4 +++-
>   tools/bpf/bpftool/main.h          |  2 ++
>   tools/bpf/bpftool/prog.c          | 17 +++++++---------
>   tools/bpf/bpftool/xlated_dumper.c | 32 ++++++++++++++++++++++++++++-
>   6 files changed, 87 insertions(+), 25 deletions(-)

> diff --git a/tools/bpf/bpftool/btf_dumper.c  
> b/tools/bpf/bpftool/btf_dumper.c
> index e7f6ec3a8f35..504d7c75cc27 100644
> --- a/tools/bpf/bpftool/btf_dumper.c
> +++ b/tools/bpf/bpftool/btf_dumper.c
> @@ -821,3 +821,37 @@ void btf_dump_linfo_json(const struct btf *btf,
>   					BPF_LINE_INFO_LINE_COL(linfo->line_col));
>   	}
>   }
> +
> +static void dotlabel_puts(const char *s)
> +{
> +	FILE *stream = stdout;
> +
> +	for (; *s; ++s) {
> +		switch (*s) {
> +		case '\\':
> +		case '"':
> +		case '{':
> +		case '}':
> +		case '>':
> +		case '|':
> +			fputc('\\', stream);
> +			__fallthrough;
> +		default:
> +			fputc(*s, stream);
> +		}

nit: optionally, if you're going to respin, maybe do putchar instead
of fputc + stdout? (not sure why you're doing fputs)

> +	}
> +}
> +
> +void btf_dump_linfo_dotlabel(const struct btf *btf,
> +			     const struct bpf_line_info *linfo)
> +{
> +	const char *line = btf__name_by_offset(btf, linfo->line_off);
> +
> +	if (!line)
> +		return;
> +	line = ltrim(line);
> +
> +	printf("; ");
> +	dotlabel_puts(line);
> +	printf("\\l\\\n");
> +}
> diff --git a/tools/bpf/bpftool/cfg.c b/tools/bpf/bpftool/cfg.c
> index 1951219a9af7..9fdc1f0cdd6e 100644
> --- a/tools/bpf/bpftool/cfg.c
> +++ b/tools/bpf/bpftool/cfg.c
> @@ -380,7 +380,8 @@ static void cfg_destroy(struct cfg *cfg)
>   	}
>   }

> -static void draw_bb_node(struct func_node *func, struct bb_node *bb)
> +static void
> +draw_bb_node(struct func_node *func, struct bb_node *bb, struct  
> dump_data *dd)
>   {
>   	const char *shape;

> @@ -398,13 +399,9 @@ static void draw_bb_node(struct func_node *func,  
> struct bb_node *bb)
>   		printf("EXIT");
>   	} else {
>   		unsigned int start_idx;
> -		struct dump_data dd = {};
> -
> -		printf("{");
> -		kernel_syms_load(&dd);
> +		printf("{\\\n");
>   		start_idx = bb->head - func->start;
> -		dump_xlated_for_graph(&dd, bb->head, bb->tail, start_idx);
> -		kernel_syms_destroy(&dd);
> +		dump_xlated_for_graph(dd, bb->head, bb->tail, start_idx);
>   		printf("}");
>   	}

> @@ -430,12 +427,12 @@ static void draw_bb_succ_edges(struct func_node  
> *func, struct bb_node *bb)
>   	}
>   }

> -static void func_output_bb_def(struct func_node *func)
> +static void func_output_bb_def(struct func_node *func, struct dump_data  
> *dd)
>   {
>   	struct bb_node *bb;

>   	list_for_each_entry(bb, &func->bbs, l) {
> -		draw_bb_node(func, bb);
> +		draw_bb_node(func, bb, dd);
>   	}
>   }

> @@ -455,7 +452,7 @@ static void func_output_edges(struct func_node *func)
>   	       func_idx, ENTRY_BLOCK_INDEX, func_idx, EXIT_BLOCK_INDEX);
>   }

> -static void cfg_dump(struct cfg *cfg)
> +static void cfg_dump(struct cfg *cfg, struct dump_data *dd)
>   {
>   	struct func_node *func;

> @@ -463,14 +460,14 @@ static void cfg_dump(struct cfg *cfg)
>   	list_for_each_entry(func, &cfg->funcs, l) {
>   		printf("subgraph \"cluster_%d\"  
> {\n\tstyle=\"dashed\";\n\tcolor=\"black\";\n\tlabel=\"func_%d ()\";\n",
>   		       func->idx, func->idx);
> -		func_output_bb_def(func);
> +		func_output_bb_def(func, dd);
>   		func_output_edges(func);
>   		printf("}\n");
>   	}
>   	printf("}\n");
>   }

> -void dump_xlated_cfg(void *buf, unsigned int len)
> +void dump_xlated_cfg(struct dump_data *dd, void *buf, unsigned int len)
>   {
>   	struct bpf_insn *insn = buf;
>   	struct cfg cfg;
> @@ -479,7 +476,7 @@ void dump_xlated_cfg(void *buf, unsigned int len)
>   	if (cfg_build(&cfg, insn, len))
>   		return;

> -	cfg_dump(&cfg);
> +	cfg_dump(&cfg, dd);

>   	cfg_destroy(&cfg);
>   }
> diff --git a/tools/bpf/bpftool/cfg.h b/tools/bpf/bpftool/cfg.h
> index e144257ea6d2..909d17e6d4c2 100644
> --- a/tools/bpf/bpftool/cfg.h
> +++ b/tools/bpf/bpftool/cfg.h
> @@ -4,6 +4,8 @@
>   #ifndef __BPF_TOOL_CFG_H
>   #define __BPF_TOOL_CFG_H

> -void dump_xlated_cfg(void *buf, unsigned int len);
> +#include "xlated_dumper.h"
> +
> +void dump_xlated_cfg(struct dump_data *dd, void *buf, unsigned int len);

>   #endif /* __BPF_TOOL_CFG_H */
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index 0ef373cef4c7..e9ee514b22d4 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -229,6 +229,8 @@ void btf_dump_linfo_plain(const struct btf *btf,
>   			  const char *prefix, bool linum);
>   void btf_dump_linfo_json(const struct btf *btf,
>   			 const struct bpf_line_info *linfo, bool linum);
> +void btf_dump_linfo_dotlabel(const struct btf *btf,
> +			     const struct bpf_line_info *linfo);

>   struct nlattr;
>   struct ifinfomsg;
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index afbe3ec342c8..d855118f0d96 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -840,11 +840,6 @@ prog_dump(struct bpf_prog_info *info, enum dump_mode  
> mode,
>   					      false))
>   				goto exit_free;
>   		}
> -	} else if (visual) {
> -		if (json_output)
> -			jsonw_null(json_wtr);
> -		else
> -			dump_xlated_cfg(buf, member_len);
>   	} else {
>   		kernel_syms_load(&dd);
>   		dd.nr_jited_ksyms = info->nr_jited_ksyms;
> @@ -854,12 +849,14 @@ prog_dump(struct bpf_prog_info *info, enum  
> dump_mode mode,
>   		dd.finfo_rec_size = info->func_info_rec_size;
>   		dd.prog_linfo = prog_linfo;

> -		if (json_output)
> -			dump_xlated_json(&dd, buf, member_len, opcodes,
> -					 linum);
> +		if (json_output && visual)
> +			jsonw_null(json_wtr);
> +		else if (json_output)
> +			dump_xlated_json(&dd, buf, member_len, opcodes, linum);
> +		else if (visual)
> +			dump_xlated_cfg(&dd, buf, member_len);
>   		else
> -			dump_xlated_plain(&dd, buf, member_len, opcodes,
> -					  linum);
> +			dump_xlated_plain(&dd, buf, member_len, opcodes, linum);
>   		kernel_syms_destroy(&dd);
>   	}

> diff --git a/tools/bpf/bpftool/xlated_dumper.c  
> b/tools/bpf/bpftool/xlated_dumper.c
> index 3daa05d9bbb7..5fbe94aa8589 100644
> --- a/tools/bpf/bpftool/xlated_dumper.c
> +++ b/tools/bpf/bpftool/xlated_dumper.c
> @@ -369,20 +369,50 @@ void dump_xlated_for_graph(struct dump_data *dd,  
> void *buf_start, void *buf_end,
>   		.cb_imm		= print_imm,
>   		.private_data	= dd,
>   	};
> +	const struct bpf_prog_linfo *prog_linfo = dd->prog_linfo;
> +	const struct bpf_line_info *last_linfo = NULL;
> +	struct bpf_func_info *record = dd->func_info;
>   	struct bpf_insn *insn_start = buf_start;
>   	struct bpf_insn *insn_end = buf_end;
>   	struct bpf_insn *cur = insn_start;
> +	struct btf *btf = dd->btf;
>   	bool double_insn = false;
> +	char func_sig[1024];

>   	for (; cur <= insn_end; cur++) {
> +		unsigned int insn_off;
> +
>   		if (double_insn) {
>   			double_insn = false;
>   			continue;
>   		}
>   		double_insn = cur->code == (BPF_LD | BPF_IMM | BPF_DW);

> -		printf("% 4d: ", (int)(cur - insn_start + start_idx));
> +		insn_off = (unsigned int)(cur - insn_start + start_idx);
> +		if (btf && record) {
> +			if (record->insn_off == insn_off) {
> +				btf_dumper_type_only(btf, record->type_id,
> +						     func_sig,
> +						     sizeof(func_sig));
> +				if (func_sig[0] != '\0')
> +					printf("; %s:\\l\\\n", func_sig);
> +				record = (void *)record + dd->finfo_rec_size;
> +			}
> +		}
> +
> +		if (prog_linfo) {
> +			const struct bpf_line_info *linfo;
> +
> +			linfo = bpf_prog_linfo__lfind(prog_linfo, insn_off, 0);
> +			if (linfo && linfo != last_linfo) {
> +				btf_dump_linfo_dotlabel(btf, linfo);
> +				last_linfo = linfo;
> +			}
> +		}
> +
> +		printf("%d: ", insn_off);
>   		print_bpf_insn(&cbs, cur, true);
> +
>   		if (cur != insn_end)
>   			printf(" | ");
>   	}
> --
> 2.34.1