On Mon, 2023-03-27 at 12:06 +0100, Quentin Monnet wrote:
> Add support for displaying opcodes or/and file references (filepath,
> line and column numbers) when dumping the control flow graphs of loaded
> BPF programs with bpftool.
>
> Signed-off-by: Quentin Monnet <quentin@xxxxxxxxxxxxx>
> Acked-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
> ---
> tools/bpf/bpftool/btf_dumper.c | 19 ++++++++++++++++++-
> tools/bpf/bpftool/cfg.c | 22 ++++++++++++++--------
> tools/bpf/bpftool/cfg.h | 3 ++-
> tools/bpf/bpftool/main.h | 2 +-
> tools/bpf/bpftool/prog.c | 2 +-
> tools/bpf/bpftool/xlated_dumper.c | 15 +++++++++++++--
> tools/bpf/bpftool/xlated_dumper.h | 3 ++-
> 7 files changed, 51 insertions(+), 15 deletions(-)
>
> diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c
> index 8bfc1b69497d..24835c3f9a1c 100644
> --- a/tools/bpf/bpftool/btf_dumper.c
> +++ b/tools/bpf/bpftool/btf_dumper.c
> @@ -841,7 +841,7 @@ static void dotlabel_puts(const char *s)
> }
>
> void btf_dump_linfo_dotlabel(const struct btf *btf,
> - const struct bpf_line_info *linfo)
> + const struct bpf_line_info *linfo, bool linum)
> {
> const char *line = btf__name_by_offset(btf, linfo->line_off);
>
> @@ -849,6 +849,23 @@ void btf_dump_linfo_dotlabel(const struct btf *btf,
> return;
> line = ltrim(line);
>
> + if (linum) {
> + const char *file = btf__name_by_offset(btf, linfo->file_name_off);
> +
> + /* More forgiving on file because linum option is
> + * expected to provide more info than the already
> + * available src line.
> + */
> + if (!file)
> + file = "";
> +
> + printf("; [file:");
> + dotlabel_puts(file);
> + printf("line_num:%u line_col:%u]\\l\\\n",
Space between file name and 'line_num' is missing.
Also, at-least for BPF test-cases the labels might become quite long,
which makes graph unnecessarily wide, e.g.:
; [file:/home/eddy/work/bpf-next/tools/testing/selftests/bpf/progs/bpf_flow.cline_num:97 line_col:34]\l\
The file names are encoded in full during compilation, but maybe
shorten long file names by removing some preceding levels
(and use shorter tags 'line:', 'col:', is 'file:' tag necessary at all?).
For example:
; [.../progs/bpf_flow.c line:97 col:34]\l\.
> + BPF_LINE_INFO_LINE_NUM(linfo->line_col),
> + BPF_LINE_INFO_LINE_COL(linfo->line_col));
> + }
> +
> printf("; ");
> dotlabel_puts(line);
> printf("\\l\\\n");
> diff --git a/tools/bpf/bpftool/cfg.c b/tools/bpf/bpftool/cfg.c
> index 9fdc1f0cdd6e..eec437cca2ea 100644
> --- a/tools/bpf/bpftool/cfg.c
> +++ b/tools/bpf/bpftool/cfg.c
> @@ -381,7 +381,8 @@ static void cfg_destroy(struct cfg *cfg)
> }
>
> static void
> -draw_bb_node(struct func_node *func, struct bb_node *bb, struct dump_data *dd)
> +draw_bb_node(struct func_node *func, struct bb_node *bb, struct dump_data *dd,
> + bool opcodes, bool linum)
> {
> const char *shape;
>
> @@ -401,7 +402,8 @@ draw_bb_node(struct func_node *func, struct bb_node *bb, struct dump_data *dd)
> unsigned int start_idx;
> printf("{\\\n");
> start_idx = bb->head - func->start;
> - dump_xlated_for_graph(dd, bb->head, bb->tail, start_idx);
> + dump_xlated_for_graph(dd, bb->head, bb->tail, start_idx,
> + opcodes, linum);
> printf("}");
> }
>
> @@ -427,12 +429,14 @@ static void draw_bb_succ_edges(struct func_node *func, struct bb_node *bb)
> }
> }
>
> -static void func_output_bb_def(struct func_node *func, struct dump_data *dd)
> +static void
> +func_output_bb_def(struct func_node *func, struct dump_data *dd,
> + bool opcodes, bool linum)
> {
> struct bb_node *bb;
>
> list_for_each_entry(bb, &func->bbs, l) {
> - draw_bb_node(func, bb, dd);
> + draw_bb_node(func, bb, dd, opcodes, linum);
> }
> }
>
> @@ -452,7 +456,8 @@ 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, struct dump_data *dd)
> +static void
> +cfg_dump(struct cfg *cfg, struct dump_data *dd, bool opcodes, bool linum)
> {
> struct func_node *func;
>
> @@ -460,14 +465,15 @@ static void cfg_dump(struct cfg *cfg, struct dump_data *dd)
> 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, dd);
> + func_output_bb_def(func, dd, opcodes, linum);
> func_output_edges(func);
> printf("}\n");
> }
> printf("}\n");
> }
>
> -void dump_xlated_cfg(struct dump_data *dd, void *buf, unsigned int len)
> +void dump_xlated_cfg(struct dump_data *dd, void *buf, unsigned int len,
> + bool opcodes, bool linum)
> {
> struct bpf_insn *insn = buf;
> struct cfg cfg;
> @@ -476,7 +482,7 @@ void dump_xlated_cfg(struct dump_data *dd, void *buf, unsigned int len)
> if (cfg_build(&cfg, insn, len))
> return;
>
> - cfg_dump(&cfg, dd);
> + cfg_dump(&cfg, dd, opcodes, linum);
>
> cfg_destroy(&cfg);
> }
> diff --git a/tools/bpf/bpftool/cfg.h b/tools/bpf/bpftool/cfg.h
> index 909d17e6d4c2..b3793f4e1783 100644
> --- a/tools/bpf/bpftool/cfg.h
> +++ b/tools/bpf/bpftool/cfg.h
> @@ -6,6 +6,7 @@
>
> #include "xlated_dumper.h"
>
> -void dump_xlated_cfg(struct dump_data *dd, void *buf, unsigned int len);
> +void dump_xlated_cfg(struct dump_data *dd, void *buf, unsigned int len,
> + bool opcodes, bool linum);
>
> #endif /* __BPF_TOOL_CFG_H */
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index e9ee514b22d4..00d11ca6d3f2 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -230,7 +230,7 @@ void btf_dump_linfo_plain(const struct btf *btf,
> 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);
> + const struct bpf_line_info *linfo, bool linum);
>
> struct nlattr;
> struct ifinfomsg;
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index 567ac37dbd86..848f57a7d762 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -854,7 +854,7 @@ prog_dump(struct bpf_prog_info *info, enum dump_mode mode,
> else if (json_output)
> dump_xlated_json(&dd, buf, member_len, opcodes, linum);
> else if (visual)
> - dump_xlated_cfg(&dd, buf, member_len);
> + dump_xlated_cfg(&dd, buf, member_len, opcodes, linum);
> else
> 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 5fbe94aa8589..c5e03833fadf 100644
> --- a/tools/bpf/bpftool/xlated_dumper.c
> +++ b/tools/bpf/bpftool/xlated_dumper.c
> @@ -361,7 +361,8 @@ void dump_xlated_plain(struct dump_data *dd, void *buf, unsigned int len,
> }
>
> void dump_xlated_for_graph(struct dump_data *dd, void *buf_start, void *buf_end,
> - unsigned int start_idx)
> + unsigned int start_idx,
> + bool opcodes, bool linum)
> {
> const struct bpf_insn_cbs cbs = {
> .cb_print = print_insn_for_graph,
> @@ -405,7 +406,7 @@ void dump_xlated_for_graph(struct dump_data *dd, void *buf_start, void *buf_end,
>
> linfo = bpf_prog_linfo__lfind(prog_linfo, insn_off, 0);
> if (linfo && linfo != last_linfo) {
> - btf_dump_linfo_dotlabel(btf, linfo);
> + btf_dump_linfo_dotlabel(btf, linfo, linum);
> last_linfo = linfo;
> }
> }
> @@ -413,6 +414,16 @@ void dump_xlated_for_graph(struct dump_data *dd, void *buf_start, void *buf_end,
> printf("%d: ", insn_off);
> print_bpf_insn(&cbs, cur, true);
>
> + if (opcodes) {
> + printf(" ");
These spaces are treated as a single space by the dot renderer, as [1]
says: "Spaces are interpreted as separators between tokens, so they
must be escaped if you want spaces in the text."
[1] https://graphviz.org/doc/info/shapes.html#record
> + fprint_hex(stdout, cur, 8, " ");
> + if (double_insn && cur <= insn_end - 1) {
> + printf(" ");
> + fprint_hex(stdout, cur + 1, 8, " ");
> + }
> + printf("\\l\\\n");
> + }
> +
> if (cur != insn_end)
> printf(" | ");
> }
> diff --git a/tools/bpf/bpftool/xlated_dumper.h b/tools/bpf/bpftool/xlated_dumper.h
> index 54847e174273..9a946377b0e6 100644
> --- a/tools/bpf/bpftool/xlated_dumper.h
> +++ b/tools/bpf/bpftool/xlated_dumper.h
> @@ -34,6 +34,7 @@ void dump_xlated_json(struct dump_data *dd, void *buf, unsigned int len,
> void dump_xlated_plain(struct dump_data *dd, void *buf, unsigned int len,
> bool opcodes, bool linum);
> void dump_xlated_for_graph(struct dump_data *dd, void *buf, void *buf_end,
> - unsigned int start_index);
> + unsigned int start_index,
> + bool opcodes, bool linum);
>
> #endif
