Re: [PATCH] bpftool: fix line info in multi-function CFG dumps

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 3172 bytes --]

> bpftool: fix line info in multi-function CFG dumps

TL;DR:
- Selftest's expected string likely never matches: it doesn't escape '>' in
  "skb->len", unlike the DOT output
- Even if fixed, the check may not distinguish fixed from old code

> diff --git a/tools/testing/selftests/bpf/prog_tests/bpftool_cfg.c b/tools/testing/selftests/bpf/prog_tests/bpftool_cfg.c
> new file mode 100644
> index 000000000000..7d5b80927b65
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/bpftool_cfg.c
> @@ -0,0 +1,41 @@

[ ... ]

> +void test_bpftool_cfg(void)
> +{
> +	struct bpftool_cfg *skel = NULL;
> +	struct bpf_prog_info info = {};
> +	__u32 info_len = sizeof(info);
> +	char output[MAX_BPFTOOL_OUTPUT_LEN];
> +	char cmd[MAX_BPFTOOL_CMD_LEN];
> +	const char *expected = "return\\ bpf_ktime_get_ns()\\ ?\\ skb->len\\ :\\ 0;";
> +	int err;

Can this expected string ever match the bpftool output?

The DOT output prints source lines through btf_dump_linfo_dotlabel() ->
dotlabel_puts(), which escapes '>' as well as ' ':

    static void dotlabel_puts(const char *s)
    {
        for (; *s; ++s) {
            switch (*s) {
            ...
            case '<':
            case '>':
            case '|':
            case ' ':
                putchar('\\');

So the line "return bpf_ktime_get_ns() ? skb->len : 0;" is printed as:

    return\ bpf_ktime_get_ns()\ ?\ skb-\>len\ :\ 0;

The expected string escapes the spaces but not the '>', so it contains
"skb->len" where the output has "skb-\>len".  Would the string need to be
"skb-\\>len" for the strstr() in ASSERT_HAS_SUBSTR() to find it?

As far as I can tell, no other part of the visual output contains this source
text unescaped, since print_insn_for_graph() escapes '>' as well.

[ ... ]

> +	err = get_bpftool_command_output(cmd, output, sizeof(output));
> +	if (ASSERT_OK(err, "dump program CFG")) {
> +		ASSERT_HAS_SUBSTR(output, "cluster_1", "second function in CFG");
> +		ASSERT_HAS_SUBSTR(output, expected, "second function line info");

Even with the escaping corrected, can this check tell the fixed code from the
old code?

In this program the main function is only two instructions (call; exit) and
helper() is longer.  With the old function-relative lookup, the basic blocks
of helper() at relative index 2 or higher already resolve to helper()'s own
line info records, because their global insn_off is also 2 or higher, and
those records all point at this same source line:

    return bpf_ktime_get_ns() ? skb->len : 0;

So that line probably appears in cluster_1 before the fix as well.

The commit message says the selftest "checks the second function source line
in visual output".  Would it discriminate better to check that the main
function's line, "return\ helper(skb);", does not appear in cluster_1, or that
the helper() line comes right before the first instruction of the cluster?

> +	}
> +
> +cleanup:
> +	bpftool_cfg__destroy(skel);
> +}


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/40071369261