Em Thu, Mar 14, 2019 at 11:53:54AM -0700, Andrii Nakryiko escreveu:
> On Thu, Mar 14, 2019 at 10:45 AM Arnaldo Carvalho de Melo
> <arnaldo.melo@xxxxxxxxx> wrote:
> >
> > Em Wed, Mar 13, 2019 at 11:07:36PM -0700, Andrii Nakryiko escreveu:
> > > Bitfield offsets can be negative, if field "borrows" few bits from
> > > following aligned field. This causes a bunch of surprises down the line
> > > in pahole's logic (e.g., for hole calculation logic), so instead of
> > > waiting till printf routines adjust this for display, adjust them early
> > > and keep less surprising semantics.
> >
> > > +++ b/btf_loader.c
> > > @@ -482,6 +482,12 @@ static int class__fixup_btf_bitfields(struct tag *tag, struct cu *cu, struct btf
> > > pos->bitfield_offset = pos->bit_offset - pos->byte_offset * 8;
> > > if (!btfe->is_big_endian)
> > > pos->bitfield_offset = pos->bit_size - pos->bitfield_offset - pos->bitfield_size;
> > > + /* re-adjust bitfield offset if it is negative */
> > > + if (pos->bitfield_offset < 0) {
> > > + pos->bitfield_offset += pos->bit_size;
> > > + pos->byte_offset += pos->byte_size;
> > > + pos->bit_offset = pos->byte_offset * 8 + pos->bitfield_offset;
> > > + }
> > > } else {
> > > pos->byte_offset = pos->bit_offset / 8;
> > > }
> >
> > While testing this one I noticed this output:
> >
> > [acme@quaco pahole]$ pahole -F btf -C ieee80211_tx_rate examples/vmlinux-aarch64
> > struct ieee80211_tx_rate {
> > s8 idx; /* 0 1 */
> >
> > /* Bitfield combined with previous fields */
> >
> > u16 count:5; /* 0: 8 2 */
> > u16 flags:11; /* 0:13 2 */
>
> These offsets are wrong (pahole always prints them in big-endian
> notation, right?)
>
> It should be:
>
> u16 count:5; /* 0: 0 2 */
> u16 flags:11; /* 2: 8 2 */
>
> But now I'm super confused about pahole's and DWARF's assumption of
> big-endianness... (see below)
>
> >
> > /* size: 3, cachelines: 1, members: 3 */
> > /* padding: 1 */
> > /* last cacheline: 3 bytes */
> > };
> > [acme@quaco pahole]$
> >
> > The message is confusing, as count:5 is not being combined with previous
> > fields, i.e. the previous field has 8 bits and all those bits are being
> > used by 'idx'.
>
> I think technically count is combined with idx, see DWARF analysis below.
>
> We should probably check what assembly is generated to know for sure
> if it's combined into aligned first 2 bytes, or non-aligned 2 bytes at
> offset 1.
>
> >
> > The original struct definition in include/net/mac80211.h:
> >
> > struct ieee80211_tx_rate {
> > s8 idx;
> > u16 count:5,
> > flags:11;
> > } __packed;
> >
> > So we could infer that this is a packed struct, because the next field
> > doesn't come u16 aligned, not that it is a bitfield combined with
> > previous fields.
> >
> > The DWARF info:
> > <1><27639>: Abbrev Number: 16 (DW_TAG_structure_type)
> > <2763a> DW_AT_name : (indirect string, offset: 0x19d5a): ieee80211_tx_rate
> > <2763e> DW_AT_byte_size : 3
> > <27643> DW_AT_sibling : <0x27678>
> > <2><27647>: Abbrev Number: 19 (DW_TAG_member)
> > <27648> DW_AT_name : idx
> > <27650> DW_AT_type : <0x11d>
> > <27654> DW_AT_data_member_location: 0
> > <2><27655>: Abbrev Number: 30 (DW_TAG_member)
> > <27656> DW_AT_name : (indirect string, offset: 0x1789d): count
> > <2765e> DW_AT_type : <0x144>
> > <27662> DW_AT_byte_size : 2
> > <27663> DW_AT_bit_size : 5
> > <27664> DW_AT_bit_offset : 8
> > <27665> DW_AT_data_member_location: 0
> > <2><27666>: Abbrev Number: 30 (DW_TAG_member)
> > <27667> DW_AT_name : (indirect string, offset: 0xc978): flags
> > <2766f> DW_AT_type : <0x144>
> > <27673> DW_AT_byte_size : 2
> > <27674> DW_AT_bit_size : 11
> > <27675> DW_AT_bit_offset : 5
> > <27676> DW_AT_data_member_location: 1
>
>
> So this entire DWARF makes sense only if all the bit_offsets are
> little-endian (and I somehow got impression that DWARF is always
> specified in big-endian notation, even for little-endian targets...
> weird).
So:
The vmlinux being used is the aarch64 one, that is big endian.
[acme@quaco perf]$ file ../pahole/examples/vmlinux-aarch64
../pahole/examples/vmlinux-aarch64: ELF 64-bit MSB pie executable, ARM aarch64, version 1 (SYSV), statically linked, BuildID[sha1]=7fcff18ec214eee2f8c6b288fca21dc6d9771ad6, with debug_info, not stripped
[acme@quaco perf]$ file ../build/v5.0+/vmlinux
../build/v5.0+/vmlinux: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), statically linked, BuildID[sha1]=a64f366e91a0342c858bac5442dccd8a29601d9c, with debug_info, not stripped
[acme@quaco perf]$
But, in http://dwarfstd.org/doc/Dwarf3.pdf, page 75, we have:
<quote>
If the data member entry describes a bit field, then that entry has the
following attributes:
- A DW_AT_byte_size attribute whose value (see Section 2.19) is the
number of bytes that contain an instance of the bit field and any
padding bits. The byte size attribute may be omitted if the size of
the object containing the bit field can be inferred from the type
attribute of the data member containing the bit field.
- A DW_AT_bit_offset attribute whose value (see Section 2.19) is the
number of bits to the left of the leftmost (most significant) bit of
the bit field value.
- A DW_AT_bit_size attribute whose value (see Section 2.19) is the
number of bits occupied by the bit field value. The location
description for a bit field calculates the address of an anonym ous
object containing the bit field. The address is relati ve to the
structure, union, or cla ss that most closely encloses the bit field
declaration. The number of bytes in this anonymous object is the value
of the byte size attribute of the bit field. The offset (in bits) fr
om the most significant bit of the anonymous object to the most
significant bit of the bit field is the value of the bit offset
attribute.
And following it there is an example with some tables, I'll read this
more thorougly later.
> 1. idx is single byte, at offset 0 (DW_AT_data_member_location: 0)
> 2. count is part of 16-bit bitfield (DW_AT_byte_size : 2), located
> at offset 0 (DW_AT_data_member_location: 0, so bitfields appear to
> always be aligned here), at little-endian offset 8 (so is part of
> second byte), takes next 5 bits (bits 8-12 in little-endian).
> 3. flags is again part of 16-bit bitfield, with base byte offset 1
> (DW_AT_data_member_location: 1). It has bit_offset: 5 and bit size 11,
> so takes bits 5-15 starting at byte 1, or in absolute bit
> "coordinates" within struct: 13-23. So technically, this field is part
> of another bitfield, which overlaps with the one that has count.
>
> We can represent this equivalently as bitfield starting at offset 2,
> but borrowing 3 bits from previous byte. Which will preserve
> alignedness of bitfields. That was my intent, but I was always
> interpreting DWARF values as big-endian offsets.
>
> Seems like we actually need to detect big/little endiannes for DWARF,
> similar to BTF, what do you think?
I guess so :-)
> > So the message has to take into account if the previous field has a
> > bitfield_size != 0, no?
>
> I honestly didn't have time to go and check the logic of detection of
> merging with prev/next bitfield, but I suspect it still has bugs,
> because it derives it from field sizes, not they bit positions. So
> there are probably some more examples that have confusing hints.
>
> >
> > I have to look at the other csets, perhaps you do this there?
> >
>
> No, I think this problem persists. I changed dwarf_loader to align
> bitfields naturally, which btf_loader does, but it might be sometimes
> misleading w.r.t. these "combined with previous/next fields". With
> DWARF we have DW_AT_data_member_location, which provides more
> information than BTF's field bit offset.
>
> > I'm just trying to test it patch by patch to see what changes after each
> > one.
> >
> > - Arnaldo
--
- Arnaldo
