On 12/15/18 1:49 PM, Martin Lau wrote:
> On Fri, Dec 14, 2018 at 03:34:34PM -0800, Yonghong Song wrote:
>> The following example shows map pretty print with structures
>> which include bitfield members.
>>
>> enum A { A1, A2, A3, A4, A5 };
>> typedef enum A ___A;
>> struct tmp_t {
>> char a1:4;
>> int a2:4;
>> int :4;
>> __u32 a3:4;
>> int b;
>> ___A b1:4;
>> enum A b2:4;
>> };
>> struct bpf_map_def SEC("maps") tmpmap = {
>> .type = BPF_MAP_TYPE_ARRAY,
>> .key_size = sizeof(__u32),
>> .value_size = sizeof(struct tmp_t),
>> .max_entries = 1,
>> };
>> BPF_ANNOTATE_KV_PAIR(tmpmap, int, struct tmp_t);
>>
>> and the following map update in the bpf program:
>>
>> key = 0;
>> struct tmp_t t = {};
>> t.a1 = 2;
>> t.a2 = 4;
>> t.a3 = 6;
>> t.b = 7;
>> t.b1 = 8;
>> t.b2 = 10;
>> bpf_map_update_elem(&tmpmap, &key, &t, 0);
>>
>> With this patch, I am able to print out the map values
>> correctly with this patch:
>> bpftool map dump id 187
>> [{
>> "key": 0,
>> "value": {
>> "a1": 0x2,
>> "a2": 0x4,
>> "a3": 0x6,
>> "b": 7,
>> "b1": 0x8,
>> "b2": 0xa
>> }
>> }
>> ]
>>
>> The following example shows forward type can be properly
>> printed in function prototype with modified tst_btf_haskv.c.
>>
>> struct t;
>> union u;
>>
>> __attribute__((noinline))
>> static int test_long_fname_1(struct dummy_tracepoint_args *arg,
>> struct t *p1, union u *p2,
>> __u32 unused)
>> ...
>> int _dummy_tracepoint(struct dummy_tracepoint_args *arg) {
>> return test_long_fname_1(arg, 0, 0);
>> }
>>
>> $ bpftool p d xlated id 24
>> ...
>> int test_long_fname_1(struct dummy_tracepoint_args * arg,
>> struct t * p1, union u * p2,
>> __u32 unused)
>> ...
>>
>> Signed-off-by: Yonghong Song <y...@fb.com>
>> ---
>> tools/bpf/bpftool/btf_dumper.c | 36 +++++++++++++++++++++++++---------
>> 1 file changed, 27 insertions(+), 9 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c
>> index ec1da87c3d65..23fc6a84d82b 100644
>> --- a/tools/bpf/bpftool/btf_dumper.c
>> +++ b/tools/bpf/bpftool/btf_dumper.c
>> @@ -190,6 +190,7 @@ static int btf_dumper_struct(const struct btf_dumper *d,
>> __u32 type_id,
>> const struct btf_type *t;
>> struct btf_member *m;
>> const void *data_off;
>> + int kind_flag;
>> int ret = 0;
>> int i, vlen;
>>
>> @@ -197,18 +198,32 @@ static int btf_dumper_struct(const struct btf_dumper
>> *d, __u32 type_id,
>> if (!t)
>> return -EINVAL;
>>
>> + kind_flag = BTF_INFO_KFLAG(t->info);
>> vlen = BTF_INFO_VLEN(t->info);
>> jsonw_start_object(d->jw);
>> m = (struct btf_member *)(t + 1);
>>
>> for (i = 0; i < vlen; i++) {
>> - data_off = data + BITS_ROUNDDOWN_BYTES(m[i].offset);
>
>
>> + __u32 bit_offset = m[i].offset;
> Without always checking the kind_flag first, here is an example
> that accessing the .offset looks incorrect at the first glance.
>
>> + __u32 bitfield_size = 0;
>> +
>> + if (kind_flag) {
> but then I noticed it is fine here ;)
>
>> + bitfield_size = BTF_MEMBER_BITFIELD_SIZE(bit_offset);
>> + bit_offset = BTF_MEMBER_BIT_OFFSET(bit_offset);
>> + }
>> +
>> jsonw_name(d->jw, btf__name_by_offset(d->btf, m[i].name_off));
>> - ret = btf_dumper_do_type(d, m[i].type,
>> - BITS_PER_BYTE_MASKED(m[i].offset),
>> - data_off);
>> - if (ret)
>> - break;
>> + if (bitfield_size) {
>> + btf_dumper_bitfield(bitfield_size, bit_offset,
>> + data, d->jw, d->is_plain_text);
>> + } else {
>> + data_off = data + BITS_ROUNDDOWN_BYTES(bit_offset);
>> + ret = btf_dumper_do_type(d, m[i].type,
>> +
>> BITS_PER_BYTE_MASKED(bit_offset),
>> + data_off);
>> + if (ret)
>> + break;
>> + }
>> }
>>
>> jsonw_end_object(d->jw);
>> @@ -295,6 +310,7 @@ static int __btf_dumper_type_only(const struct btf *btf,
>> __u32 type_id,
>>
>> switch (BTF_INFO_KIND(t->info)) {
>> case BTF_KIND_INT:
>> + case BTF_KIND_TYPEDEF:
>> BTF_PRINT_ARG("%s ", btf__name_by_offset(btf, t->name_off));
>> break;
>> case BTF_KIND_STRUCT:
>> @@ -318,10 +334,11 @@ static int __btf_dumper_type_only(const struct btf
>> *btf, __u32 type_id,
>> BTF_PRINT_TYPE(t->type);
>> BTF_PRINT_ARG("* ");
>> break;
>> - case BTF_KIND_UNKN:
>> case BTF_KIND_FWD:
>> - case BTF_KIND_TYPEDEF:
> hmm.... Is this TYPEDEF change related to this patch?
> If not, a comment in the commit message would help.
TYPEDEF is not related to kind_flag.
I will add a comment in commit message.
>
> Other than that,
>
> Acked-by: Martin KaFai Lau <ka...@fb.com>
>
>> - return -1;
>> + BTF_PRINT_ARG("%s %s ",
>> + BTF_INFO_KFLAG(t->info) ? "union" : "struct",
>> + btf__name_by_offset(btf, t->name_off));
>> + break;
>> case BTF_KIND_VOLATILE:
>> BTF_PRINT_ARG("volatile ");
>> BTF_PRINT_TYPE(t->type);
>> @@ -345,6 +362,7 @@ static int __btf_dumper_type_only(const struct btf *btf,
>> __u32 type_id,
>> if (pos == -1)
>> return -1;
>> break;
>> + case BTF_KIND_UNKN:
>> default:
>> return -1;
>> }
>> --
>> 2.17.1
>>
