[PATCH dwarves] btf_loader: handle union forward declaration properly

[PATCH dwarves] btf_loader: handle union forward declaration properly

[Andrii Nakryiko]

Differentiate between struct and union forwards. For BTF_KIND_FWD this is
determined by kflag. So teach btf_loader to use that bit to decide whether
forward is for union or struct.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
N.B. This patch is based on top of tmp.libbtf_encoder branch.

Also seems like non-forward declared union has a slightly different
representation from struct (class). Not sure why it is so, but this change
doesn't seem to break anything.
---

 btf_loader.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/btf_loader.c b/btf_loader.c
index 9b5da3a4997a..0cb23967fec3 100644
--- a/btf_loader.c
+++ b/btf_loader.c
@@ -134,12 +134,13 @@ 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, bool is_union)
 {
 	struct class *class = tag__alloc(sizeof(*class));
+	uint32_t tag = is_union ? DW_TAG_union_type : DW_TAG_structure_type;
 
 	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);
 	}
 
@@ -228,7 +229,7 @@ static int create_members(struct btf_elf *btfe, const struct btf_type *tp,
 
 static int create_new_class(struct btf_elf *btfe, const struct btf_type *tp, uint32_t id)
 {
-	struct class *class = class__new(tp->name_off, tp->size);
+	struct class *class = class__new(tp->name_off, tp->size, false);
 	int member_size = create_members(btfe, tp, &class->type);
 
 	if (member_size < 0)
@@ -313,7 +314,7 @@ static int create_new_subroutine_type(struct btf_elf *btfe, const struct btf_typ
 
 static int create_new_forward_decl(struct btf_elf *btfe, const struct btf_type *tp, uint32_t id)
 {
-	struct class *fwd = class__new(tp->name_off, 0);
+	struct class *fwd = class__new(tp->name_off, 0, btf_kind(tp));
 
 	if (fwd == NULL)
 		return -ENOMEM;
-- 
2.24.1

Re: [PATCH dwarves] btf_loader: handle union forward declaration properly

[Andrii Nakryiko]

On Fri, Oct 9, 2020 at 12:26 PM Andrii Nakryiko <andrii@kernel.org> wrote:
>
> Differentiate between struct and union forwards. For BTF_KIND_FWD this is
> determined by kflag. So teach btf_loader to use that bit to decide whether
> forward is for union or struct.
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
> N.B. This patch is based on top of tmp.libbtf_encoder branch.
>
> Also seems like non-forward declared union has a slightly different
> representation from struct (class). Not sure why it is so, but this change
> doesn't seem to break anything.
> ---

Ping on this one, let's include it with upcoming pahole 1.19 as well?

>
>  btf_loader.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/btf_loader.c b/btf_loader.c
> index 9b5da3a4997a..0cb23967fec3 100644
> --- a/btf_loader.c
> +++ b/btf_loader.c
> @@ -134,12 +134,13 @@ 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, bool is_union)
>  {
>         struct class *class = tag__alloc(sizeof(*class));
> +       uint32_t tag = is_union ? DW_TAG_union_type : DW_TAG_structure_type;
>
>         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);
>         }
>
> @@ -228,7 +229,7 @@ static int create_members(struct btf_elf *btfe, const struct btf_type *tp,
>
>  static int create_new_class(struct btf_elf *btfe, const struct btf_type *tp, uint32_t id)
>  {
> -       struct class *class = class__new(tp->name_off, tp->size);
> +       struct class *class = class__new(tp->name_off, tp->size, false);
>         int member_size = create_members(btfe, tp, &class->type);
>
>         if (member_size < 0)
> @@ -313,7 +314,7 @@ static int create_new_subroutine_type(struct btf_elf *btfe, const struct btf_typ
>
>  static int create_new_forward_decl(struct btf_elf *btfe, const struct btf_type *tp, uint32_t id)
>  {
> -       struct class *fwd = class__new(tp->name_off, 0);
> +       struct class *fwd = class__new(tp->name_off, 0, btf_kind(tp));
>
>         if (fwd == NULL)
>                 return -ENOMEM;
> --
> 2.24.1
>

Re: [PATCH dwarves] btf_loader: handle union forward declaration properly

[Arnaldo Carvalho de Melo]

Em Wed, Oct 21, 2020 at 08:35:34AM -0700, Andrii Nakryiko escreveu:
> On Fri, Oct 9, 2020 at 12:26 PM Andrii Nakryiko <andrii@kernel.org> wrote:
> >
> > Differentiate between struct and union forwards. For BTF_KIND_FWD this is
> > determined by kflag. So teach btf_loader to use that bit to decide whether
> > forward is for union or struct.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> > N.B. This patch is based on top of tmp.libbtf_encoder branch.
> >
> > Also seems like non-forward declared union has a slightly different
> > representation from struct (class). Not sure why it is so, but this change
> > doesn't seem to break anything.
> > ---
> 
> Ping on this one, let's include it with upcoming pahole 1.19 as well?

I missed this one, but please consider providing concrete examples, can
you provide one so that its completely spelled out, like for a 4.9yo
kid? 8-)

- Arnaldo
 
> >
> >  btf_loader.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/btf_loader.c b/btf_loader.c
> > index 9b5da3a4997a..0cb23967fec3 100644
> > --- a/btf_loader.c
> > +++ b/btf_loader.c
> > @@ -134,12 +134,13 @@ 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, bool is_union)
> >  {
> >         struct class *class = tag__alloc(sizeof(*class));
> > +       uint32_t tag = is_union ? DW_TAG_union_type : DW_TAG_structure_type;
> >
> >         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);
> >         }
> >
> > @@ -228,7 +229,7 @@ static int create_members(struct btf_elf *btfe, const struct btf_type *tp,
> >
> >  static int create_new_class(struct btf_elf *btfe, const struct btf_type *tp, uint32_t id)
> >  {
> > -       struct class *class = class__new(tp->name_off, tp->size);
> > +       struct class *class = class__new(tp->name_off, tp->size, false);
> >         int member_size = create_members(btfe, tp, &class->type);
> >
> >         if (member_size < 0)
> > @@ -313,7 +314,7 @@ static int create_new_subroutine_type(struct btf_elf *btfe, const struct btf_typ
> >
> >  static int create_new_forward_decl(struct btf_elf *btfe, const struct btf_type *tp, uint32_t id)
> >  {
> > -       struct class *fwd = class__new(tp->name_off, 0);
> > +       struct class *fwd = class__new(tp->name_off, 0, btf_kind(tp));
> >
> >         if (fwd == NULL)
> >                 return -ENOMEM;
> > --
> > 2.24.1
> >

-- 

- Arnaldo

Re: [PATCH dwarves] btf_loader: handle union forward declaration properly

[Arnaldo Carvalho de Melo]

Em Fri, Oct 09, 2020 at 12:26:07PM -0700, Andrii Nakryiko escreveu:
> Differentiate between struct and union forwards. For BTF_KIND_FWD this is
> determined by kflag. So teach btf_loader to use that bit to decide whether
> forward is for union or struct.

So, before this patch 'btfdiff vmlinux' comes clean, i.e. pretty
printing from DWARF matches pretty printing from BTF, after it:

[acme@five pahole]$ btfdiff vmlinux  | wc -l
1500
[acme@five pahole]$

One of the differences:

@@ -117457,7 +117457,7 @@ struct wireless_dev {

 	/* XXX last struct has 1 byte of padding */

-	struct cfg80211_cqm_config * cqm_config;         /*   952     8 */
+	union cfg80211_cqm_config * cqm_config;          /*   952     8 */
 	/* --- cacheline 15 boundary (960 bytes) --- */
 	struct list_head           pmsr_list;            /*   960    16 */
 	spinlock_t                 pmsr_lock;            /*   976     4 */
[acme@five pahole]$

Looking at the source code:

struct wireless_dev {
...
        struct cfg80211_cqm_config *cqm_config;
...
}

Also:

 struct nfnl_ct_hook {
-	struct nf_conn *           (*get_ct)(const struct sk_buff  *, enum ip_conntrack_info *); /*     0     8 */
-	size_t                     (*build_size)(const struct nf_conn  *); /*     8     8 */
-	int                        (*build)(struct sk_buff *, struct nf_conn *, enum ip_conntrack_info, u_int16_t, u_int16_t); /*    16     8 */
-	int                        (*parse)(const struct nlattr  *, struct nf_conn *); /*    24     8 */
-	int                        (*attach_expect)(const struct nlattr  *, struct nf_conn *, u32, u32); /*    32     8 */
-	void                       (*seq_adjust)(struct sk_buff *, struct nf_conn *, enum ip_conntrack_info, s32); /*    40     8 */
+	union nf_conn *            (*get_ct)(const struct sk_buff  *, enum ip_conntrack_info *); /*     0     8 */
+	size_t                     (*build_size)(const union nf_conn  *); /*     8     8 */
+	int                        (*build)(struct sk_buff *, union nf_conn *, enum ip_conntrack_info, u_int16_t, u_int16_t); /*    16     8 */
+	int                        (*parse)(const struct nlattr  *, union nf_conn *); /*    24     8 */
+	int                        (*attach_expect)(const struct nlattr  *, union nf_conn *, u32, u32); /*    32     8 */
+	void                       (*seq_adjust)(struct sk_buff *, union nf_conn *, enum ip_conntrack_info, s32); /*    40     8 */1

Looking at the source code:

struct nfnl_ct_hook {
        struct nf_conn *(*get_ct)(const struct sk_buff *skb,
                                  enum ip_conntrack_info *ctinfo);
        size_t (*build_size)(const struct nf_conn *ct);
        int (*build)(struct sk_buff *skb, struct nf_conn *ct,
                     enum ip_conntrack_info ctinfo,
                     u_int16_t ct_attr, u_int16_t ct_info_attr);
        int (*parse)(const struct nlattr *attr, struct nf_conn *ct);
        int (*attach_expect)(const struct nlattr *attr, struct nf_conn *ct,
                             u32 portid, u32 report);
        void (*seq_adjust)(struct sk_buff *skb, struct nf_conn *ct,
                           enum ip_conntrack_info ctinfo, s32 off);
};

- Arnaldo
 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
> N.B. This patch is based on top of tmp.libbtf_encoder branch.
> 
> Also seems like non-forward declared union has a slightly different
> representation from struct (class). Not sure why it is so, but this change
> doesn't seem to break anything.
> ---
> 
>  btf_loader.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/btf_loader.c b/btf_loader.c
> index 9b5da3a4997a..0cb23967fec3 100644
> --- a/btf_loader.c
> +++ b/btf_loader.c
> @@ -134,12 +134,13 @@ 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, bool is_union)
>  {
>  	struct class *class = tag__alloc(sizeof(*class));
> +	uint32_t tag = is_union ? DW_TAG_union_type : DW_TAG_structure_type;
>  
>  	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);
>  	}
>  
> @@ -228,7 +229,7 @@ static int create_members(struct btf_elf *btfe, const struct btf_type *tp,
>  
>  static int create_new_class(struct btf_elf *btfe, const struct btf_type *tp, uint32_t id)
>  {
> -	struct class *class = class__new(tp->name_off, tp->size);
> +	struct class *class = class__new(tp->name_off, tp->size, false);
>  	int member_size = create_members(btfe, tp, &class->type);
>  
>  	if (member_size < 0)
> @@ -313,7 +314,7 @@ static int create_new_subroutine_type(struct btf_elf *btfe, const struct btf_typ
>  
>  static int create_new_forward_decl(struct btf_elf *btfe, const struct btf_type *tp, uint32_t id)
>  {
> -	struct class *fwd = class__new(tp->name_off, 0);
> +	struct class *fwd = class__new(tp->name_off, 0, btf_kind(tp));
>  
>  	if (fwd == NULL)
>  		return -ENOMEM;
> -- 
> 2.24.1
> 

-- 

- Arnaldo

Re: [PATCH dwarves] btf_loader: handle union forward declaration properly

[Andrii Nakryiko]

On Wed, Oct 21, 2020 at 12:25 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Fri, Oct 09, 2020 at 12:26:07PM -0700, Andrii Nakryiko escreveu:
> > Differentiate between struct and union forwards. For BTF_KIND_FWD this is
> > determined by kflag. So teach btf_loader to use that bit to decide whether
> > forward is for union or struct.
>
> So, before this patch 'btfdiff vmlinux' comes clean, i.e. pretty
> printing from DWARF matches pretty printing from BTF, after it:
>
> [acme@five pahole]$ btfdiff vmlinux  | wc -l
> 1500
> [acme@five pahole]$
>
> One of the differences:
>
> @@ -117457,7 +117457,7 @@ struct wireless_dev {
>
>         /* XXX last struct has 1 byte of padding */
>
> -       struct cfg80211_cqm_config * cqm_config;         /*   952     8 */
> +       union cfg80211_cqm_config * cqm_config;          /*   952     8 */
>         /* --- cacheline 15 boundary (960 bytes) --- */
>         struct list_head           pmsr_list;            /*   960    16 */
>         spinlock_t                 pmsr_lock;            /*   976     4 */
> [acme@five pahole]$
>
> Looking at the source code:
>
> struct wireless_dev {
> ...
>         struct cfg80211_cqm_config *cqm_config;
> ...
> }
>
> Also:
>
>  struct nfnl_ct_hook {
> -       struct nf_conn *           (*get_ct)(const struct sk_buff  *, enum ip_conntrack_info *); /*     0     8 */
> -       size_t                     (*build_size)(const struct nf_conn  *); /*     8     8 */
> -       int                        (*build)(struct sk_buff *, struct nf_conn *, enum ip_conntrack_info, u_int16_t, u_int16_t); /*    16     8 */
> -       int                        (*parse)(const struct nlattr  *, struct nf_conn *); /*    24     8 */
> -       int                        (*attach_expect)(const struct nlattr  *, struct nf_conn *, u32, u32); /*    32     8 */
> -       void                       (*seq_adjust)(struct sk_buff *, struct nf_conn *, enum ip_conntrack_info, s32); /*    40     8 */
> +       union nf_conn *            (*get_ct)(const struct sk_buff  *, enum ip_conntrack_info *); /*     0     8 */
> +       size_t                     (*build_size)(const union nf_conn  *); /*     8     8 */
> +       int                        (*build)(struct sk_buff *, union nf_conn *, enum ip_conntrack_info, u_int16_t, u_int16_t); /*    16     8 */
> +       int                        (*parse)(const struct nlattr  *, union nf_conn *); /*    24     8 */
> +       int                        (*attach_expect)(const struct nlattr  *, union nf_conn *, u32, u32); /*    32     8 */
> +       void                       (*seq_adjust)(struct sk_buff *, union nf_conn *, enum ip_conntrack_info, s32); /*    40     8 */1
>
> Looking at the source code:
>
> struct nfnl_ct_hook {
>         struct nf_conn *(*get_ct)(const struct sk_buff *skb,
>                                   enum ip_conntrack_info *ctinfo);
>         size_t (*build_size)(const struct nf_conn *ct);
>         int (*build)(struct sk_buff *skb, struct nf_conn *ct,
>                      enum ip_conntrack_info ctinfo,
>                      u_int16_t ct_attr, u_int16_t ct_info_attr);
>         int (*parse)(const struct nlattr *attr, struct nf_conn *ct);
>         int (*attach_expect)(const struct nlattr *attr, struct nf_conn *ct,
>                              u32 portid, u32 report);
>         void (*seq_adjust)(struct sk_buff *skb, struct nf_conn *ct,
>                            enum ip_conntrack_info ctinfo, s32 off);
> };
>
> - Arnaldo
>
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> > N.B. This patch is based on top of tmp.libbtf_encoder branch.
> >
> > Also seems like non-forward declared union has a slightly different
> > representation from struct (class). Not sure why it is so, but this change
> > doesn't seem to break anything.
> > ---
> >
> >  btf_loader.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/btf_loader.c b/btf_loader.c
> > index 9b5da3a4997a..0cb23967fec3 100644
> > --- a/btf_loader.c
> > +++ b/btf_loader.c
> > @@ -134,12 +134,13 @@ 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, bool is_union)
> >  {
> >       struct class *class = tag__alloc(sizeof(*class));
> > +     uint32_t tag = is_union ? DW_TAG_union_type : DW_TAG_structure_type;
> >
> >       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);
> >       }
> >
> > @@ -228,7 +229,7 @@ static int create_members(struct btf_elf *btfe, const struct btf_type *tp,
> >
> >  static int create_new_class(struct btf_elf *btfe, const struct btf_type *tp, uint32_t id)
> >  {
> > -     struct class *class = class__new(tp->name_off, tp->size);
> > +     struct class *class = class__new(tp->name_off, tp->size, false);
> >       int member_size = create_members(btfe, tp, &class->type);
> >
> >       if (member_size < 0)
> > @@ -313,7 +314,7 @@ static int create_new_subroutine_type(struct btf_elf *btfe, const struct btf_typ
> >
> >  static int create_new_forward_decl(struct btf_elf *btfe, const struct btf_type *tp, uint32_t id)
> >  {
> > -     struct class *fwd = class__new(tp->name_off, 0);
> > +     struct class *fwd = class__new(tp->name_off, 0, btf_kind(tp));

*FACEPALM*... This should be btf_kflag(tp) instead. I'll use btfdiff
on all my patches from now on, sorry about this!


> >
> >       if (fwd == NULL)
> >               return -ENOMEM;
> > --
> > 2.24.1
> >
>
> --
>
> - Arnaldo

Re: [PATCH dwarves] btf_loader: handle union forward declaration properly

[Arnaldo Carvalho de Melo]

Em Wed, Oct 21, 2020 at 12:47:30PM -0700, Andrii Nakryiko escreveu:
> On Wed, Oct 21, 2020 at 12:25 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > > @@ -313,7 +314,7 @@ static int create_new_subroutine_type(struct btf_elf *btfe, const struct btf_typ
> > >
> > >  static int create_new_forward_decl(struct btf_elf *btfe, const struct btf_type *tp, uint32_t id)
> > >  {
> > > -     struct class *fwd = class__new(tp->name_off, 0);
> > > +     struct class *fwd = class__new(tp->name_off, 0, btf_kind(tp));
 
> *FACEPALM*... This should be btf_kflag(tp) instead. I'll use btfdiff
> on all my patches from now on, sorry about this!

:-)

I'll retest when you resubmit.

One other thing I like to use is 'fullcircle':


<SNIP>
# See how your DW_AT_producer looks like and find the
# right regexp to get after the GCC version string, this one
# seems good enough for Red Hat/Fedora/CentOS that look like:
#
#   DW_AT_producer    : (indirect string, offset: 0x3583): GNU C89 8.2.1 20181215 (Red Hat 8.2.1-6) -mno-sse -mno-mmx
#
# So we need from -mno-sse onwards

CFLAGS=$(readelf -wi $file | grep -w DW_AT_producer | sed -r      's/.*\)( -[[:alnum:]]+.*)+/\1/g')

# Check if we managed to do the sed or if this is something like GNU AS
[ "${CFLAGS/DW_AT_producer/}" != "${CFLAGS}" ] && exit

${pfunct_bin} --compile $file > $c_output
gcc $CFLAGS -c -g $c_output -o $o_output
${codiff_bin} -q -s $file $o_output
<SNIP>


$ pfunct --help |& grep compile
      --compile[=FUNCTION]   Generate compilable source code with types
$

It generates code for all functions in a .o that touch its parameters
and right before those functions it regenerates all the types, so you go
from type information to C code that gets compiled with the same
compiler command line setting (obtained from DW_AT_producer for DWARF)
with type information and then it compares the original type information
with the one generated from the regenerated "original" source code.

Right now it works only with DWARF, because for it we derive the packed
attribute (pahole does that for BTF too) but, unlike bpftool btf it
doesn't add the needed padding for __alignment__ things, and DWARF
provides that info.

I'll work on that eventually and then fullcircle will do it for both
DWARF and for BTF.

- Arnaldo