From 3187212bef7cb94de1958846fc544d7c8ec91e17 Mon Sep 17 00:00:00 2001 From: Lorenz Bauer Date: Tue, 3 May 2022 18:08:31 +0000 Subject: [PATCH] btf: remove Int.Bits and Int.Offset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unfortunately BTF has two ways to encode bitfields. You can create a Member and set Offset / BitfieldSize or create an Int that has the offset and bitfield size baked in. The background for this can be found in [1]. Remove Int.Bits and Int.Offset in favour of Member.BitfieldSize and Member.Offset. Also get rid of a bunch of incorrect checks for bitfields, which we didn't catch due to the weird encoding. This cleans up the API, but may make our lives worse if we end up having to support writing out legacy bitfields. Let's hope it doesn't come to that. The performance of parsing vmlinux hasn't changed dramatically: name old time/op new time/op delta ParseVmlinux-4 62.1ms ± 1% 61.8ms ± 2% ~ (p=0.686 n=4+4) name old alloc/op new alloc/op delta ParseVmlinux-4 44.2MB ± 0% 44.7MB ± 0% +1.14% (p=0.029 n=4+4) name old allocs/op new allocs/op delta ParseVmlinux-4 718k ± 0% 718k ± 0% +0.00% (p=0.029 n=4+4) 1: https://github.com/torvalds/linux/commit/9d5f9f701b1891466fb3dbb1806ad97716f95cc3 --- internal/btf/btf_test.go | 54 +++++++++++++++---------- internal/btf/core.go | 17 +------- internal/btf/core_test.go | 2 - internal/btf/types.go | 82 ++++++++++++++++++++++++++++---------- internal/btf/types_test.go | 2 +- 5 files changed, 95 insertions(+), 62 deletions(-) diff --git a/internal/btf/btf_test.go b/internal/btf/btf_test.go index 06d6f27cc..1697d9058 100644 --- a/internal/btf/btf_test.go +++ b/internal/btf/btf_test.go @@ -134,28 +134,38 @@ func TestTypeByName(t *testing.T) { t.Fatal("multiple TypeByName calls for `iphdr` name do not return the same addresses") } - for _, m := range iphdr1.Members { - if m.Name == "version" { - // __u8 is a typedef - td, ok := m.Type.(*Typedef) - if !ok { - t.Fatalf("version member of iphdr should be a __u8 typedef: actual: %T", m.Type) - } - u8int, ok := td.Type.(*Int) - if !ok { - t.Fatalf("__u8 typedef should point to an Int type: actual: %T", td.Type) - } - if u8int.Bits != 8 { - t.Fatalf("incorrect bit size of an __u8 int: expected: 8 actual: %d", u8int.Bits) - } - if u8int.Encoding != 0 { - t.Fatalf("incorrect encoding of an __u8 int: expected: 0 actual: %x", u8int.Encoding) - } - if u8int.Offset != 0 { - t.Fatalf("incorrect int offset of an __u8 int: expected: 0 actual: %d", u8int.Offset) - } - break - } + // Excerpt from linux/ip.h, https://elixir.bootlin.com/linux/latest/A/ident/iphdr + // + // struct iphdr { + // #if defined(__LITTLE_ENDIAN_BITFIELD) + // __u8 ihl:4, version:4; + // #elif defined (__BIG_ENDIAN_BITFIELD) + // __u8 version:4, ihl:4; + // #else + // ... + // } + // + // The BTF we test against is for little endian. + m := iphdr1.Members[1] + if m.Name != "version" { + t.Fatal("Expected version as the second member, got", m.Name) + } + td, ok := m.Type.(*Typedef) + if !ok { + t.Fatalf("version member of iphdr should be a __u8 typedef: actual: %T", m.Type) + } + u8, ok := td.Type.(*Int) + if !ok { + t.Fatalf("__u8 typedef should point to an Int type: actual: %T", td.Type) + } + if m.BitfieldSize != 4 { + t.Fatalf("incorrect bitfield size: expected: 4 actual: %d", m.BitfieldSize) + } + if u8.Encoding != 0 { + t.Fatalf("incorrect encoding of an __u8 int: expected: 0 actual: %x", u8.Encoding) + } + if m.Offset != 4 { + t.Fatalf("incorrect bitfield offset: expected: 4 actual: %d", m.Offset) } } diff --git a/internal/btf/core.go b/internal/btf/core.go index 7042a1e70..5e521dedb 100644 --- a/internal/btf/core.go +++ b/internal/btf/core.go @@ -893,15 +893,9 @@ func coreAreTypesCompatible(localType Type, targetType Type) error { } switch lv := (localType).(type) { - case *Void, *Struct, *Union, *Enum, *Fwd: + case *Void, *Struct, *Union, *Enum, *Fwd, *Int: // Nothing to do here - case *Int: - tv := targetType.(*Int) - if lv.isBitfield() || tv.isBitfield() { - return fmt.Errorf("bitfield: %w", errImpossibleRelocation) - } - case *Pointer, *Array: depth++ localType.walk(&localTs) @@ -983,7 +977,7 @@ func coreAreMembersCompatible(localType Type, targetType Type) error { } switch lv := localType.(type) { - case *Array, *Pointer, *Float: + case *Array, *Pointer, *Float, *Int: return nil case *Enum: @@ -994,13 +988,6 @@ func coreAreMembersCompatible(localType Type, targetType Type) error { tv := targetType.(*Fwd) return doNamesMatch(lv.Name, tv.Name) - case *Int: - tv := targetType.(*Int) - if lv.isBitfield() || tv.isBitfield() { - return fmt.Errorf("bitfield: %w", errImpossibleRelocation) - } - return nil - default: return fmt.Errorf("type %s: %w", localType, ErrNotSupported) } diff --git a/internal/btf/core_test.go b/internal/btf/core_test.go index 1dd678dac..69c9fca71 100644 --- a/internal/btf/core_test.go +++ b/internal/btf/core_test.go @@ -25,7 +25,6 @@ func TestCOREAreTypesCompatible(t *testing.T) { {&Enum{Name: "a"}, &Enum{Name: "b"}, true}, {&Fwd{Name: "a"}, &Fwd{Name: "b"}, true}, {&Int{Name: "a", Size: 2}, &Int{Name: "b", Size: 4}, true}, - {&Int{Offset: 1}, &Int{}, false}, {&Pointer{Target: &Void{}}, &Pointer{Target: &Void{}}, true}, {&Pointer{Target: &Void{}}, &Void{}, false}, {&Array{Type: &Void{}}, &Array{Type: &Void{}}, true}, @@ -99,7 +98,6 @@ func TestCOREAreMembersCompatible(t *testing.T) { {&Fwd{Name: "a"}, &Fwd{Name: "a___foo"}, true}, {&Fwd{Name: "a"}, &Fwd{Name: ""}, true}, {&Int{Name: "a", Size: 2}, &Int{Name: "b", Size: 4}, true}, - {&Int{Offset: 1}, &Int{}, false}, {&Pointer{Target: &Void{}}, &Pointer{Target: &Void{}}, true}, {&Pointer{Target: &Void{}}, &Void{}, false}, {&Array{Type: &Int{Size: 1}}, &Array{Type: &Int{Encoding: Signed}}, true}, diff --git a/internal/btf/types.go b/internal/btf/types.go index 0615a8474..782bd6754 100644 --- a/internal/btf/types.go +++ b/internal/btf/types.go @@ -127,20 +127,10 @@ type Int struct { // The size of the integer in bytes. Size uint32 Encoding IntEncoding - // Offset is the starting bit offset. Currently always 0. - Offset Bits - Bits Bits } func (i *Int) Format(fs fmt.State, verb rune) { - extra := []interface{}{ - i.Encoding, - "size=", i.Size * 8, - } - if i.Bits > 0 { - extra = append(extra, "bits=", i.Bits) - } - formatType(fs, verb, i, extra...) + formatType(fs, verb, i, i.Encoding, "size=", i.Size*8) } func (i *Int) TypeName() string { return i.Name } @@ -151,10 +141,6 @@ func (i *Int) copy() Type { return &cpy } -func (i *Int) isBitfield() bool { - return i.Offset > 0 -} - // Pointer is a pointer to another type. type Pointer struct { TypeID @@ -844,6 +830,15 @@ func inflateRawTypes(rawTypes []rawType, rawStrings *stringTable) ([]Type, map[e return nil } + type bitfieldFixupDef struct { + id TypeID + m *Member + } + + var ( + legacyBitfields = make(map[TypeID][2]Bits) // offset, size + bitfieldFixups []bitfieldFixupDef + ) convertMembers := func(raw []btfMember, kindFlag bool) ([]Member, error) { // NB: The fixup below relies on pre-allocating this array to // work, since otherwise append might re-allocate members. @@ -853,18 +848,48 @@ func inflateRawTypes(rawTypes []rawType, rawStrings *stringTable) ([]Type, map[e if err != nil { return nil, fmt.Errorf("can't get name for member %d: %w", i, err) } - m := Member{ + + members = append(members, Member{ Name: name, Offset: Bits(btfMember.Offset), - } + }) + + m := &members[i] + fixup(raw[i].Type, &m.Type) + if kindFlag { m.BitfieldSize = Bits(btfMember.Offset >> 24) m.Offset &= 0xffffff + // We ignore legacy bitfield definitions if the current composite + // is a new-style bitfield. This is kind of safe since offset and + // size on the type of the member must be zero if kindFlat is set + // according to spec. + continue } - members = append(members, m) - } - for i := range members { - fixup(raw[i].Type, &members[i].Type) + + // This may be a legacy bitfield, try to fix it up. + data, ok := legacyBitfields[raw[i].Type] + if ok { + // Bingo! + m.Offset += data[0] + m.BitfieldSize = data[1] + continue + } + + if m.Type != nil { + // We couldn't find a legacy bitfield, but we know that the member's + // type has already been inflated. Hence we know that it can't be + // a legacy bitfield and there is nothing left to do. + continue + } + + // We don't have fixup data, and the type we're pointing + // at hasn't been inflated yet. No choice but to defer + // the fixup. + bitfieldFixups = append(bitfieldFixups, bitfieldFixupDef{ + raw[i].Type, + m, + }) } return members, nil } @@ -884,8 +909,12 @@ func inflateRawTypes(rawTypes []rawType, rawStrings *stringTable) ([]Type, map[e switch raw.Kind() { case kindInt: + size := raw.Size() encoding, offset, bits := intEncoding(*raw.data.(*uint32)) - typ = &Int{id, name, raw.Size(), encoding, offset, bits} + if offset > 0 || bits.Bytes() != size { + legacyBitfields[id] = [2]Bits{offset, bits} + } + typ = &Int{id, name, size, encoding} case kindPointer: ptr := &Pointer{id, nil} @@ -1031,6 +1060,15 @@ func inflateRawTypes(rawTypes []rawType, rawStrings *stringTable) ([]Type, map[e *fixup.typ = types[i] } + for _, bitfieldFixup := range bitfieldFixups { + data, ok := legacyBitfields[bitfieldFixup.id] + if ok { + // This is indeed a legacy bitfield, fix it up. + bitfieldFixup.m.Offset += data[0] + bitfieldFixup.m.BitfieldSize = data[1] + } + } + for _, assertion := range assertions { if reflect.TypeOf(*assertion.typ) != assertion.want { return nil, nil, fmt.Errorf("expected %s, got %T", assertion.want, *assertion.typ) diff --git a/internal/btf/types_test.go b/internal/btf/types_test.go index 2d46bb7ee..b0c70c7b4 100644 --- a/internal/btf/types_test.go +++ b/internal/btf/types_test.go @@ -102,7 +102,7 @@ func ExampleType_validTypes() { func TestType(t *testing.T) { types := []func() Type{ func() Type { return &Void{} }, - func() Type { return &Int{Size: 2, Bits: 3} }, + func() Type { return &Int{Size: 2} }, func() Type { return &Pointer{Target: &Void{}} }, func() Type { return &Array{Type: &Int{}} }, func() Type {