Re: LLVM bug when storing unpacked struct?

[Yonghong Song <yhs@fb.com>]

On 6/2/21 9:57 AM, Julian P Samaroo wrote:
> This is my first LKML email, so let me know if I'm doing something wrong! :)
> 
> I believe I've found a bug in LLVM's generation of BPF bytecode, and would like
> to get advice on whether this is truly a bug before considering writing a
> patch.
> 
> When storing an unpacked struct such as { i64, i32 } to the stack (as part of
> writing a struct-typed map key), LLVM 11.0.1 generates BPF bytecode like the
> following:
> 
> ...
> 2: (b7) r1 = 2
> 3: (63) *(u32 *)(r10 -24) = r1
> 4: (b7) r1 = 4
> 5: (7b) *(u64 *)(r10 -32) = r1
> ...
> 8: (bf) r3 = r10
> 9: (07) r3 += -32
> ...
> 13: (85) call bpf_map_update_elem#2
> invalid indirect read from stack off -32+12 size 16
> 
> The verifier understandably complains about this when verifying a call that
> uses these stack slots, such as bpf_map_update_elem, because the associated map
> definition has a key size of 16 bytes, not 12 bytes as this bytecode would
> suggest. In my particular case that generated this code, my frontend doesn't
> have the notion of packed structs, so I can't workaround this by making the
> struct packed.
> 
> My belief is that for unpacked structs, LLVM should emit these stores as 64-bit
> stores, which should be OK since the padding bytes are going to be zero (from
> my limited understanding of LLVM structs). Does this seem like a reasonable

Your assumption about padding bytes to be zero is not correct. Except 
explicitly requesting to fill padding bytes with zero e.g., using
__builtin_memset(), the compiler doesn't need to write to padding bytes.
So this is not a compiler bug.

The best approach is to do manual padding or using __builtin_memset()
before assigning values to each individual field.

> change to make? I'm also unable to test this on LLVM 12 (my language hasn't yet
> updated to support that version), so this could have possibly already been
> fixed; please let me know if so!
> 
> Julian
>