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

[Quentin Monnet <quentin@isovalent.com>]

2023-12-12 15:58 UTC+0000 ~ Eduard Zingerman <eddyz87@gmail.com>
> On Tue, 2023-12-12 at 11:39 +0000, Quentin Monnet wrote:
> [...]
>> 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.
> 
> Understood, thank you for feedback.
> The second option is to:
> 
> 1. Define __bpf_ctx macro in linux headers as follows:
> 
>     #if __has_attribute(preserve_static_offset) && defined(__bpf__)
>     #define __bpf_ctx __attribute__((preserve_static_offset)) \
>                       __attribute__((btf_decl_tag(preserve_static_offset)))
>     #else
>     #define __bpf_ctx
>     #endif
> 
> 2a. Update libbpf to emit __attribute__((preserve_static_offset)) when
>     corresponding decl tag is present in the BTF.
> 
> 2b. Update bpftool to emit __attribute__((preserve_static_offset)) for
>     types with corresponding decl tag. (Like in this patch but check
>     for decl tag instead of name).

I don't have a strong opinion on that part, so...

> I think that 2b is better, because emitting
> BPF_NO_PRESERVE_STATIC_OFFSET from the same place where
> BPF_NO_PRESERVE_ACCESS_INDEX makes more sense,
> libbpf does not emit any macro definitions at the moment.

... the above makes sense, I'd say let's go for this if nobody else
objects (or wants it in libbpf instead - but bpftool is fine as far as
I'm concerned).

Thanks,
Quentin