On Mon, Feb 25, 2019 at 5:00 PM Yonghong Song <yhs@xxxxxx> wrote:
>
>
>
> On 2/25/19 4:08 PM, Andrii Nakryiko wrote:
> > 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.
> >
> > 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
> >
> > Signed-off-by: Andrii Nakryiko <andriin@xxxxxx>
>
> Thanks for the fix.
> Maybe add:
> Fixes: b18354f64cc2 ("btf: Generate correct struct bitfield member types")
>
> In commit message, add what Commit b18354f64cc2 tries to do
> and why it is not needed any more.
> It tries to generate correct struct bitfield member type if the
> member is an enum. This is dated before kind_flag implementation.
> later kind_flag supported is added and 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" can make handling packed enum correctly too.
>
> With this change,
> Acked-by: Yonghong Song <yhs@xxxxxx>
Will do! Thanks for providing context. I was guessing this must have
been done before BTF had good support for bitfields.
>
> > ---
> > 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;
> >
