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@xxxxxxxxx/ 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? [...]
