diff mbox series

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

Message ID 20240603155308.199254-3-cupertino.miranda@oracle.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Regular expression support for test output matching | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 15 maintainers not CCed: jolsa@kernel.org shuah@kernel.org john.fastabend@gmail.com ast@kernel.org haoluo@google.com song@kernel.org mykolal@fb.com martin.lau@linux.dev daniel@iogearbox.net andrii@kernel.org sdf@google.com memxor@gmail.com kpsingh@kernel.org davemarchevsky@fb.com linux-kselftest@vger.kernel.org
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch warning WARNING: 'Signed-off-by:' is the preferred signature form WARNING: line length of 105 exceeds 80 columns WARNING: line length of 114 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Cupertino Miranda June 3, 2024, 3:53 p.m. UTC 
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(-)

Comments

Andrii Nakryiko June 4, 2024, 6:16 p.m. UTC | #1 
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?

>  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")
> +__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?

>  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
>
Cupertino Miranda June 6, 2024, 10:50 a.m. UTC | #2 
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")
>> +__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.
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
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.

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
>>
Alexei Starovoitov June 6, 2024, 3:50 p.m. UTC | #3 
On Thu, Jun 6, 2024 at 3:51 AM Cupertino Miranda
<cupertino.miranda@oracle.com> wrote:
>
> 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
> 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.

Not really.
Allocating vars in different order within a section is fine,
but compilers are not allowed to reorder fields within structs.
There is a plugin for gcc that allows opt-in via
__attribute__((randomize_layout)).
But never by default.
Andrii Nakryiko June 6, 2024, 5:19 p.m. UTC | #4 
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")
> >> +__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.

> 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
> >>
Eduard Zingerman June 6, 2024, 5:47 p.m. UTC | #5 
On Thu, 2024-06-06 at 10:19 -0700, Andrii Nakryiko wrote:

[...]

> > 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.

I'll chime in as well :)
Could you please show a few examples when GCC does reordering?
As Alexei and Andrii point out in general C language standard does not
allow reordering for fields, e.g. here is a wording from section
6.7.2.1, paragraph 17 of "WG 14/N 3088, Programming languages — C":

> Within a structure object, the non-bit-field members and the units
> in which bit-fields reside have addresses that increase in the order
> in which they are declared. A pointer to a structure object,
> suitably converted, points to its initial member (or if that member
> is a bit-field, then to the unit in which it resides), and vice
> versa. There may be unnamed padding within a structure object, but
> not at its beginning.

So, I'm curious what's happening.
Cupertino Miranda June 6, 2024, 6:07 p.m. UTC | #6 
Alexei Starovoitov writes:

> On Thu, Jun 6, 2024 at 3:51 AM Cupertino Miranda
> <cupertino.miranda@oracle.com> wrote:
>>
>> 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
>> 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.
>
> Not really.
> Allocating vars in different order within a section is fine,
> but compilers are not allowed to reorder fields within structs.
> There is a plugin for gcc that allows opt-in via
> __attribute__((randomize_layout)).
> But never by default.

Apologies for the mess up. Indeed the reordering happens on variable
declarations. In prog/test_core_autosize.c, it declares variables and in
prog_tests/core_autosize.c it checks content with a "template" struct
that should map how the data is layout in memory.
Somehow I miss remembered the actual problem and got confused with what
was actually being reorderd.

In the end both examples are the same.
Cupertino Miranda June 6, 2024, 6:35 p.m. UTC | #7 
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
>> >>
Jose E. Marchesi June 6, 2024, 7:27 p.m. UTC | #8 
> On Thu, 2024-06-06 at 10:19 -0700, Andrii Nakryiko wrote:
>
> [...]
>
>> > 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.
>
> I'll chime in as well :)
> Could you please show a few examples when GCC does reordering?
> As Alexei and Andrii point out in general C language standard does not
> allow reordering for fields, e.g. here is a wording from section
> 6.7.2.1, paragraph 17 of "WG 14/N 3088, Programming languages — C":

GCC does not reorder struct fields.
The option -ftoplevel-reorder enables reordering of data declarations.

>> Within a structure object, the non-bit-field members and the units
>> in which bit-fields reside have addresses that increase in the order
>> in which they are declared. A pointer to a structure object,
>> suitably converted, points to its initial member (or if that member
>> is a bit-field, then to the unit in which it resides), and vice
>> versa. There may be unnamed padding within a structure object, but
>> not at its beginning.
>
> So, I'm curious what's happening.
diff mbox series

Patch

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")
 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")
+__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]+")
 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]+")
 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)
 {
 	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]+")
 __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 ("					\