On Fri, May 3, 2024 at 2:18 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > 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. And that's very unfortunate, which makes this not a good option, IMO. > > 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? I think if we are talking about the current API, then extending it with some pre/post type callback would solve this specific problem (but even then it feels dirty, because of "this callback is called after } but before ," sadness). I really dislike callbacks as part of public APIs like this. It feels like the user has to have control and the library should provide building blocks. So how about an alternative view on this problem. What if we add an API that will sort types in "C type system" order, i.e., it will return a sequence of BTF type ID + a flag whether it's a full BTF type definition or forward declaration only for that type. And then, separately, instead of btf_dump__dump_type() API that emits type *and* all its dependencies (unless they were already emitted, it's very stateful), we'll have an analogous API that will emit a full definition of one isolated btf_type (and no dependencies). The user will need to add semicolons after each type (and empty lines and stuff like that, probably), but they will also have control over appending/prepending any extra attributes and whatnot (or #ifdef guards). Also, when we have this API, we'll have all the necessary building blocks to finally be able to emit only types for module BTF without duplicated types from vmlinux.h (under assumption that vmlinux.h will be included before that). Libbpf will return fully sorted type order, including vmlinux BTF types, but bpftool (or whoever is using this API) will be smart in ignoring those types and/or emitting just forward declaration for them. With the decomposition into sort + emit string representation, it's now trivial to use in this flexible way. Thoughts? > > [...]
