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