bpf2go: support multiple source files #1762

yonch · 2025-04-21T19:37:56Z

See issue #1758

Has been used successfully in unvariance/collector#109, PR unvariance/collector#110.

Tested in a dev container on MacOs and on a Linux bare metal box.

Signed-off-by: Jonathan Perry <[email protected]>

- replace the direct command execution for linking multiple BPF object files with a new `Link` function in the `gen` package. - add unit tests for linking Signed-off-by: Jonathan Perry <[email protected]>

- Moved the temporary object renaming logic from compileOne() to convert() - Simplified naming and cleanup Signed-off-by: Jonathan Perry <[email protected]>

Signed-off-by: Jonathan Perry <[email protected]>

…fic endianness handling Signed-off-by: Jonathan Perry <[email protected]>

… name Signed-off-by: Jonathan Perry <[email protected]>

ti-mo

This looks good overall, thank you! Left a few nits.

ti-mo · 2025-05-06T12:56:03Z

cmd/bpf2go/gen/link_test.go

+		Source:           filepath.Join(dir, "func1.c"),
+		Dest:             obj1,
+	})
+	if err != nil {


Here and everywhere: use qt.Assert().

ti-mo · 2025-05-06T12:56:43Z

cmd/bpf2go/main.go

-(usually some version of clang).
+source files are C files that are compiled using the specified compiler
+(usually some version of clang) and linked together into a single
+BPF program.


Specifying multiple source files incurs an extra dependency (bpftool). I'd document that clearly here.

ti-mo · 2025-05-06T13:03:56Z

cmd/bpf2go/main.go

+	for _, source := range b2g.sourceFiles {
+		if _, err := os.Stat(source); os.IsNotExist(err) {
+			return fmt.Errorf("file %s doesn't exist", source)
+		} else if err != nil {


} else ... { is not necessary here since the opposing case returns. The typical pattern is:

_, err := ... if errors.Is(err, fs.ErrNotExist) { // Note: os.IsFoo functions are unofficially deprecated. return fmt.Errorf(...) } if err != nil { return err }

Also, the PathError returned by os.Stat always contains the path provided to the function call, so it's not necessary to generate your own error. Doing so also swallows the fs.ErrNotExist sentinel, making error handling confusing for the user.

ti-mo · 2025-05-06T13:10:10Z

cmd/bpf2go/main.go

+	for _, source := range b2g.sourceFiles {
+		// Determine the target object file name
+		var targetObjFileName string
+		if len(b2g.sourceFiles) > 1 {


Consider initializing targetObjFileName := objFileName and drop the the else below.

ti-mo · 2025-05-06T13:15:49Z

cmd/bpf2go/makedep.go

+	for _, deps := range depsSlices {
+		for _, dep := range deps {
+			// If we've seen this file before, merge prerequisites
+			if existing, ok := merged[dep.file]; ok {


Consider removing one level of indentation for the majority of the body using continue:

existing, ok := merged[dep.file] if !ok { // First time seeing this file, just copy prerequisites merged[dep.file] = make([]string, len(dep.prerequisites)) copy(merged[dep.file], dep.prerequisites) continue } ...

lmb

Agree with Timo, this is nice :) Be aware that libbpf linking may produce linked object files which we don't parse: #739 I don't think it's a pre-requisite to get this fixed, but something to be aware of which might make this less useful to you.

lmb · 2025-05-07T09:59:04Z

cmd/bpf2go/gen/link.go

+
+// LinkArgs specifies the arguments for linking multiple BPF object files together.
+type LinkArgs struct {
+	// Destination object file name


Mention that paths must be absolute?

lmb · 2025-05-07T09:59:23Z

cmd/bpf2go/gen/link.go

+)
+
+// LinkArgs specifies the arguments for linking multiple BPF object files together.
+type LinkArgs struct {


Document that this only supports host endianness.

lmb · 2025-05-07T10:00:48Z

cmd/bpf2go/main_test.go

+		t.Fatal("Can't get module root:", err)
+	}
+
+	if _, err := os.Stat(filepath.Join(modRoot, "go.mod")); os.IsNotExist(err) {


Can you move creating the temporary module into a helper instead of copying the code?

lmb · 2025-05-07T10:03:13Z

cmd/bpf2go/makedep_test.go

+		{"/foo/third.c", []string{"/foo/bar.h"}},
+	}
+
+	if !reflect.DeepEqual(merged, want) {


Suggested change

if !reflect.DeepEqual(merged, want) {

qt.Assert(t, qt.DeepEquals(merged, want))

lmb · 2025-05-07T10:07:05Z

cmd/bpf2go/makedep.go

+			// If we've seen this file before, merge prerequisites
+			if existing, ok := merged[dep.file]; ok {
+				// Combine prerequisites, avoiding duplicates
+				prereqs := make(map[string]struct{})


I think you might be able to simplify this by appending to a slice. At the end, sort the slice and use https://pkg.go.dev/slices#Compact in a second pass.

lmb · 2025-05-07T10:09:14Z

cmd/bpf2go/makedep_test.go

+	})
+
+	want := []dependency{
+		{"/foo/main.c", []string{"/foo/bar.h", "/foo/baz.h", "/foo/qux.h"}},


How can we get different dependencies for two compilations of the same file? Wouldn't it be more appropriate to check that both sets of dependencies are identical? Or that the sets are disjoint?

lmb · 2025-05-07T10:10:48Z

cmd/bpf2go/main.go

+		if err != nil {
+			return nil, fmt.Errorf("convert source file to absolute path: %w", err)
+		}
+		b2g.sourceFiles[i] = absPath


Nit: just appending to b2g.sourceFiles without pre-allocating the slice is probably fine / easier to reason about.

Suggested change

b2g.sourceFiles[i] = absPath

b2g.sourceFiles = append(b2g.sourceFiles, absPath)

yonch requested a review from a team as a code owner April 21, 2025 19:37

yonch force-pushed the main branch from b709db4 to 8f0c2e7 Compare April 21, 2025 19:39

yonch added 6 commits April 21, 2025 14:40

bpf2go: support multiple source files

4500480

Signed-off-by: Jonathan Perry <[email protected]>

bpf2go: refactor object file linking into gen and add tests

38f89b6

- replace the direct command execution for linking multiple BPF object files with a new `Link` function in the `gen` package. - add unit tests for linking Signed-off-by: Jonathan Perry <[email protected]>

bpf2go: refactor compileOne function to simplify object file handling

0104501

- Moved the temporary object renaming logic from compileOne() to convert() - Simplified naming and cleanup Signed-off-by: Jonathan Perry <[email protected]>

bpf2go: add test for handling multiple source files

438ad98

Signed-off-by: Jonathan Perry <[email protected]>

bpf2go: fix tests for multiple source files to use architecture-speci…

70844c7

…fic endianness handling Signed-off-by: Jonathan Perry <[email protected]>

bpf2go: avoid os.Rename, instead compiling to the desired object file…

e83058d

… name Signed-off-by: Jonathan Perry <[email protected]>

yonch force-pushed the main branch from 8f0c2e7 to e83058d Compare April 21, 2025 19:41

yonch mentioned this pull request Apr 23, 2025

bpf2go: support multiple source files #1758

Open

ti-mo reviewed May 6, 2025

View reviewed changes

ti-mo requested review from lmb and dylandreimerink May 6, 2025 13:18

lmb requested changes May 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

bpf2go: support multiple source files #1762

bpf2go: support multiple source files #1762

Uh oh!

yonch commented Apr 21, 2025

Uh oh!

ti-mo left a comment

Uh oh!

ti-mo May 6, 2025

Uh oh!

ti-mo May 6, 2025

Uh oh!

ti-mo May 6, 2025

Uh oh!

ti-mo May 6, 2025

Uh oh!

ti-mo May 6, 2025

Uh oh!

lmb left a comment

Uh oh!

lmb May 7, 2025

Uh oh!

lmb May 7, 2025

Uh oh!

lmb May 7, 2025

Uh oh!

lmb May 7, 2025

Uh oh!

lmb May 7, 2025

Uh oh!

lmb May 7, 2025

Uh oh!

lmb May 7, 2025

Uh oh!

Uh oh!

	if !reflect.DeepEqual(merged, want) {
	qt.Assert(t, qt.DeepEquals(merged, want))

	b2g.sourceFiles[i] = absPath
	b2g.sourceFiles = append(b2g.sourceFiles, absPath)

bpf2go: support multiple source files #1762

Are you sure you want to change the base?

bpf2go: support multiple source files #1762

Uh oh!

Conversation

yonch commented Apr 21, 2025

Uh oh!

ti-mo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!