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

[Eduard Zingerman <eddyz87@gmail.com>]

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).

[...]