[PATCH v2 pahole 0/2] fix two more DWARF/BTF discrepancies

[PATCH v2 pahole 0/2] fix two more DWARF/BTF discrepancies

[Andrii Nakryiko]

This patchset fixes two more issues where DWARF/BTF encoding/loading
produce different result:
  - patch #1 fixes incorrect handling of "unknown" base types;
  - patch #2 fixes incorrect handling of packed enums during encoding.

Andrii Nakryiko (2):
  btf_loader: simplify fixup code by relying on BTF data more
  btf_encoder: don't special case packed enums

 btf_encoder.c |  9 --------
 btf_loader.c  | 59 ++++++++++-----------------------------------------
 2 files changed, 11 insertions(+), 57 deletions(-)

-- 
2.17.1

[PATCH v2 pahole 1/2] btf_loader: simplify fixup code by relying on BTF data more

[Andrii Nakryiko]

btf_loader relies on guessing base integral type size for enums and
integers, which is unreliable. There doesn't seem to be a need for
that, as all this information could be extracted from BTF information.

Before:
$ PAHOLE=~/local/pahole/build/pahole ./btfdiff ~/local/btf/libc-2.28.so.debug
base_type__name_to_size: base_type _Float128
class__fixup_btf_bitfields: unknown base type name "_Float128"!
base_type__name_to_size: base_type _Float128
class__fixup_btf_bitfields: unknown base type name "_Float128"!
base_type__name_to_size: base_type _Float128
class__fixup_btf_bitfields: unknown base type name "_Float128"!
base_type__name_to_size: base_type _Float128
class__fixup_btf_bitfields: unknown base type name "_Float128"!
base_type__name_to_size: base_type _Float128
class__fixup_btf_bitfields: unknown base type name "_Float128"!
base_type__name_to_size: base_type _Float128
class__fixup_btf_bitfields: unknown base type name "_Float128"!
--- /tmp/btfdiff.dwarf.aV4wSL   2019-02-25 13:31:54.787923673 -0800
+++ /tmp/btfdiff.btf.22NQmJ     2019-02-25 13:31:54.802923668 -0800
@@ -461,9 +461,15 @@ struct La_x86_64_retval {
        uint64_t                   lrv_rdx;              /*     8     8 */
        La_x86_64_xmm              lrv_xmm0;             /*    16    16 */
        La_x86_64_xmm              lrv_xmm1;             /*    32    16 */
-       long double                lrv_st0;              /*    48    16 */
+       long double                lrv_st0;              /*    48     8 */
+
+       /* XXX 8 bytes hole, try to pack */
+
        /* --- cacheline 1 boundary (64 bytes) --- */
-       long double                lrv_st1;              /*    64    16 */
+       long double                lrv_st1;              /*    64     8 */
+
+       /* XXX 8 bytes hole, try to pack */
+
        La_x86_64_vector           lrv_vector0;          /*    80    64 */
        /* --- cacheline 2 boundary (128 bytes) was 16 bytes ago --- */
        La_x86_64_vector           lrv_vector1;          /*   144    64 */
@@ -472,6 +478,7 @@ struct La_x86_64_retval {
        __int128                   lrv_bnd1;             /*   224    16 */

        /* size: 240, cachelines: 4, members: 10 */
+       /* sum members: 224, holes: 2, sum holes: 16 */
        /* last cacheline: 48 bytes */
 };
 struct r_debug {
@@ -2044,7 +2051,7 @@ union ieee754_float {
        } ieee_nan;                                    /*     0     4 */
 };
 union ieee854_long_double {
-       long double                d;                  /*     0    16 */
+       long double                d;                  /*     0     8 */
        struct {
                unsigned int       mantissa1:32;       /*     0: 0  4 */
                unsigned int       mantissa0:32;       /*     4: 0  4 */
@@ -2141,7 +2148,7 @@ struct ucontext_t {
        /* last cacheline: 8 bytes */
 };
 union ieee854_float128 {
-       _Float128                  d;                  /*     0    16 */
+       _Float128                  d;                  /*     0     0 */
        struct {
                unsigned int       mantissa3:32;       /*     0: 0  4 */
                unsigned int       mantissa2:32;       /*     4: 0  4 */
@@ -2219,7 +2226,7 @@ union printf_arg {
        long unsigned int          pa_u_long_int;      /*     0     8 */
        long long unsigned int     pa_u_long_long_int; /*     0     8 */
        double                     pa_double;          /*     0     8 */
-       long double                pa_long_double;     /*     0    16 */
+       long double                pa_long_double;     /*     0     8 */
        const char  *              pa_string;          /*     0     8 */
        const wchar_t  *           pa_wstring;         /*     0     8 */
        void *                     pa_pointer;         /*     0     8 */

$ PAHOLE=~/local/pahole/build/pahole ./btfdiff ~/local/btf/libc-2.28.so.debug
<empty output>

Still good for kernel image:
$ PAHOLE=~/local/pahole/build/pahole ./btfdiff ~/local/btf/vmlinux4
<empty output>

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 btf_loader.c | 59 ++++++++++------------------------------------------
 1 file changed, 11 insertions(+), 48 deletions(-)

diff --git a/btf_loader.c b/btf_loader.c
index 62e7e30..3b4fb40 100644
--- a/btf_loader.c
+++ b/btf_loader.c
@@ -463,64 +463,27 @@ static int class__fixup_btf_bitfields(struct tag *tag, struct cu *cu, struct btf
 			continue;
 
 		pos->bitfield_offset = 0;
+		pos->byte_size = tag__size(type, cu);
+		pos->bit_size = pos->byte_size * 8;
 
-		uint16_t type_bit_size;
-		size_t integral_bit_size;
-
-		switch (type->tag) {
-		case DW_TAG_enumeration_type:
-			type_bit_size = tag__type(type)->size;
-			/* Best we can do to check if this is a packed enum */
-			if (is_power_of_2(type_bit_size))
-				integral_bit_size = roundup(type_bit_size, 8);
-			else
-				integral_bit_size = sizeof(int) * 8;
-			break;
-		case DW_TAG_base_type: {
-			struct base_type *bt = tag__base_type(type);
-			char name[256];
-			type_bit_size = bt->bit_size;
-			integral_bit_size = base_type__name_to_size(bt, cu);
-			if (integral_bit_size == 0) {
-				fprintf(stderr, "%s: unknown base type name \"%s\"!\n",
-					__func__, base_type__name(bt, cu, name,
-								  sizeof(name)));
-			}
-		}
-			break;
-		default:
-			pos->byte_size = tag__size(type, cu);
-			pos->bit_size = pos->byte_size * 8;
+		/* bitfield fixup is needed for enums and base types only */
+		if (type->tag != DW_TAG_base_type && type->tag != DW_TAG_enumeration_type)
 			continue;
-		}
 
-		/*
-		 * XXX: integral_bit_size can be zero if base_type__name_to_size doesn't
-		 * know about the base_type name, so one has to add there when
-		 * such base_type isn't found. pahole will put zero on the
-		 * struct output so it should be easy to spot the name when
-		 * such unlikely thing happens.
-		 */
-		pos->byte_size = integral_bit_size / 8;
-		pos->bit_size = type_bit_size;
-
-		if (integral_bit_size == 0) {
-			pos->bit_size = 0;
+		/* if BTF data is incorrect and has size == 0, skip field, 
+		 * instead of crashing */
+		if (pos->byte_size == 0) {
 			continue;
 		}
 
 		if (pos->bitfield_size) {
 			/* bitfields seem to be always aligned, no matter the packing */
-			pos->byte_offset = pos->bit_offset / integral_bit_size * integral_bit_size / 8;
-		} else {
-			pos->byte_offset = pos->bit_offset / 8;
-		}
-
-
-		if (pos->bitfield_size) {
+			pos->byte_offset = pos->bit_offset / pos->bit_size * pos->bit_size / 8;
 			pos->bitfield_offset = pos->bit_offset - pos->byte_offset * 8;
 			if (!btfe->is_big_endian)
-				pos->bitfield_offset = integral_bit_size - pos->bitfield_offset - pos->bitfield_size;
+				pos->bitfield_offset = pos->bit_size - pos->bitfield_offset - pos->bitfield_size;
+		} else {
+			pos->byte_offset = pos->bit_offset / 8;
 		}
 	}
 
-- 
2.17.1

[PATCH v2 pahole 2/2] btf_encoder: don't special case packed enums

[Andrii Nakryiko]

BTF data can represent packed enums correctly without any special
handling from pahole side. Previously pahole's own `enum vscope` would
be omitted causing problems.

Original commit tried to generate correct struct bitfield member type if
the member is an enum. This was dated before kind_flag implementation.
Later, kind_flag support was added and now pahole always generates BTF
with kind_flag = 1 for structures with bitfield, where bitfield size is
encoded in btf_member, so this workaround is not needed any more.
Removing this "hack" makes handling it easier to handle packed enums
correctly.

Repro:

$ cat test/packed_enum.c
enum packed_enum {
        VALUE1,
        VALUE2,
        VALUE3
} __attribute__((packed));

struct s {
        int x;
        enum packed_enum e;
        int y;
};

int main()
{
        struct s s;
        return 0;
}

$ gcc -g -c test/packed_enum.c -o test/packed_enum.o

$ ~/local/pahole/build/pahole -JV test/packed_enum.o
File test/packed_enum.o:
[1] INT (anon) size=1 bit_offset=0 nr_bits=8 encoding=SIGNED
[2] STRUCT s kind_flag=0 size=12 vlen=3
        x type_id=3 bits_offset=0
        e type_id=1 bits_offset=32
        y type_id=3 bits_offset=64
[3] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED

$ ~/local/pahole/build/pahole -F dwarf test/packed_enum.o
struct s {
        int                        x;                    /*     0     4 */
        enum packed_enum           e;                    /*     4     1 */

        /* XXX 3 bytes hole, try to pack */

        int                        y;                    /*     8     4 */

        /* size: 12, cachelines: 1, members: 3 */
        /* sum members: 9, holes: 1, sum holes: 3 */
        /* last cacheline: 12 bytes */
};

$ ~/local/pahole/build/pahole -F btf test/packed_enum.o
struct s {
        int                        x;                    /*     0     4 */
        nameless base type!        e;                    /*     4     1 */

        /* XXX 3 bytes hole, try to pack */

        int                        y;                    /*     8     4 */

        /* size: 12, cachelines: 1, members: 3 */
        /* sum members: 9, holes: 1, sum holes: 3 */
        /* last cacheline: 12 bytes */
};

Notice how pahole's log doesn't have a mention of encoding 'packed_enum'
(anonymous integer is generated instead), which causes 'nameless base
type!' output above.

Fix this change:

$ ~/local/pahole/build/pahole -JV test/packed_enum.o
File test/packed_enum.o:
[1] ENUM packed_enum size=1 vlen=3
        VALUE1 val=0
        VALUE2 val=1
        VALUE3 val=2
[2] STRUCT s kind_flag=0 size=12 vlen=3
        x type_id=3 bits_offset=0
        e type_id=1 bits_offset=32
        y type_id=3 bits_offset=64
[3] INT int size=4 bit_offset=0 nr_bits=32 encoding=SIGNED

$ ~/local/pahole/build/pahole -F btf test/packed_enum.o
struct s {
        int                        x;                    /*     0     4 */
        enum packed_enum           e;                    /*     4     1 */

        /* XXX 3 bytes hole, try to pack */

        int                        y;                    /*     8     4 */

        /* size: 12, cachelines: 1, members: 3 */
        /* sum members: 9, holes: 1, sum holes: 3 */
        /* last cacheline: 12 bytes */
};

$ PAHOLE=~/local/pahole/build/pahole ./btfdiff test/packed_enum.o

Also verified on pahole, kernel and glibc:

$ PAHOLE=~/local/pahole/build/pahole ./btfdiff ~/local/btf/pahole.debug
$ PAHOLE=~/local/pahole/build/pahole ./btfdiff ~/local/btf/libc-2.28.so.debug
$ PAHOLE=~/local/pahole/build/pahole ./btfdiff ~/local/btf/vmlinux4

Fixes: b18354f64cc2 ("btf: Generate correct struct bitfield member types")
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
Acked-by: Yonghong Song <yhs@fb.com>
---
 btf_encoder.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 9cf0831..d8dc368 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -116,15 +116,6 @@ static int32_t enumeration_type__encode(struct btf_elf *btfe, struct tag *tag)
 	struct enumerator *pos;
 	int32_t type_id;
 
-	/* if enumerator bit_size is not 32, generate an int type instead. */
-	if (etype->size != 32) {
-		struct base_type bt = {};
-
-		bt.bit_size = etype->size;
-		bt.is_signed = true;
-		return btf_elf__add_base_type(btfe, &bt);
-	}
-
 	type_id = btf_elf__add_enum(btfe, etype->namespace.name, etype->size, etype->nr_members);
 	if (type_id < 0)
 		return type_id;
-- 
2.17.1

Re: [PATCH v2 pahole 2/2] btf_encoder: don't special case packed enums

[Arnaldo Carvalho de Melo]

Em Mon, Feb 25, 2019 at 05:19:27PM -0800, Andrii Nakryiko escreveu:
> BTF data can represent packed enums correctly without any special
> handling from pahole side. Previously pahole's own `enum vscope` would
> be omitted causing problems.
> 
> Original commit tried to generate correct struct bitfield member type if
> the member is an enum. This was dated before kind_flag implementation.
> Later, kind_flag support was added and now pahole always generates BTF
> with kind_flag = 1 for structures with bitfield, where bitfield size is
> encoded in btf_member, so this workaround is not needed any more.
> Removing this "hack" makes handling it easier to handle packed enums
> correctly.

<SNIP excellent repro before/after text>
 
> Fixes: b18354f64cc2 ("btf: Generate correct struct bitfield member types")
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> Acked-by: Yonghong Song <yhs@fb.com>

Thanks a lot, tested it with vmlinux, libc, your repro, all working
jist fine, applied!

Now out to get a vmlinux for a different arch to check that as well.

- Arnaldo