On Fri, Dec 1, 2023 at 7:16 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Thu, 2023-11-30 at 17:30 -0800, Andrii Nakryiko wrote:
> > Add selftest that establishes dead code-eliminated valid global subprog
> > (global_dead) and makes sure that it's not possible to freplace it, as
> > it's effectively not there. This test will fail with unexpected success
> > before 2afae08c9dcb ("bpf: Validate global subprogs lazily").
> >
> > Suggested-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > ---
>
> Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
Oops, didn't see your reply before sending v2. But there will be v3 anyway :)
>
> [...]
> > diff --git a/tools/testing/selftests/bpf/prog_tests/global_func_dead_code.c b/tools/testing/selftests/bpf/prog_tests/global_func_dead_code.c
> [...]
> > +void test_global_func_dead_code(void)
> > +{
> [...]
> > + ASSERT_HAS_SUBSTR(log_buf, "Subprog global_dead doesn't exist", "dead_subprog_missing_msg");
>
> Nit: the log is not printed if verbose tests execution is requested.
I'm not sure I understand. What do you expect to happen that's not
happening in verbose mode?
>
> [...]
>
> > index a0a5efd1caa1..7f9b21a1c5a7 100644
> > --- a/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c
> > +++ b/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c
> > @@ -10,25 +10,31 @@
> >
> > int arr[1];
> > int unkn_idx;
> > +const volatile bool call_dead_subprog = false;
> >
> > -__noinline long global_bad(void)
> > +__noinline long global_bad(int x)
> > {
> > - return arr[unkn_idx]; /* BOOM */
> > + return arr[unkn_idx] + x; /* BOOM */
> > }
>
> Nit/question:
> Why change prototype from (void) to (int) here and elsewhere?
> Does not seem necessary for test logic.
I had some troubles attaching freplace initially, but my freplace
skills were rusty :) I can try undoing this and leaving it as is.
>
> [...]
