| Message ID | 20181214233434.2668076-1-yhs@fb.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: btf: fix struct/union/fwd types with kind_flag | expand |
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 <yhs@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 <kafai@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 >
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 <yhs@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 <kafai@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 >>
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; + __u32 bitfield_size = 0; + + if (kind_flag) { + 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: - 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; }
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 <yhs@fb.com> --- tools/bpf/bpftool/btf_dumper.c | 36 +++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-)