Re: [PATCH bpf-next 05/12] bpftool: Add btf enum64 support

[Yonghong Song <yhs@fb.com>]

On 5/9/22 4:31 PM, Andrii Nakryiko wrote:
> On Sun, May 1, 2022 at 12:00 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> Add BTF_KIND_ENUM64 support.
>> For example, the following enum is defined in uapi bpf.h.
>>    $ cat core.c
>>    enum A {
>>          BPF_F_INDEX_MASK                = 0xffffffffULL,
>>          BPF_F_CURRENT_CPU               = BPF_F_INDEX_MASK,
>>          BPF_F_CTXLEN_MASK               = (0xfffffULL << 32),
>>    } g;
>> Compiled with
>>    clang -target bpf -O2 -g -c core.c
>> Using bpftool to dump types and generate format C file:
>>    $ bpftool btf dump file core.o
>>    ...
>>    [1] ENUM64 'A' size=8 vlen=3
>>          'BPF_F_INDEX_MASK' val=4294967295ULL
>>          'BPF_F_CURRENT_CPU' val=4294967295ULL
>>          'BPF_F_CTXLEN_MASK' val=4503595332403200ULL
>>    $ bpftool btf dump file core.o format c
>>    ...
>>    enum A {
>>          BPF_F_INDEX_MASK = 4294967295ULL,
>>          BPF_F_CURRENT_CPU = 4294967295ULL,
>>          BPF_F_CTXLEN_MASK = 4503595332403200ULL,
> 
> maybe we should have some heuristic that if the value is "big enough"
> (e.g., larger than 1-128 millions) and is unsigned we should emit it
> as hex?

I thought about this. But since you are also suggesting this, I will
do this (greater than 1 million).

> 
>>    };
>>    ...
>>
>> The 64bit value is represented properly in BTF and C dump.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
> 
> just minor nits, LGTM
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
>>   tools/bpf/bpftool/btf.c        | 47 ++++++++++++++++++++++++++++++++--
>>   tools/bpf/bpftool/btf_dumper.c | 32 +++++++++++++++++++++++
>>   tools/bpf/bpftool/gen.c        |  1 +
>>   3 files changed, 78 insertions(+), 2 deletions(-)
>>
> 
> [...]
>> +       case BTF_KIND_ENUM64: {
>> +               const struct btf_enum64 *v = (const void *)(t + 1);
> 
> can use btf_enum64() helper from libbpf?
> 
>> +               __u16 vlen = BTF_INFO_VLEN(t->info);
> 
> btf_vlen(t)

I copied the code from above BTF_KIND_ENUM. But I certainly can do this.

> 
>> +               int i;
>> +
>> +               if (json_output) {
>> +                       jsonw_uint_field(w, "size", t->size);
>> +                       jsonw_uint_field(w, "vlen", vlen);
>> +                       jsonw_name(w, "values");
>> +                       jsonw_start_array(w);
>> +               } else {
>> +                       printf(" size=%u vlen=%u", t->size, vlen);
>> +               }
>> +               for (i = 0; i < vlen; i++, v++) {
>> +                       const char *name = btf_str(btf, v->name_off);
>> +                       __u64 val = (__u64)v->hi32 << 32 | v->lo32;
> 
> () ?

okay.

> 
>> +
>> +                       if (json_output) {
>> +                               jsonw_start_object(w);
>> +                               jsonw_string_field(w, "name", name);
>> +                               if (btf_kflag(t))
>> +                                       jsonw_uint_field(w, "val", val);
>> +                               else
>> +                                       jsonw_int_field(w, "val", val);
>> +                               jsonw_end_object(w);
>> +                       } else {
>> +                               if (btf_kflag(t))
>> +                                       printf("\n\t'%s' val=%lluULL", name, val);
>> +                               else
>> +                                       printf("\n\t'%s' val=%lldLL", name, val);
>>                          }
>>                  }
>>                  if (json_output)
>> diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c
>> index f5dddf8ef404..f9f38384b9a6 100644
>> --- a/tools/bpf/bpftool/btf_dumper.c
>> +++ b/tools/bpf/bpftool/btf_dumper.c
>> @@ -182,6 +182,35 @@ static int btf_dumper_enum(const struct btf_dumper *d,
>>          return 0;
>>   }
>>
>> +static int btf_dumper_enum64(const struct btf_dumper *d,
>> +                            const struct btf_type *t,
>> +                            const void *data)
>> +{
>> +       const struct btf_enum64 *enums = btf_enum64(t);
>> +       __u32 hi32, lo32;
>> +       __u64 value;
>> +       __u16 i;
>> +
>> +       if (t->size != 8)
>> +               return -EINVAL;
> 
> no need

sure.

> 
>> +
>> +       value = *(__u64 *)data;
>> +       hi32 = value >> 32;
>> +       lo32 = (__u32)value;
>> +
> 
> [...]