Re: [PATCH bpf-next v3 1/3] libbpf: __attribute__((btf_decl_tag("..."))) for btf dump in C format

[Yonghong Song <yhs@meta.com>]

On 11/15/22 12:45 PM, Yonghong Song wrote:
> 
> 
> On 11/11/22 1:30 PM, Eduard Zingerman wrote:
>> On Fri, 2022-11-11 at 10:58 -0800, Andrii Nakryiko wrote:
>>> On Thu, Nov 10, 2022 at 6:43 AM Eduard Zingerman <eddyz87@gmail.com> 
>>> wrote:
>>>
>>>
>>> [...]
>>>
>>>>   static int btf_dump_push_decl_stack_id(struct btf_dump *d, __u32 id)
>>>> @@ -1438,9 +1593,12 @@ static void btf_dump_emit_type_chain(struct 
>>>> btf_dump *d,
>>>>                  }
>>>>                  case BTF_KIND_FUNC_PROTO: {
>>>>                          const struct btf_param *p = btf_params(t);
>>>> +                       struct decl_tag_array *decl_tags = NULL;
>>>>                          __u16 vlen = btf_vlen(t);
>>>>                          int i;
>>>>
>>>> +                       hashmap__find(d->decl_tags, id, &decl_tags);
>>>> +
>>>>                          /*
>>>>                           * GCC emits extra volatile qualifier for
>>>>                           * __attribute__((noreturn)) function 
>>>> pointers. Clang
>>>
>>> should there be btf_dump_emit_decl_tags(d, decl_tags, -1) somewhere
>>> here to emit tags of FUNC_PROTO itself?
>>
>> Actually, I have not found a way to attach decl tag to a FUNC_PROTO 
>> itself:
>>
>>    typedef void (*fn)(void) __decl_tag("..."); // here tag is attached 
>> to typedef
>>    struct foo {
>>      void (*fn)(void) __decl_tag("..."); // here tag is attached to a 
>> foo.fn field
>>    }
>>    void foo(void (*fn)(void) __decl_tag("...")); // here tag is 
>> attached to FUNC foo
>>                                                  // parameter but 
>> should probably
>>                                                  // be attached to
>>                                                  // FUNC_PROTO 
>> parameter instead.
>>
>> Also, I think that Yonghong had reservations about decl tags attached to
>> FUNC_PROTO parameters.
>> Yonghong, could you please comment?
> 
> Currently, btf decl tag is not supported to attach FUNC_PROTO 
> parameters. We could add support in clang, do we have an actual use case
> for this? if there is a use case, we can add support for it.
> 
>>
>>>
>>>> @@ -1481,6 +1639,7 @@ static void btf_dump_emit_type_chain(struct 
>>>> btf_dump *d,
>>>>
>>>>                                  name = btf_name_of(d, p->name_off);
>>>>                                  btf_dump_emit_type_decl(d, p->type, 
>>>> name, lvl);
>>>> +                               btf_dump_emit_decl_tags(d, 
>>>> decl_tags, i);
>>>>                          }
>>>>
>>>>                          btf_dump_printf(d, ")");
>>>> @@ -1896,6 +2055,7 @@ static int btf_dump_var_data(struct btf_dump *d,
>>>>                               const void *data)
>>>>   {
>>>>          enum btf_func_linkage linkage = btf_var(v)->linkage;
>>>> +       struct decl_tag_array *decl_tags = NULL;
>>>>          const struct btf_type *t;
>>>>          const char *l;
>>>>          __u32 type_id;
>>>> @@ -1920,7 +2080,10 @@ static int btf_dump_var_data(struct btf_dump *d,
>>>>          type_id = v->type;
>>>>          t = btf__type_by_id(d->btf, type_id);
>>>>          btf_dump_emit_type_cast(d, type_id, false);
>>>> -       btf_dump_printf(d, " %s = ", btf_name_of(d, v->name_off));
>>>> +       btf_dump_printf(d, " %s", btf_name_of(d, v->name_off));
>>>> +       hashmap__find(d->decl_tags, id, &decl_tags);
>>>> +       btf_dump_emit_decl_tags(d, decl_tags, -1);
>>>> +       btf_dump_printf(d, " = ");
>>>>          return btf_dump_dump_type_data(d, NULL, t, type_id, data, 
>>>> 0, 0);
>>>>   }
>>>>
>>>> @@ -2421,6 +2584,8 @@ int btf_dump__dump_type_data(struct btf_dump 
>>>> *d, __u32 id,
>>>>          d->typed_dump->skip_names = OPTS_GET(opts, skip_names, false);
>>>>          d->typed_dump->emit_zeroes = OPTS_GET(opts, emit_zeroes, 
>>>> false);
>>>>
>>>> +       btf_dump_assign_decl_tags(d);
>>>> +
>>>
>>> I'm actually not sure we want those tags on binary data dump.
>>> Generally data dump is not type definition dump, so this seems
>>> unnecessary, it will just distract from data itself. Let's drop it for
>>> now? If there would be a need we can add it easily later.
>>
>> Well, this is the only place where VARs are processed, removing this code
>> would make the second patch in a series useless.
>> But I like my second patch in a series :) should I just drop it?
>> I can extract it as a separate series and simplify some of the existing
>> data dump tests.

BTW, current use case of decl tag in the kernel is from Kumar's link 
list patch set:

https://lore.kernel.org/bpf/20221114191547.1694267-23-memxor@gmail.com/

+#define __contains(name, node) __attribute__((btf_decl_tag("contains:" 
#name ":" #node)))

to tag the struct member bpf_list_head as below:

struct foo {
	struct bpf_list_node node;
	int data;
};

struct map_value {
	struct bpf_list_head head __contains(foo, node);
};

https://lore.kernel.org/bpf/20221114191547.1694267-5-memxor@gmail.com/

>>
>>>
>>>>          ret = btf_dump_dump_type_data(d, NULL, t, id, data, 0, 0);
>>>>
>>>>          d->typed_dump = NULL;
>>>> -- 
>>>> 2.34.1
>>>>
>>