| Message ID | 20201009192607.699835-1-andrii@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [dwarves] btf_loader: handle union forward declaration properly | expand |
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 >
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 > >
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 >
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
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
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;
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(-)