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. Could you please add a doc-string somewhere saying that ".record_attrs_str" applies to 'struct' and 'union'? Spent some time reading clang to verify that this is the case for 'applies_to=record' and it is. (build/tools/clang/include/clang/Sema/AttrParsedAttrImpl.inc, function checkAttributeMatchRuleAppliesTo(), case for attr::SubjectMatchRule_record). [...]
