Re: [PATCH bpf-next 2/2] selftests/bpf: Match tests against regular expression.

[Cupertino Miranda <cupertino.miranda@oracle.com>]

Andrii Nakryiko writes:

> On Thu, Jun 6, 2024 at 3:50 AM Cupertino Miranda
> <cupertino.miranda@oracle.com> wrote:
>>
>>
>> Andrii Nakryiko writes:
>>
>> > On Mon, Jun 3, 2024 at 8:53 AM Cupertino Miranda
>> > <cupertino.miranda@oracle.com> wrote:
>> >>
>> >> This patch changes a few tests to make use of regular expressions such
>> >> that the test validation would allow to properly verify the tests when
>> >> compiled with GCC.
>> >>
>> >> signed-off-by: Cupertino Miranda <cupertino.miranda@oracle.com>
>> >> Cc: jose.marchesi@oracle.com
>> >> Cc: david.faust@oracle.com
>> >> Cc: Yonghong Song <yonghong.song@linux.dev>
>> >> Cc: Eduard Zingerman <eddyz87@gmail.com>
>> >> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
>> >> ---
>> >>  tools/testing/selftests/bpf/progs/dynptr_fail.c          | 6 +++---
>> >>  tools/testing/selftests/bpf/progs/exceptions_assert.c    | 8 ++++----
>> >>  tools/testing/selftests/bpf/progs/rbtree_fail.c          | 8 ++++----
>> >>  tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c | 4 ++--
>> >>  tools/testing/selftests/bpf/progs/verifier_sock.c        | 4 ++--
>> >>  5 files changed, 15 insertions(+), 15 deletions(-)
>> >>
>> >> diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
>> >> index 66a60bfb5867..64cc9d936a13 100644
>> >> --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
>> >> +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
>> >> @@ -964,7 +964,7 @@ int dynptr_invalidate_slice_reinit(void *ctx)
>> >>   * mem_or_null pointers.
>> >>   */
>> >>  SEC("?raw_tp")
>> >> -__failure __msg("R1 type=scalar expected=percpu_ptr_")
>> >> +__failure __regex("R[0-9]+ type=scalar expected=percpu_ptr_")
>> >>  int dynptr_invalidate_slice_or_null(void *ctx)
>> >>  {
>> >>         struct bpf_dynptr ptr;
>> >> @@ -982,7 +982,7 @@ int dynptr_invalidate_slice_or_null(void *ctx)
>> >>
>> >>  /* Destruction of dynptr should also any slices obtained from it */
>> >>  SEC("?raw_tp")
>> >> -__failure __msg("R7 invalid mem access 'scalar'")
>> >> +__failure __regex("R[0-9]+ invalid mem access 'scalar'")
>> >>  int dynptr_invalidate_slice_failure(void *ctx)
>> >>  {
>> >>         struct bpf_dynptr ptr1;
>> >> @@ -1069,7 +1069,7 @@ int dynptr_read_into_slot(void *ctx)
>> >>
>> >>  /* bpf_dynptr_slice()s are read-only and cannot be written to */
>> >>  SEC("?tc")
>> >> -__failure __msg("R0 cannot write into rdonly_mem")
>> >> +__failure __regex("R[0-9]+ cannot write into rdonly_mem")
>> >>  int skb_invalid_slice_write(struct __sk_buff *skb)
>> >>  {
>> >>         struct bpf_dynptr ptr;
>> >> diff --git a/tools/testing/selftests/bpf/progs/exceptions_assert.c b/tools/testing/selftests/bpf/progs/exceptions_assert.c
>> >> index 5e0a1ca96d4e..deb67d198caf 100644
>> >> --- a/tools/testing/selftests/bpf/progs/exceptions_assert.c
>> >> +++ b/tools/testing/selftests/bpf/progs/exceptions_assert.c
>> >> @@ -59,7 +59,7 @@ check_assert(s64, >=, ge_neg, INT_MIN);
>> >>
>> >>  SEC("?tc")
>> >>  __log_level(2) __failure
>> >> -__msg(": R0=0 R1=ctx() R2=scalar(smin=0xffffffff80000002,smax=smax32=0x7ffffffd,smin32=0x80000002) R10=fp0")
>> >> +__regex(": R0=[^ ]+ R1=ctx() R2=scalar(smin=0xffffffff80000002,smax=smax32=0x7ffffffd,smin32=0x80000002) R10=fp0")
>> >
>> > curious, what R0 value do we end up with with GCC generated code?
>> Oups, this file should have not been committed. Those changes were just
>> for experimentation, nothing else. :(
>>
>> >
>> >>  int check_assert_range_s64(struct __sk_buff *ctx)
>> >>  {
>> >>         struct bpf_sock *sk = ctx->sk;
>> >> @@ -75,7 +75,7 @@ int check_assert_range_s64(struct __sk_buff *ctx)
>> >>
>> >>  SEC("?tc")
>> >>  __log_level(2) __failure
>> >> -__msg(": R1=ctx() R2=scalar(smin=umin=smin32=umin32=4096,smax=umax=smax32=umax32=8192,var_off=(0x0; 0x3fff))")
>> >> +__regex("R[0-9]=scalar(smin=umin=smin32=umin32=4096,smax=umax=smax32=umax32=8192,var_off=(0x0; 0x3fff))")
>> >>  int check_assert_range_u64(struct __sk_buff *ctx)
>> >>  {
>> >>         u64 num = ctx->len;
>> >> @@ -86,7 +86,7 @@ int check_assert_range_u64(struct __sk_buff *ctx)
>> >>
>> >>  SEC("?tc")
>> >>  __log_level(2) __failure
>> >> -__msg(": R0=0 R1=ctx() R2=4096 R10=fp0")
>> >> +__regex(": R0=[^ ]+ R1=ctx() R2=4096 R10=fp0")
>> >>  int check_assert_single_range_s64(struct __sk_buff *ctx)
>> >>  {
>> >>         struct bpf_sock *sk = ctx->sk;
>> >> @@ -114,7 +114,7 @@ int check_assert_single_range_u64(struct __sk_buff *ctx)
>> >>
>> >>  SEC("?tc")
>> >>  __log_level(2) __failure
>> >> -__msg(": R1=pkt(off=64,r=64) R2=pkt_end() R6=pkt(r=64) R10=fp0")
>> >> +__msg("R1=pkt(off=64,r=64)")
>> >>  int check_assert_generic(struct __sk_buff *ctx)
>> >>  {
>> >>         u8 *data_end = (void *)(long)ctx->data_end;
>> >> diff --git a/tools/testing/selftests/bpf/progs/rbtree_fail.c b/tools/testing/selftests/bpf/progs/rbtree_fail.c
>> >> index 3fecf1c6dfe5..8399304eca72 100644
>> >> --- a/tools/testing/selftests/bpf/progs/rbtree_fail.c
>> >> +++ b/tools/testing/selftests/bpf/progs/rbtree_fail.c
>> >> @@ -29,7 +29,7 @@ static bool less(struct bpf_rb_node *a, const struct bpf_rb_node *b)
>> >>  }
>> >>
>> >>  SEC("?tc")
>> >> -__failure __msg("bpf_spin_lock at off=16 must be held for bpf_rb_root")
>> >> +__failure __regex("bpf_spin_lock at off=[0-9]+ must be held for bpf_rb_root")
>> >>  long rbtree_api_nolock_add(void *ctx)
>> >>  {
>> >>         struct node_data *n;
>> >> @@ -43,7 +43,7 @@ long rbtree_api_nolock_add(void *ctx)
>> >>  }
>> >>
>> >>  SEC("?tc")
>> >> -__failure __msg("bpf_spin_lock at off=16 must be held for bpf_rb_root")q
>> >> +__failure __regex("bpf_spin_lock at off=[0-9]+ must be held for bpf_rb_root")
>> >>  long rbtree_api_nolock_remove(void *ctx)
>> >>  {
>> >>         struct node_data *n;
>> >> @@ -61,7 +61,7 @@ long rbtree_api_nolock_remove(void *ctx)
>> >>  }
>> >>
>> >>  SEC("?tc")
>> >> -__failure __msg("bpf_spin_lock at off=16 must be held for bpf_rb_root")
>> >> +__failure __regex("bpf_spin_lock at off=[0-9]+ must be held for bpf_rb_root")
>> >>  long rbtree_api_nolock_first(void *ctx)
>> >>  {
>> >>         bpf_rbtree_first(&groot);
>> >> @@ -105,7 +105,7 @@ long rbtree_api_remove_unadded_node(void *ctx)
>> >>  }
>> >>
>> >>  SEC("?tc")
>> >> -__failure __msg("Unreleased reference id=3 alloc_insn=10")
>> >> +__failure __regex("Unreleased reference id=3 alloc_insn=[0-9]+")
>> >
>> > this test definitely should have been written in BPF assembly if we
>> > care to check alloc_insn... Otherwise we just care that there is
>> > "Unreleased reference" message, we should match on that without
>> > hard-coding id and alloc_insn?
>> I agree. Unfortunately I see a lot of tests that fall in this category.
>> I must admit, most of the time I do not know what is the proper approach
>> to correct it.
>>
>> Also found some tests that made expectations on .bss section data
>> layout, expeting a particular variable order.
>> For example in prog_tests/core_reloc.c, when it maps .bss and assigns it
>> to data.
>
> I haven't checked every single one, but I think most (if not all) of
> these progs/test_core_reloc_*.c tests (which are what is being tested
> in prog_tests/core_reloc.c) are structured with a singular variable in
> .bss. And then the variable type is some well-defined struct type. As
> Alexei pointed out, compiler is not allowed to just arbitrarily
> reorder fields within a struct, unless randomization is enabled with
> an extra attribute (which we do not use).
>
> So if you have specific cases where something isn't correct, let's go
> over them, but I think prog_tests/core_reloc.c should be fine.
>
I think it was in progs/test_core_reloc_type_id.c where you also define:

    /* preserve types even if Clang doesn't support built-in */
    struct a_struct t1 = {};
    union a_union t2 = {};
    enum an_enum t3 = 0;
    named_struct_typedef t4 = {};
    func_proto_typedef t5 = 0;
    arr_typedef t6 = {};


>> GCC will allocate variables in a different order then clang and when
>> comparing content is not where comparisson is expecting.
>>
>> Some other test, would expect that struct fields would be in some
>> particular order, while GCC decides it would benefit from reordering
>> struct fields. For passing those tests I need to disable GCC
>> optimization that would make this reordering.
>> However reordering of the struct fields is a perfectly valid
>
> Nope, it's not.
>
> As mentioned, struct layout is effectively an ABI, so the compiler
> cannot just reorder it. Lots and lots of things would be broken if
> this was true for C programs.
>
>> optimization. Maybe disabling for this tests is acceptable, but in any
>> case the test itself is prune for any future optimizations that can be
>> added to GCC or CLANG.
>> This happened in progs/test_core_autosize.c for example.
>
> We probably should rewrite such tests that have to deal with
> .bss/.data to BPF skeletons, I think they were written before BPF
> skeletons were available.
>
>>
>> Anyway, just a couple of examples of tests that were made very tight to
>> compiler.
>>
>> >
>> >>  long rbtree_api_remove_no_drop(void *ctx)
>> >>  {
>> >>         struct bpf_rb_node *res;
>> >> diff --git a/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c b/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c
>> >> index 1553b9c16aa7..f8d4b7cfcd68 100644
>> >> --- a/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c
>> >> +++ b/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c
>> >> @@ -32,7 +32,7 @@ static bool less(struct bpf_rb_node *a, const struct bpf_rb_node *b)
>> >>  }
>> >>
>> >>  SEC("?tc")
>> >> -__failure __msg("Unreleased reference id=4 alloc_insn=21")
>> >> +__failure __regex("Unreleased reference id=4 alloc_insn=[0-9]+")
>> >
>> > same, relying on ID and alloc_insns in tests written in C is super fragile.
>> >
>> >>  long rbtree_refcounted_node_ref_escapes(void *ctx)
>> >>  {
>> >>         struct node_acquire *n, *m;
>> >> @@ -73,7 +73,7 @@ long refcount_acquire_maybe_null(void *ctx)
>> >>  }
>> >>
>> >>  SEC("?tc")
>> >> -__failure __msg("Unreleased reference id=3 alloc_insn=9")
>> >> +__failure __regex("Unreleased reference id=3 alloc_insn=[0-9]+")
>> >>  long rbtree_refcounted_node_ref_escapes_owning_input(void *ctx)
>> >
>> > ditto
>> >
>> >>  {
>> >>         struct node_acquire *n, *m;
>> >> diff --git a/tools/testing/selftests/bpf/progs/verifier_sock.c b/tools/testing/selftests/bpf/progs/verifier_sock.c
>> >> index ee76b51005ab..450b57933c79 100644
>> >> --- a/tools/testing/selftests/bpf/progs/verifier_sock.c
>> >> +++ b/tools/testing/selftests/bpf/progs/verifier_sock.c
>> >> @@ -799,7 +799,7 @@ l0_%=:      r0 = *(u32*)(r0 + %[bpf_xdp_sock_queue_id]);    \
>> >>
>> >>  SEC("sk_skb")
>> >>  __description("bpf_map_lookup_elem(sockmap, &key)")
>> >> -__failure __msg("Unreleased reference id=2 alloc_insn=6")
>> >> +__failure __regex("Unreleased reference id=2 alloc_insn=[0-9]+")
>> >
>> > same here and below
>> >
>> >
>> >>  __naked void map_lookup_elem_sockmap_key(void)
>> >>  {
>> >>         asm volatile ("                                 \
>> >> @@ -819,7 +819,7 @@ __naked void map_lookup_elem_sockmap_key(void)
>> >>
>> >>  SEC("sk_skb")
>> >>  __description("bpf_map_lookup_elem(sockhash, &key)")
>> >> -__failure __msg("Unreleased reference id=2 alloc_insn=6")
>> >> +__failure __regex("Unreleased reference id=2 alloc_insn=[0-9]+")
>> >>  __naked void map_lookup_elem_sockhash_key(void)
>> >>  {
>> >>         asm volatile ("                                 \
>> >> --
>> >> 2.39.2
>> >>