Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpf2go: ergonomic enums #1636

Closed
wants to merge 2 commits into from
+108 −96
Closed

bpf2go: ergonomic enums #1636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ad8c53b
bpf2go: simplify types sorting
mejedi Dec 20, 2024
12b600e
bpf2go: generate short names for enum elements
mejedi Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion btf/format.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ type GoFormatter struct {
// EnumIdentifier is called for each element of an enum. By default the
// name of the enum type is concatenated with Identifier(element).
EnumIdentifier func(name, element string) string

// ShortEnumIdentifier is called for each element of an enum. An
// empty result causes short name to be omitted (default).
ShortEnumIdentifier func(name, element string) string
}

// TypeDeclaration generates a Go type declaration for a BTF type.
Expand Down Expand Up @@ -52,6 +56,14 @@ func (gf *GoFormatter) enumIdentifier(name, element string) string {
return name + gf.identifier(element)
}

func (gf *GoFormatter) shortEnumIdentifier(name, element string) string {
if gf.ShortEnumIdentifier != nil {
return gf.ShortEnumIdentifier(name, element)
}

return ""
}

// writeTypeDecl outputs a declaration of the given type.
//
// It encodes https://golang.org/ref/spec#Type_declarations:
Expand All @@ -76,13 +88,17 @@ func (gf *GoFormatter) writeTypeDecl(name string, typ Type) error {

gf.w.WriteString("; const ( ")
for _, ev := range e.Values {
id := gf.enumIdentifier(name, ev.Name)
var value any
if e.Signed {
value = int64(ev.Value)
} else {
value = ev.Value
}
if shortID := gf.shortEnumIdentifier(name, ev.Name); shortID != "" {
// output short identifier first (stringer prefers earlier decalarations)
fmt.Fprintf(&gf.w, "%s %s = %d; ", shortID, name, value)
}
id := gf.enumIdentifier(name, ev.Name)
fmt.Fprintf(&gf.w, "%s %s = %d; ", id, name, value)
}
gf.w.WriteString(")")
Expand Down
82 changes: 41 additions & 41 deletions cmd/bpf2go/gen/output.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"go/build/constraint"
"go/token"
"io"
"sort"
"strings"
"text/template"
"unicode"
Expand Down Expand Up @@ -141,22 +140,50 @@ func Generate(args GenerateArgs) error {
programs[name] = args.Identifier(name)
}

typeNames := make(map[btf.Type]string)
for _, typ := range args.Types {
// NB: This also deduplicates types.
typeNames[typ] = args.Stem + args.Identifier(typ.TypeName())
tn := templateName(args.Stem)
reservedNames := map[string]struct{}{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first sight, I'd model this as a property to be specified during construction of the GoFormatter. Everytime the formatter generates an identifier (struct or otherwise), it can check uniqueness and push it to the set of seen names. I think it's a good feature to have in the formatter to prevent it from emitting invalid code, it doesn't need to be b2g-specific.

This way, you also avoid having to close over reservedNames and typeByName to get it into an ShortEnumIdentifier function.

tn.Specs(): {},
tn.MapSpecs(): {},
tn.ProgramSpecs(): {},
tn.VariableSpecs(): {},
tn.Objects(): {},
tn.Maps(): {},
tn.Programs(): {},
tn.Variables(): {},
}

// Ensure we don't have conflicting names and generate a sorted list of
// named types so that the output is stable.
types, err := sortTypes(typeNames)
if err != nil {
return err
typeByName := map[string]btf.Type{}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use make() for consistency?

nameByType := map[btf.Type]string{}
for _, typ := range args.Types {
// NB: This also deduplicates types.
name := args.Stem + args.Identifier(typ.TypeName())
if _, reserved := reservedNames[name]; reserved {
return fmt.Errorf("type name %q is reserved", name)
}
if otherType, ok := typeByName[name]; ok {
if otherType == typ {
continue
}
return fmt.Errorf("type name %q is used multiple times", name)
}
typeByName[name] = typ
nameByType[typ] = name
}

gf := &btf.GoFormatter{
Names: typeNames,
Names: nameByType,
Identifier: args.Identifier,
ShortEnumIdentifier: func(_, element string) string {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be made a part of EnumIdentifier instead? Not sure why this deserves its own hook, it's just another step during enum identifier generation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why this deserves its own hook, it's just another step during enum identifier generation.

I propose generating BOTH long and short identifiers to maintain backwards compatibility.

elementName := args.Stem + args.Identifier(element)
if _, nameTaken := typeByName[elementName]; nameTaken {
return ""
}
if _, nameReserved := reservedNames[elementName]; nameReserved {
return ""
}
reservedNames[elementName] = struct{}{}
return elementName
},
}

ctx := struct {
Expand All @@ -168,8 +195,7 @@ func Generate(args GenerateArgs) error {
Maps map[string]string
Variables map[string]string
Programs map[string]string
Types []btf.Type
TypeNames map[btf.Type]string
Types map[string]btf.Type
File string
}{
gf,
Expand All @@ -180,44 +206,18 @@ func Generate(args GenerateArgs) error {
maps,
variables,
programs,
types,
typeNames,
typeByName,
args.ObjectFile,
}

var buf bytes.Buffer
if err := commonTemplate.Execute(&buf, &ctx); err != nil {
return fmt.Errorf("can't generate types: %s", err)
return fmt.Errorf("can't generate types: %v", err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why %v over %s? This change feels a little arbitrary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually meant %w.

}

return internal.WriteFormatted(buf.Bytes(), args.Output)
}

// sortTypes returns a list of types sorted by their (generated) Go type name.
//
// Duplicate Go type names are rejected.
func sortTypes(typeNames map[btf.Type]string) ([]btf.Type, error) {
var types []btf.Type
var names []string
for typ, name := range typeNames {
i := sort.SearchStrings(names, name)
if i >= len(names) {
types = append(types, typ)
names = append(names, name)
continue
}

if names[i] == name {
return nil, fmt.Errorf("type name %q is used multiple times", name)
}

types = append(types[:i], append([]btf.Type{typ}, types[i:]...)...)
names = append(names[:i], append([]string{name}, names[i:]...)...)
}

return types, nil
}

func toUpperFirst(str string) string {
first, n := utf8.DecodeRuneInString(str)
return string(unicode.ToUpper(first)) + str[n:]
Expand Down
4 changes: 2 additions & 2 deletions cmd/bpf2go/gen/output.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ import (
)

{{- if .Types }}
{{- range $type := .Types }}
{{ $.TypeDeclaration (index $.TypeNames $type) $type }}
{{- range $name, $type := .Types }}
{{ $.TypeDeclaration $name $type }}

{{ end }}
{{- end }}
Expand Down
96 changes: 44 additions & 52 deletions cmd/bpf2go/gen/output_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package gen
import (
"bytes"
"fmt"
"regexp"
"strings"
"testing"

Expand All @@ -12,58 +13,6 @@ import (
"github.com/cilium/ebpf/cmd/bpf2go/internal"
)

func TestOrderTypes(t *testing.T) {
a := &btf.Int{}
b := &btf.Int{}
c := &btf.Int{}

for _, test := range []struct {
name string
in map[btf.Type]string
out []btf.Type
}{
{
"order",
map[btf.Type]string{
a: "foo",
b: "bar",
c: "baz",
},
[]btf.Type{b, c, a},
},
} {
t.Run(test.name, func(t *testing.T) {
result, err := sortTypes(test.in)
qt.Assert(t, qt.IsNil(err))
qt.Assert(t, qt.Equals(len(result), len(test.out)))
for i, o := range test.out {
if result[i] != o {
t.Fatalf("Index %d: expected %p got %p", i, o, result[i])
}
}
})
}

for _, test := range []struct {
name string
in map[btf.Type]string
}{
{
"duplicate names",
map[btf.Type]string{
a: "foo",
b: "foo",
},
},
} {
t.Run(test.name, func(t *testing.T) {
result, err := sortTypes(test.in)
qt.Assert(t, qt.IsNotNil(err))
qt.Assert(t, qt.IsNil(result))
})
}
}

func TestPackageImport(t *testing.T) {
var buf bytes.Buffer
err := Generate(GenerateArgs{
Expand Down Expand Up @@ -116,3 +65,46 @@ func TestObjects(t *testing.T) {
qt.Assert(t, qt.StringContains(str, "Var1 *ebpf.Variable `ebpf:\"var_1\"`"))
qt.Assert(t, qt.StringContains(str, "ProgFoo1 *ebpf.Program `ebpf:\"prog_foo_1\"`"))
}

func TestEnums(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to write this test without the range over conflict true/false? I'm not a big fan of treating invariants like a list of test cases, because you inevitably end up sprinkling if <invariant> around anyway.

These are technically separate tests; perhaps you could extract the series of qt.Assert calls into a test() closure within the function to avoid repetition.

for _, conflict := range []bool{false, true} {
t.Run(fmt.Sprintf("conflict=%v", conflict), func(t *testing.T) {
var buf bytes.Buffer
args := GenerateArgs{
Package: "foo",
Stem: "bar",
Types: []btf.Type{
&btf.Enum{Name: "EnumName", Size: 4, Values: []btf.EnumValue{
{Name: "V1", Value: 1}, {Name: "V2", Value: 2}, {Name: "conflict", Value: 0}}},
},
Output: &buf,
}
if conflict {
args.Types = append(args.Types, &btf.Struct{Name: "conflict", Size: 4})
}
err := Generate(args)
qt.Assert(t, qt.IsNil(err))

str := buf.String()

qt.Assert(t, qt.Matches(str, wsSeparated("barEnumNameV1", "barEnumName", "=", "1")))
qt.Assert(t, qt.Matches(str, wsSeparated("barEnumNameV2", "barEnumName", "=", "2")))
qt.Assert(t, qt.Matches(str, wsSeparated("barEnumNameConflict", "barEnumName", "=", "0")))

// short enum element names, only generated if they don't conflict with other decls
qt.Assert(t, qt.Matches(str, wsSeparated("barV1", "barEnumName", "=", "1")))
qt.Assert(t, qt.Matches(str, wsSeparated("barV2", "barEnumName", "=", "2")))

pred := qt.Matches(str, wsSeparated("barConflict", "barEnumName", "=", "0"))
if conflict {
qt.Assert(t, qt.Not(pred))
} else {
qt.Assert(t, pred)
}
})
}
}

func wsSeparated(terms ...string) *regexp.Regexp {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this strictly required? IMO it's harder to read wsSeparated("barEnumNameV1", "barEnumName", "=", "1") than simply "barEnumNameV1 barEnumName = 1". Or is the amount of whitespace not deterministic?

Copy link
Contributor Author

@mejedi mejedi Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the amount of whitespace not deterministic?

The amount of whitespace depends on the length of other identifiers in the set since formatter inserts extra spaces for alignment. It doesn't improve readability indeed, but makes it easier to update test code in the future.

return regexp.MustCompile(strings.Join(terms, `\s+`))
}
2 changes: 2 additions & 0 deletions cmd/bpf2go/test/test_bpfeb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions cmd/bpf2go/test/test_bpfel.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading