> 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. Indeed. But for the specific case case of preserve_access_info and vmlinux.h, having the attribute applied to all types (both directly referred and indirectly dependent) is actually what is required, isn't it? That is what the current implicity push-attribute clang pragma does. I have sent a tentative patch that adds the `record_attrs_str' configuration parameter to the btf_dump_opts, incorporating a few changes after Eduard's suggestions regarding avoiding double negations and docstrings. >> 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? I am not familiar with the particular use cases, but generally speaking separating sorting and emission makes sense to me. I would also prefer that to iterators. > > >> >> [...]
