Review feedback

Author: gnurizen

.github/workflows/build.yml

@@ -37,10 +37,7 @@ jobs:
         with:
           go-version: '~1.25'
 
-      # The goreleaser-cross image is Debian-based and ships clang, but not
-      # libbpf-dev or linux-libc-dev. Install them so the goreleaser
-      # `before:` hook (make probes-bpf-all) can compile the probes BPF
-      # objects that //go:embed bpf needs.
+      # BPF dependencies for probes feature 
       - name: Install BPF build dependencies
         run: |
           apt-get update

.gitignore

@@ -6,6 +6,9 @@
 /out
 /bin
 *.bpf.o
+# bpf2go output: .o is regenerated by `make probes-bpf`; the .go wrapper is committed.
+probes/probe_bpfel.o
+probes/probe_bpfeb.o
 probes/bpf/*.ll
 TODO.md
 minikube-*

.goreleaser.yml

@@ -1,12 +1,12 @@
 version: 2
 env:
   - CGO_ENABLED=1
-# Build the BPF object before goreleaser builds the Go binaries.
-# //go:embed bpf needs probe.bpf.o present on disk at `go build` time;
-# goreleaser cross-compiles Go but doesn't otherwise know about our BPF
-# sidecar. Requires clang + libbpf-dev + linux-libc-dev in the build
-# environment. One object covers every Go target arch -- our BPF code is
-# arch-agnostic at the bytecode level.
+# Regenerate the bpf2go-emitted BPF loader + object before goreleaser builds
+# the Go binaries. probes/probe_bpfel.go uses `//go:embed probe_bpfel.o` and
+# needs the .o present on disk at `go build` time; goreleaser cross-compiles
+# Go but doesn't otherwise know about our BPF sidecar. Requires clang +
+# libbpf-dev + linux-libc-dev in the build environment. One object covers
+# every Go target arch -- our BPF code is arch-agnostic at the bytecode level.
 before:
   hooks:
     - make probes-bpf

Makefile

@@ -3,7 +3,8 @@
 GOARCH ?= $(shell go env GOARCH)
 CLANG ?= clang
 
-PROBES_BPF_OBJ := probes/bpf/probe.bpf.o
+PROBES_BPF_GO  := probes/probe_bpfel.go
+PROBES_BPF_OBJ := probes/probe_bpfel.o
 
 # On Debian-based systems (including the goreleaser-cross container)
 # `linux-libc-dev` installs arch-specific headers under
@@ -19,26 +20,22 @@ BPF_CFLAGS := $(if $(MULTIARCH),-I/usr/include/$(MULTIARCH))
 
 all: crossbuild
 
-# BPF compilation is host-architecture-independent: clang -target bpf emits
-# the same bytecode regardless of where it runs. Our BPF program doesn't
-# touch any arch-specific macros (PT_REGS_*, etc.), so a single object
-# loads on amd64 and arm64 alike -- the embedded loader picks it without
-# consulting runtime.GOARCH.
+# BPF compilation runs through cilium/ebpf's bpf2go (driven by `go generate`
+# via probes/gen.go). bpf2go compiles probe.bpf.c with clang, parses the
+# resulting BTF, and emits a Go file mirroring the C structs + a loader.
 #
-# If a future change adds PT_REGS_PARM1(ctx) or similar, the file will need
-# to be compiled separately per arch with the matching -D__TARGET_ARCH_*
-# flag; at that point this rule should grow per-target variables again.
+# We restrict to `-target bpfel` because parca-agent only ships amd64/arm64
+# (both little-endian). Our BPF program touches no arch-specific macros
+# (PT_REGS_*, etc.), so a single bpfel object loads on both arches.
 #
 # Requires clang + libbpf headers (libbpf-dev / libbpf-devel) + kernel UAPI
 # headers (linux-libc-dev / kernel-headers) on the host. No cross-toolchain
 # needed.
 probes-bpf: $(PROBES_BPF_OBJ)
 
-$(PROBES_BPF_OBJ): probes/bpf/probe.bpf.c
-	$(CLANG) -O2 -g -target bpf \
-		$(BPF_CFLAGS) \
-		-Wall -Werror \
-		-c $< -o $@
+$(PROBES_BPF_GO) $(PROBES_BPF_OBJ): probes/bpf/probe.bpf.c probes/gen.go
+	BPF2GO_CC=$(CLANG) BPF2GO_CFLAGS="-O2 -g -Wall -Werror $(BPF_CFLAGS)" \
+		go generate ./probes/
 
 # crossbuild produces both amd64 and arm64 release binaries via goreleaser
 # inside its cross container. Goreleaser doesn't invoke make, so we build

flags/flags.go

@@ -165,7 +165,7 @@ type Flags struct {
 
 	OTLPLogging bool `default:"false" help:"Forward parca-agent's own logrus output to the remote-store as OTLP log records (in addition to local stderr). Requires a remote-store; ignored in offline mode."`
 
-	ProbeConfig string `default:"" help:"Path to a YAML file declaring uprobe attachments. When set, parca-agent attaches a uprobe per matching binary and streams probe-fire events to the configured remote-store as OTLP/Arrow logs. Empty disables the feature."`
+	ProbeConfigFile string `default:"" help:"Path to a YAML file declaring uprobe attachments. When set, parca-agent attaches a uprobe per matching binary and streams probe-fire events to the configured remote-store as OTLP/Arrow logs. Empty disables the feature."`
 }
 
 type ExitCode int

main.go

@@ -444,22 +444,26 @@ func mainWithExitCode() flags.ExitCode {
 		}
 	}
 
-	if f.ProbeConfig != "" {
+	if f.ProbeConfigFile != "" {
 		if grpcConn == nil {
 			return flags.Failure("--probe-config requires a remote-store; cannot be used in offline mode")
 		}
 		probesSvc, err := probes.Start(mainCtx, probes.StartConfig{
-			ConfigPath: f.ProbeConfig,
+			ConfigPath: f.ProbeConfigFile,
 		}, parcaReporter)
 		if err != nil {
-			return flags.Failure("Failed to start probes: %v", err)
+			// Probes are a non-essential feature; a bad config (typo in
+			// probes.yaml, missing symbol, etc.) shouldn't take the whole
+			// agent down. Log and continue without probe support.
+			log.Errorf("probes disabled — failed to start: %v", err)
+		} else {
+			parcaReporter.SetProbes(probesSvc)
+			defer func() {
+				if err := probesSvc.Close(); err != nil {
+					log.Warnf("probes: close: %v", err)
+				}
+			}()
 		}
-		parcaReporter.SetProbes(probesSvc)
-		defer func() {
-			if err := probesSvc.Close(); err != nil {
-				log.Warnf("probes: close: %v", err)
-			}
-		}()
 	}
 
 	includeEnvVars := libpf.Set[string]{}

probes/bpf/README.md

@@ -1,31 +1,27 @@
 # probes BPF
 
-`probe.bpf.c` is the kernel-side uprobe program. The compiled output
-`probe.bpf.o` is produced by `make probes-bpf` from the parca-agent
-repository root and is git-ignored.
+`probe.bpf.c` is the kernel-side uprobe program. It is compiled by
+cilium/ebpf's `bpf2go` (driven from `../gen.go` via `go generate`), which
+emits `../probe_bpfel.go` (a Go mirror of the C structs plus a loader)
+and `../probe_bpfel.o` (the BPF bytecode, embedded into the .go file
+via `//go:embed`).
 
-A single object covers every supported Go architecture. Our BPF program
-does not touch any arch-specific macros (`PT_REGS_PARM1` and friends from
-`bpf/bpf_tracing.h`), so `clang -target bpf` emits identical bytecode
-regardless of host arch and regardless of which `__TARGET_ARCH_*` define
-might be in scope. The `runtime.GOARCH`-keyed lookup in `../loader.go`
-was deliberately removed to avoid pretending we ship arch-specific
-artifacts when we don't.
-
-If a future change adds anything that branches on `__TARGET_ARCH_*` (most
-likely a `PT_REGS_PARMn(ctx)` to read a function argument at uprobe
-entry), the build will need to split per arch again:
-
-- per-target `BPF_ARCH` variable in the Makefile mapping `amd64 -> x86`
-  and `arm64 -> arm64`,
-- `-D__TARGET_ARCH_$(BPF_ARCH)` passed to clang,
-- a `probe.bpf.<GOARCH>` naming scheme,
-- a matching `runtime.GOARCH`-keyed embed lookup in `loader.go`.
+Run `make probes-bpf` from the repository root to regenerate both files.
+The Makefile injects `BPF2GO_CFLAGS` with the multiarch include path
+that Debian-derived distros need; on Fedora/RHEL `go generate ./probes/`
+works directly.
 
-Git history has the previous form if you need to crib from it.
-
-This README is committed so `//go:embed bpf` in `../loader.go` always
-has a valid embed target even before any BPF object has been compiled.
+A single object covers every supported Go architecture. Our BPF program
+does not touch any arch-specific macros (`PT_REGS_PARM1` and friends
+from `bpf/bpf_tracing.h`), so `clang -target bpf` emits identical
+bytecode regardless of host arch. We restrict bpf2go to `-target bpfel`
+because parca-agent only ships amd64/arm64 (both little-endian).
+
+If a future change adds anything that branches on `__TARGET_ARCH_*`
+(most likely a `PT_REGS_PARMn(ctx)` to read a function argument at
+uprobe entry), the build will need to split per arch again. The bpf2go
+invocation in `../gen.go` would grow a second `-target bpfeb` variant
+and per-arch `__TARGET_ARCH_*` defines in `BPF2GO_CFLAGS`.
 
 Build dependencies on the host:
 

probes/bpf/probe.bpf.c

@@ -46,9 +46,13 @@ struct probe_event {
 	__u32 tid;
 	char  comm[16];
 	__u32 spec_id;
-	__u32 is_main;      // 1 iff tid == tgid
 };
 
+// Force probe_event into BTF so bpf2go can mirror it as a Go struct. Without
+// this, the type is only referenced from a local in probe_exit and clang's
+// BTF emitter elides it, breaking `bpf2go -type probe_event`.
+const struct probe_event *_probe_event_btf_force __attribute__((unused));
+
 // scope_stacks: per-tid open-scope state. LRU evicts dead threads without
 // requiring a sched_process_exit hook.
 struct {
@@ -143,7 +147,6 @@ int probe_exit(struct pt_regs *ctx)
 	e->pid         = tgid;
 	e->tid         = tid;
 	e->spec_id     = cookie_spec_id(cookie);
-	e->is_main     = (tid == tgid) ? 1 : 0;
 	bpf_get_current_comm(&e->comm, sizeof(e->comm));
 
 	bpf_ringbuf_submit(e, 0);

probes/gen.go

@@ -0,0 +1,18 @@
+//go:build linux
+
+package probes
+
+// probe_bpfel.{go,o} are generated from bpf/probe.bpf.c by cilium/ebpf's
+// bpf2go. The Go file is committed; the .o file is embedded into it via
+// `go:embed` and is also written next to it so the loader can find it.
+//
+// To regenerate after editing the C source, run `make probes-bpf` (the
+// Makefile injects BPF2GO_CFLAGS with the multiarch include path that
+// Debian-derived distros need). On distros without multiarch headers
+// (Fedora/RHEL), `go generate ./probes/` works directly.
+//
+// We restrict to `bpfel` because parca-agent only ships amd64 and arm64
+// builds (both little-endian); generating a `bpfeb` variant would add
+// dead bytes to the binary.
+
+//go:generate go run github.com/cilium/ebpf/cmd/bpf2go -target bpfel -type probe_event probe bpf/probe.bpf.c

probes/loader.go

@@ -3,56 +3,17 @@
 package probes
 
 import (
-	"bytes"
-	"embed"
-	"encoding/binary"
 	"fmt"
 
-	"github.com/cilium/ebpf"
 	"github.com/cilium/ebpf/ringbuf"
 )
 
-//go:embed bpf
-var bpfFS embed.FS
-
-// probeEventSize is the on-the-wire size of `struct probe_event` from
-// probe.bpf.c. Keep the Go decoder in sync if the C struct changes.
-const probeEventSize = 48
-
-// rawProbeEvent is the layout produced by bpf/probe.bpf.c. Decoded from
-// little-endian raw bytes; do not change field order without updating
-// the C struct.
-type rawProbeEvent struct {
-	KtimeNs    uint64
-	DurationNs uint64
-	PID        uint32
-	TID        uint32
-	Comm       [16]byte
-	SpecID     uint32
-	IsMain     uint32
-}
-
-func decodeEvent(b []byte, e *rawProbeEvent) error {
-	if len(b) < probeEventSize {
-		return fmt.Errorf("ringbuf record too short: %d bytes", len(b))
-	}
-	e.KtimeNs = binary.LittleEndian.Uint64(b[0:8])
-	e.DurationNs = binary.LittleEndian.Uint64(b[8:16])
-	e.PID = binary.LittleEndian.Uint32(b[16:20])
-	e.TID = binary.LittleEndian.Uint32(b[20:24])
-	copy(e.Comm[:], b[24:40])
-	e.SpecID = binary.LittleEndian.Uint32(b[40:44])
-	e.IsMain = binary.LittleEndian.Uint32(b[44:48])
-	return nil
-}
-
-// loadedBPF holds the resources owned by a loaded BPF program.
+// loadedBPF holds the resources owned by a loaded BPF program. The
+// `objs` struct is the bpf2go-generated container for the maps and
+// programs in bpf/probe.bpf.c.
 type loadedBPF struct {
-	coll       *ebpf.Collection
-	progEntry  *ebpf.Program
-	progExit   *ebpf.Program
-	ringbufMap *ebpf.Map
-	reader     *ringbuf.Reader
+	objs   probeObjects
+	reader *ringbuf.Reader
 }
 
 func (l *loadedBPF) Close() error {
@@ -61,74 +22,26 @@ func (l *loadedBPF) Close() error {
 	}
 	var firstErr error
 	if l.reader != nil {
-		if err := l.reader.Close(); err != nil && firstErr == nil {
+		if err := l.reader.Close(); err != nil {
 			firstErr = err
 		}
 	}
-	if l.coll != nil {
-		l.coll.Close()
+	if err := l.objs.Close(); err != nil && firstErr == nil {
+		firstErr = err
 	}
 	return firstErr
 }
 
 func loadBPF() (*loadedBPF, error) {
-	blob, err := readEmbeddedBPF()
-	if err != nil {
-		return nil, err
-	}
-
-	spec, err := ebpf.LoadCollectionSpecFromReader(bytes.NewReader(blob))
-	if err != nil {
-		return nil, fmt.Errorf("load BPF collection spec: %w", err)
+	var l loadedBPF
+	if err := loadProbeObjects(&l.objs, nil); err != nil {
+		return nil, fmt.Errorf("load probe BPF objects: %w", err)
 	}
-
-	coll, err := ebpf.NewCollection(spec)
+	reader, err := ringbuf.NewReader(l.objs.ProbeEvents)
 	if err != nil {
-		return nil, fmt.Errorf("create BPF collection: %w", err)
-	}
-
-	progEntry, ok := coll.Programs["probe_entry"]
-	if !ok {
-		coll.Close()
-		return nil, fmt.Errorf("BPF object missing program 'probe_entry'")
-	}
-	progExit, ok := coll.Programs["probe_exit"]
-	if !ok {
-		coll.Close()
-		return nil, fmt.Errorf("BPF object missing program 'probe_exit'")
-	}
-	rbMap, ok := coll.Maps["probe_events"]
-	if !ok {
-		coll.Close()
-		return nil, fmt.Errorf("BPF object missing map 'probe_events'")
-	}
-
-	reader, err := ringbuf.NewReader(rbMap)
-	if err != nil {
-		coll.Close()
+		_ = l.objs.Close()
 		return nil, fmt.Errorf("open ringbuf reader: %w", err)
 	}
-
-	return &loadedBPF{
-		coll:       coll,
-		progEntry:  progEntry,
-		progExit:   progExit,
-		ringbufMap: rbMap,
-		reader:     reader,
-	}, nil
-}
-
-func readEmbeddedBPF() ([]byte, error) {
-	// One object covers every arch: clang -target bpf is host-independent
-	// and our program uses no arch-specific macros (PT_REGS_*, etc.). If
-	// that changes, this lookup will need a runtime.GOARCH-keyed suffix.
-	const name = "bpf/probe.bpf.o"
-	blob, err := bpfFS.ReadFile(name)
-	if err != nil {
-		return nil, fmt.Errorf("embedded BPF object %s not found: run `make probes-bpf` (%w)", name, err)
-	}
-	if len(blob) < 4 || string(blob[:4]) != "\x7fELF" {
-		return nil, fmt.Errorf("embedded BPF object %s is not a valid ELF (size=%d): run `make probes-bpf`", name, len(blob))
-	}
-	return blob, nil
+	l.reader = reader
+	return &l, nil
 }