Re: [RFC v2 2/3] bpftool: add attribute preserve_static_offset for context types

[Quentin Monnet <quentin@isovalent.com>]

2023-12-12 02:32 UTC+0000 ~ Eduard Zingerman <eddyz87@gmail.com>
> When printing vmlinux.h emit attribute preserve_static_offset [0] for
> types that are used as context parameters for BPF programs. To avoid
> hacking libbpf dump logic emit forward declarations annotated with
> attribute. Such forward declarations have to come before structure
> definitions.
> 
> Only emit such forward declarations when context types are present in
> target BTF (identified by name).
> 
> C language standard wording in section "6.7.2.1 Structure and union
> specifiers" [1] is vague, but example in 6.7.2.1.21 explicitly allows
> such notation, and it matches clang behavior.
> 
> Here is how 'bpftool btf gen ... format c' looks after this change:
> 
>     #ifndef __VMLINUX_H__
>     #define __VMLINUX_H__
> 
>     #if !defined(BPF_NO_PRESERVE_STATIC_OFFSET) && \
>         __has_attribute(preserve_static_offset)
>     #pragma clang attribute push \
>               (__attribute__((preserve_static_offset)), apply_to = record)
> 
>     struct bpf_cgroup_dev_ctx;
>     ...
> 
>     #pragma clang attribute pop
>     #endif
> 
>     ... rest of the output unchanged ...
> 
> This is a follow up for discussion in thread [2].
> 
> [0] https://clang.llvm.org/docs/AttributeReference.html#preserve-static-offset
> [1] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3088.pdf
> [2] https://lore.kernel.org/bpf/20231208000531.19179-1-eddyz87@gmail.com/
> 
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  tools/bpf/bpftool/btf.c | 131 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 116 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> index 91fcb75babe3..2abe71194afb 100644
> --- a/tools/bpf/bpftool/btf.c
> +++ b/tools/bpf/bpftool/btf.c
> @@ -460,11 +460,118 @@ static void __printf(2, 0) btf_dump_printf(void *ctx,
>  	vfprintf(stdout, fmt, args);
>  }
>  
> +static const char * const context_types[] = {
> +	"bpf_cgroup_dev_ctx",
> +	"bpf_nf_ctx",
> +	"bpf_perf_event_data",
> +	"bpf_raw_tracepoint_args",
> +	"bpf_sk_lookup",
> +	"bpf_sock",
> +	"bpf_sock_addr",
> +	"bpf_sock_ops",
> +	"bpf_sockopt",
> +	"bpf_sysctl",
> +	"__sk_buff",
> +	"sk_msg_md",
> +	"sk_reuseport_md",
> +	"xdp_md",
> +	"pt_regs",
> +};

Hi, and thanks for this!

Apologies for missing the discussion on v1. Reading through the previous
thread I see that they were votes in favour of the hard-coded approach,
but I would ask folks to please reconsider.

I'm not keen on taking this list in bpftool, it doesn't seem to be the
right place for that. I understand there's no plan to add new mirror
context structs, but if we change policy for whatever reason, we're sure
to miss the update in this list and that's a bug in bpftool. If bpftool
ever gets ported to Windows and Windows needs support for new structs,
that's more juggling to do to support multiple platforms. And if any
tool other than bpftool needs to generate vmlinux.h headers in the
future, it's back to square one - although by then there might be extra
pushback for changing the BTF, if bpftool already does the work.

Like Alan, I rather share your own inclination towards the more generic
declaration tags approach, if you don't mind the additional work.

Quentin