Prepare bpftrace for struct arguments to kfuncs

[BurntBrunch]

BPF is gaining support for struct fentry arguments. These are currently rejected in the verifier or, if you try to use kprobe, you see the argument split across multiple argX values.

The code from bpftrace 0.15 looks reasonable enough:

> bpftrace -dd -e 'kfunc:udp_setsockopt { $tmp = args->optval; }'
...
define i64 @"kfunc:udp_setsockopt"(i8* %0) section "s_kfunc:udp_setsockopt_1" {
entry:
  %"$tmp" = alloca i64, align 8
  %1 = bitcast i64* %"$tmp" to i8*
  call void @llvm.lifetime.start.p0i8(i64 -1, i8* %1)
  store i64 0, i64* %"$tmp", align 8
  %2 = ptrtoint i8* %0 to i64
  %3 = bitcast i8* %0 to i64*
  %4 = getelementptr i64, i64* %3, i64 3
  %optval = load volatile [16 x i8], i64* %4, align 1
  %5 = ptrtoint [16 x i8] %optval to i64
  store i64 %5, i64* %"$tmp", align 8
  ret i64 0
}

(This code blows up at the assertion here in debug builds.)

Interestingly, this 16 byte copy is kinda correct. However, semantically, it produces a value that cannot be used. Namely, the type of $tmp is an anonymous struct (typedef sockptr_t) with unnamed members (a union), so it cannot be passed to buf, str, printf, or print directly. -> navigation also doesn't work because it's not a pointer to a struct.

I'm opening this issue to discuss what the language semantics for these values should be.

I am of the opinion that $tmp in the above program should magically become pointer-to-struct, so everything works transparently but maybe that's too much magic.

I'm also interested to hear if we think this is worth solving for kprobe programs as well. (It can be, it's just more complexity).

[viktormalik]

I am of the opinion that $tmp in the above program should magically become pointer-to-struct, so everything works transparently but maybe that's too much magic.

Personally, I don't like this too much as we'd basically change the type of the argument (non-pointer to pointer). I'd prefer treating it as the struct (which it is) and accessing the members with . (e.g. args->optval.kernel). We already support this syntax, e.g. we can do bpftrace -e 'BEGIN { @ = curtask->thread_info.cpu }'.

In fact, the only problem I can see here is the way we handle anonymous structs at the moment. We rely on the type name to fill the correct type defs from ClangParser, which is obviously a problem. It'll be partially eliminated by resolving the structs directly in FieldAnalyser (part of #2315) and we could also store the type under the typedef name for these cases, which would resolve it completely.

Interestingly, this 16 byte copy is kinda correct.

Is it? There's a 16-byte load but after that only 8 bytes are stored (i64) into $tmp which doesn't feel correct.

[BurntBrunch]

Yeah, the store is wrong of course. This assumption needs unwinding, that whole ptrtoint cast would now be wrong:

https://github.com/iovisor/bpftrace/blob/833ca149e8e1f41a0d8b13826f8261c1b75673a4/src/ast/passes/codegen_llvm.cpp#L2090-L2097

In fact, the only problem I can see here is the way we handle anonymous structs at the moment

we could also store the type under the typedef name for these cases, which would resolve it completely.

I'd like to avoid storing it under the typedef name, if possible, just to keep things similar to btf. Maybe we can internally refer to the type by (btf type id, btf source) tuples or even by a struct btf_type *?

[viktormalik]

I'd like to avoid storing it under the typedef name, if possible, just to keep things similar to btf. Maybe we can internally refer to the type by (btf type id, btf source) tuples or even by a struct btf_type *?

So we have StructManager that keeps track of all types. It currently has a map which maps struct names to their definitions. I don't think that we can store the types under their BTF id as we don't always use BTF (and, e.g., for userspace probes, we never will).

On the other hand, when using BTF, types can be resolved in FieldAnalyser (this is in #2315) and assigned directly to the expr they belong to. They still have to be stored inside the struct manager, but we could probably store anonymous types in a list instead of map (just to have some storage for them).