On Thu, Jan 10, 2019 at 5:06 AM Arnaldo Carvalho de Melo
<acme@xxxxxxxxxx> wrote:
>
> Em Mon, Dec 31, 2018 at 06:03:27AM +0000, Andrii Nakryiko escreveu:
> > Union, when loaded, were created as 'struct type', while structs were
> > created as 'struct class', that contained 'struct type' within it. When
> > processing, though, unions and structs were treated the same with
> > assumption that they both are represented by 'struct class'. This patch
> > fixes issue by making sure unions and structs are created the same,
> > modulo DW_TAG_structure_type vs DW_TAG_union_type tag.
> >
> > Without fix:
> >
> > andriin@devvm$ ~/local/pahole/build/pahole -F dwarf -C fpregs_state ~/local/linux/net/ipv4/tcp.o
> > union fpregs_state {
> > struct fxregs_state fxsave; /* 0 512 */
> > struct swregs_state soft; /* 0 136 */
> > struct xregs_state xsave; /* 0 576 */
> > u8 __padding[4096]; /* 0 4096 */
> > };
> > andriin@devvm$ ~/local/pahole/build/pahole -F btf -C fpregs_state ~/local/linux/net/ipv4/tcp.o
> > Segmentation fault (core dumped)
>
> I couldn't reproduce this here:
>
> [acme@quaco pahole]$ pahole -J ~/git/build/v4.20-rc5/net/ipv4/tcp.o
> [acme@quaco pahole]$ pahole -F btf -C fpregs_state ~/git/build/v4.20-rc5/net/ipv4/tcp.o
> union fpregs_state {
> struct fregs_state fsave; /* 0 112 */
> struct fxregs_state fxsave; /* 0 512 */
> struct swregs_state soft; /* 0 136 */
> struct xregs_state xsave; /* 0 576 */
> u8 __padding[4096]; /* 0 4096 */
> };
> [acme@quaco pahole]$
>
> Can you try with what in my current tree? I think these address it:
Yes, you are right, your changes fixed the issue for both BTF and DWARF.
Please disregard the other patch I've sent for dwarf_encoder as well
([PATCH pahole] dwarf_loader: Represent union in same way as struct).
>
> e066ed0431b6 (HEAD -> master, acme.korg/master) pahole: Fix logic inversion wrt finding holes in structs
> 6ebb50f47b19 pahole: Do not apply 'struct class' filters to 'struct type'
> c9afdf88411d dwarves: Check if the tag is a 'struct class' in class__find_holes()
>
> And I merged 6ebb50f47b19 with e066ed0431b6 to preserve bisectability.
>
> So update the branch and this is the fix:
>
> commit 2c4569a952e0910a4a19d87d636f0887f5f7ed4d
> Author: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> Date: Fri Dec 21 14:20:58 2018 -0300
>
> pahole: Do not apply 'struct class' filters to 'struct type'
>
> Now unions are handled as well in pahole, so bail out before checking
> 'struct class' members for struct specific filters when handling
> non-structs in class__filter()
>
> Fixes: 31664d60ad41 ("pahole: Show tagged enums as well when no class is specified")
> Signed-off-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
>
>
> - Arnaldo
>
> >
> > With fix:
> >
> > andriin@devvm$ ~/local/pahole/build/pahole -F dwarf -C fpregs_state ~/local/linux/net/ipv4/tcp.o
> > union fpregs_state {
> > struct fxregs_state fxsave; /* 0 512 */
> > struct swregs_state soft; /* 0 136 */
> > struct xregs_state xsave; /* 0 576 */
> > u8 __padding[4096]; /* 0 4096 */
> > };
> > andriin@devvm$ ~/local/pahole/build/pahole -F btf -C fpregs_state ~/local/linux/net/ipv4/tcp.o
> > union fpregs_state {
> > struct fregs_state fsave; /* 0 112 */
> > struct fxregs_state fxsave; /* 0 512 */
> > struct swregs_state soft; /* 0 136 */
> > struct xregs_state xsave; /* 0 576 */
> > u8 __padding[4096]; /* 0 4096 */
> > };
> >
> > Note that there is a difference between DWARF and BTF loader outputs:
> > DWARF one seems to always skip first member of union type. It's not the case
> > for structs, though. This issue will be investigated and fixed separately.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@xxxxxx>
> > ---
> > btf_loader.c | 39 ++++++++++++---------------------------
> > 1 file changed, 12 insertions(+), 27 deletions(-)
> >
> > diff --git a/btf_loader.c b/btf_loader.c
> > index dae21d5..f04b12a 100644
> > --- a/btf_loader.c
> > +++ b/btf_loader.c
> > @@ -1,4 +1,4 @@
> > -/*
> > +/*
> > * btf_loader.c
> > *
> > * Copyright (C) 2018 Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> > @@ -121,12 +121,12 @@ static struct type *type__new(uint16_t tag, strings_t name, size_t size)
> > return type;
> > }
> >
> > -static struct class *class__new(strings_t name, size_t size)
> > +static struct class *class__new(strings_t name, size_t size, uint16_t tag)
> > {
> > struct class *class = tag__alloc(sizeof(*class));
> >
> > if (class != NULL) {
> > - type__init(&class->type, DW_TAG_structure_type, name, size);
> > + type__init(&class->type, tag, name, size);
> > INIT_LIST_HEAD(&class->vtable);
> > }
> >
> > @@ -205,7 +205,10 @@ static int create_members(struct btf *btf, void *ptr, int vlen, struct type *cla
> > static int create_new_class(struct btf *btf, void *ptr, int vlen, struct btf_type *tp, uint64_t size, long id)
> > {
> > strings_t name = btf__get32(btf, &tp->name_off);
> > - struct class *class = class__new(name, size);
> > + uint16_t type_tag = BTF_INFO_KIND(tp->info) == BTF_KIND_STRUCT
> > + ? DW_TAG_structure_type
> > + : DW_TAG_union_type;
> > + struct class *class = class__new(name, size, type_tag);
> > int member_size = create_members(btf, ptr, vlen, &class->type);
> >
> > if (member_size < 0)
> > @@ -219,25 +222,6 @@ out_free:
> > return -ENOMEM;
> > }
> >
> > -static int create_new_union(struct btf *btf, void *ptr,
> > - int vlen, struct btf_type *tp,
> > - uint64_t size, long id)
> > -{
> > - strings_t name = btf__get32(btf, &tp->name_off);
> > - struct type *un = type__new(DW_TAG_union_type, name, size);
> > - int member_size = create_members(btf, ptr, vlen, un);
> > -
> > - if (member_size < 0)
> > - goto out_free;
> > -
> > - cu__add_tag(btf->priv, &un->namespace.tag, &id);
> > -
> > - return (vlen * member_size);
> > -out_free:
> > - type__delete(un, btf->priv);
> > - return -ENOMEM;
> > -}
> > -
> > static struct enumerator *enumerator__new(strings_t name, uint32_t value)
> > {
> > struct enumerator *en = tag__alloc(sizeof(*en));
> > @@ -302,7 +286,10 @@ static int create_new_subroutine_type(struct btf *btf, void *ptr,
> > static int create_new_forward_decl(struct btf *btf, struct btf_type *tp, uint64_t size, long id)
> > {
> > strings_t name = btf__get32(btf, &tp->name_off);
> > - struct class *fwd = class__new(name, size);
> > + uint16_t type_tag = BTF_INFO_KIND(tp->info)
> > + ? DW_TAG_union_type
> > + : DW_TAG_structure_type;
> > + struct class *fwd = class__new(name, size, type_tag);
> >
> > if (fwd == NULL)
> > return -ENOMEM;
> > @@ -384,10 +371,8 @@ static int btf__load_types(struct btf *btf)
> > vlen = create_new_base_type(btf, ptr, type_ptr, type_index);
> > } else if (type == BTF_KIND_ARRAY) {
> > vlen = create_new_array(btf, ptr, type_index);
> > - } else if (type == BTF_KIND_STRUCT) {
> > + } else if (type == BTF_KIND_STRUCT || type == BTF_KIND_UNION) {
> > vlen = create_new_class(btf, ptr, vlen, type_ptr, size, type_index);
> > - } else if (type == BTF_KIND_UNION) {
> > - vlen = create_new_union(btf, ptr, vlen, type_ptr, size, type_index);
> > } else if (type == BTF_KIND_ENUM) {
> > vlen = create_new_enumeration(btf, ptr, vlen, type_ptr, size, type_index);
> > } else if (type == BTF_KIND_FWD) {
> > --
> > 2.17.1
>
> --
>
> - Arnaldo
