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

["Jose E. Marchesi" <jose.marchesi@oracle.com>]

> On Fri, May 3, 2024 at 2:18 PM Eduard Zingerman <eddyz87@gmail.com> 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@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?
>
> 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.

>
>
>>
>> [...]