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

[Eduard Zingerman <eddyz87@gmail.com>]

On Mon, 2023-03-27 at 12:06 +0100, 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>
> Acked-by: Stanislav Fomichev <sdf@google.com>
> ---
>  tools/bpf/bpftool/btf_dumper.c    | 32 +++++++++++++++++++++++++++++++
>  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, 85 insertions(+), 25 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c
> index e7f6ec3a8f35..8bfc1b69497d 100644
> --- a/tools/bpf/bpftool/btf_dumper.c
> +++ b/tools/bpf/bpftool/btf_dumper.c
> @@ -821,3 +821,35 @@ void btf_dump_linfo_json(const struct btf *btf,
>  					BPF_LINE_INFO_LINE_COL(linfo->line_col));
>  	}
>  }
> +
> +static void dotlabel_puts(const char *s)
> +{
> +	for (; *s; ++s) {
> +		switch (*s) {
> +		case '\\':
> +		case '"':
> +		case '{':
> +		case '}':
> +		case '>':

The "case '<':" is missing, w/o it dot reports warnings as follows:

  Error: bad label format {; if (hdr + hdr_size <= data_end)...

I used existing bpt testcase for testing:

  $ cd <kernel>/tools/testing/selftests/bpf
  $ bpftool prog load bpf_flow.bpf.o /sys/fs/bpf/test-prog
  $ prog dump xlated pinned /sys/fs/bpf/test-prog visual > test.cfg
  $ dot -Tpng -O test.cfg
  
Also [1] says the following:

> Braces, vertical bars and angle brackets must be escaped with a
> backslash character if you wish them to appear as a literal
> character. Spaces are interpreted as separators between tokens, so
> they must be escaped if you want spaces in the text.

So, maybe escape spaces as well?

[1] https://graphviz.org/doc/info/shapes.html#record

> +		case '|':
> +			putchar('\\');
> +			__fallthrough;
> +		default:
> +			putchar(*s);
> +		}
> +	}
> +}
> +
> +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);

Should this be an error? Maybe check that json_output is false when
arguments are parsed and 'visual' is specified?

> +		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(" | ");
>  	}