Re: [PATCH bpf-next 19/20] selftests/bpf: Cover the exception cleanup shapes the chain does not reach

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] bpf: multi-line comments violate BPF subsystem style
--

commit 621c13df81280f1c0c32e09255ce8c64860bd768
Author: Yonghong Song <yonghong.song@linux.dev>

selftests/bpf: Cover the exception cleanup shapes the chain does not reach
    
The end-to-end test walks one call chain with a pad in most of its frames.
This adds the shapes that chain does not reach: a callee called from both a
covered and an uncovered site, a pad that reads its frame's callee-saved
registers, a tail-call-reachable callee, a tail call that is taken, an
extension standing in for a covered call, a pad in the main program's own
frame, a record covering bpf_throw() itself, a record covering only a
nounwind call, a throwing subprogram named by a BPF_PSEUDO_FUNC, a region
ending on a 16-byte instruction, a pad that reloads from and writes to its
own frame, a pad two frames up, a pad that calls a subprogram which tail
calls, an extension carrying a table of its own, and a pad terminated by
_Unwind_Resume rather than bpf_unwind_resume.

> diff --git a/tools/testing/selftests/bpf/prog_tests/exceptions_cleanup.c b/tools/testing/selftests/bpf/prog_tests/exceptions_cleanup.c
> index d1e45b765af26..ffc0b95191681 100644
> --- a/tools/testing/selftests/bpf/prog_tests/exceptions_cleanup.c
> +++ b/tools/testing/selftests/bpf/prog_tests/exceptions_cleanup.c
> @@ -212,6 +212,9 @@ static void test_shapes(void)
>  
[ ... ]
> +	/* The same call site either way: the subprogram's throw unwinds into
> +	 * this frame and runs its pad, an extension's stops at its own boundary.
> +	 */

[Severity: Low]
Does this multi-line comment in test_shapes() violate the BPF subsystem style?

The style guidelines prefer the opening /* on its own line:

	/*
	 * The same call site either way: the subprogram's throw unwinds into
	 * this frame and runs its pad, an extension's stops at its own boundary.
	 */

[ ... ]

> +	/* A tail call that is taken: the walk ends at the target, so the cookie
> +	 * comes back from there and this frame's pad does not run.
> +	 */

[Severity: Low]
Could this multi-line comment in test_shapes() also be updated to start the text
on the line following the opening /* to match the preferred BPF style?

> diff --git a/tools/testing/selftests/bpf/progs/exceptions_cleanup_shapes.c b/tools/testing/selftests/bpf/progs/exceptions_cleanup_shapes.c
> new file mode 100644
> index 0000000000000..f5eb2ff15c896
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/exceptions_cleanup_shapes.c
> @@ -214,6 +214,9 @@ static __used __noinline __u64 tc_thrower(void *ctx)
>  {
> +	/* Never taken; its presence is what makes this frame, whose spill the
> +	 * walker reads, tail-call-reachable.
> +	 */

[Severity: Low]
Is this multi-line comment in tc_thrower() formatted correctly?

The BPF subsystem style requires the opening /* on its own line for multi-line
comments.

[ ... ]

> @@ -311,6 +311,9 @@ static __used __noinline __u64 tc_taken_callee(void *ctx, __u64 x)
>  {
> +	/* Never true at run time; the verifier cannot know that, and its
> +	 * unwind out of here is what keeps the caller's pad alive.
> +	 */

[Severity: Low]
Should this multi-line comment in tc_taken_callee() also be adjusted so the text
starts on the next line after the opening /* marker?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917055645.3926444-1-yonghong.song@linux.dev?part=19