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@xxxxxxxxxxxxx>
> Acked-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
> ---
> 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(" | ");
> }
