Re: [PATCH bpf-next v3 2/3] selftests/bpf: Test fetching AND/OR/XOR atomics in arena

[bot+bpf-ci@kernel.org]

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

> diff --git a/tools/testing/selftests/bpf/progs/arena_atomics.c b/tools/testing/selftests/bpf/progs/arena_atomics.c
> index 73bc2b835f3f..3beca5eab387 100644
> --- a/tools/testing/selftests/bpf/progs/arena_atomics.c
> +++ b/tools/testing/selftests/bpf/progs/arena_atomics.c

[ ... ]

> @@ -91,13 +91,22 @@ int sub(const void *ctx)
>  	return 0;
>  }
>
> -#ifdef __BPF_FEATURE_ATOMIC_MEM_ORDERING
> -_Atomic __u64 __arena_global and64_value = (0x110ull << 32);
> -_Atomic __u32 __arena_global and32_value = 0x110;
> -#else
> +/*
> + * Discarding the result of an atomic RMW is what selects the non-fetching
> + * insn, so keep one of each width per operation to cover that lowering too.
> + * Declared together rather than per operation to keep them in one place.
> + */
> +_Atomic __u64 __arena_global and64_noreturn_value = (0x110ull << 32);
> +_Atomic __u64 __arena_global or64_noreturn_value = (0x110ull << 32);
> +_Atomic __u64 __arena_global xor64_noreturn_value = (0x110ull << 32);
> +_Atomic __u32 __arena_global and32_noreturn_value = 0x110;
> +_Atomic __u32 __arena_global or32_noreturn_value = 0x110;
> +_Atomic __u32 __arena_global xor32_noreturn_value = 0x110;
> +
>  __u64 __arena_global and64_value = (0x110ull << 32);
> +__u64 __arena_global and64_result = 0;
>  __u32 __arena_global and32_value = 0x110;
> -#endif
> +__u32 __arena_global and32_result = 0;

Can this ordering still build on clang-17?

The file's existing clang-17 guard (lines 379-402) says clang-17 crashes
if the .addr_space.1 ELF section has holes, and works around this by
declaring variables as 64-bit. Before __BPF_FEATURE_ADDR_SPACE_CAST is
defined (clang 19+), __arena_global expands to SEC(".addr_space.1"), so
all of these globals go into that section.

Before this commit, the layout had no holes: add/sub (96 bytes), then
and32/and64/or32/or64/xor64/xor32 filled 96-132 continuously, then
cmpxchg32 x3 at 132-144 and cmpxchg64 at 144.

After this commit, the three _Atomic __u64 noreturn values fill 96-120
and the three _Atomic __u32 noreturn values fill 120-132. Then
and64_value needs 8-byte alignment, creating a 4-byte hole at 132-136.
The and/or/xor blocks (u64 value, u64 result, u32 value, u32 result) fill
136-208. cmpxchg32 x3 fill 208-220, then cmpxchg64_value needs 8-byte
alignment, creating a second 4-byte hole at 220-224.

clang-17's BPF backend pads these holes with code nops. BPF writeNopData
only handles multiples of 8, so the build fails with 'fatal error: error
in backend: unable to write nop sequence of 4 bytes'. This is the same
failure the x86_64-llvm-17 CI job hit when load-acquire tests were added
(see lore message Z6a_UILNqVGBqnvY@google.com; fixed only in LLVM
f27c4903c43b, clang 18+).

Could the __u32 globals be reordered to avoid these holes? For example,
moving xor32_noreturn_value from the grouped block to just after
xor32_result, next to the three cmpxchg32 values, would give runs of 2
and 6 u32 values instead of 3 + 3, eliminating both holes. Alternatively,
the existing clang-17 pattern (declaring them as 64-bit) could be used.

[ ... ]


---
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/36144954909