Re: [RFC bpf-next] bpf: avoid clang-specific push/pop attribute pragmas in bpftool

[Eduard Zingerman <eddyz87@gmail.com>]

On Fri, 2024-05-03 at 13:36 -0700, Eduard Zingerman wrote:
> On Fri, 2024-05-03 at 13:18 +0200, Jose E. Marchesi wrote:
> [...]
> 
> > This patch modifies bpftool in order to, instead of using the pragmas,
> > define ATTR_PRESERVE_ACCESS_INDEX to conditionally expand to the CO-RE
> > attribute:
> > 
> >   #ifndef __VMLINUX_H__
> >   #define __VMLINUX_H__
> > 
> >   #ifndef BPF_NO_PRESERVE_ACCESS_INDEX
> >   #define ATTR_PRESERVE_ACCESS_INDEX __attribute__((preserve_access_index))
> >   #else
> >   #define ATTR_PRESERVE_ACCESS_INDEX
> >   #endif
> 
> Nit: maybe swap the branches to avoid double negation?
> 
> > 
> >   [... type definitions generated from kernel BTF ... ]
> > 
> >   #undef ATTR_PRESERVE_ACCESS_INDEX
> > 
> > and then the new btf_dump__dump_type_with_opts is used with options
> > specifying that we wish to have struct type attributes:
> > 
> >   DECLARE_LIBBPF_OPTS(btf_dump_type_opts, opts);
> >   [...]
> >   opts.record_attrs_str = "ATTR_PRESERVE_ACCESS_INDEX";
> >   [...]
> >   err = btf_dump__dump_type_with_opts(d, root_type_ids[i], &opts);
> > 
> > This is a RFC because introducing a new libbpf public function
> > btf_dump__dump_type_with_opts may not be desirable.
> > 
> > An alternative could be to, instead of passing the record_attrs_str
> > option in a btf_dump_type_opts, pass it in the global dumper's option
> > btf_dump_opts:
> > 
> >   DECLARE_LIBBPF_OPTS(btf_dump_opts, opts);
> >   [...]
> >   opts.record_attrs_str = "ATTR_PRESERVE_ACCESS_INDEX";
> >   [...]
> >   d = btf_dump__new(btf, btf_dump_printf, NULL, &opts);
> >   [...]
> >   err = btf_dump__dump_type(d, root_type_ids[i]);
> > 
> > This would be less disruptive regarding library API, and an overall
> > simpler change.  But it would prevent to use the same btf dumper to
> > dump types with and without attribute definitions.  Not sure if that
> > matters much in practice.
> > 
> > Thoughts?
> 
> I think that generating attributes explicitly is fine.
> 
> I also think that moving '.record_attrs_str' to 'btf_dump_opts' is preferable,
> in order to avoid adding new API functions.

On more argument for making it a part of btf_dump_opts is that
btf_dump__dump_type() walks the chain of dependent types,
so attribute placement control is not per-type anyways.

I also remembered my stalled attempt to emit preserve_static_offset
attribute for certain types [1] (need to finish with it).
There I needed to attach attributes to a dozen specific types.

[1] https://lore.kernel.org/bpf/20231220133411.22978-3-eddyz87@gmail.com/

So, I think that it would be better if '.record_attrs_str' would be a
callback accepting the name of the type and it's kind. Wdyt?

[...]