diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 69db468..b091456 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -10,6 +10,7 @@ concurrency: cancel-in-progress: ${{ github.event_name == 'pull_request' }} jobs: + # Lint the codebase using golangci-lint golangci: name: lint runs-on: ubuntu-latest @@ -22,3 +23,24 @@ jobs: uses: golangci/golangci-lint-action@v9 with: version: v2.11 + # Run go test to ensure tests pass + gotest: + name: tests + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + - uses: actions/setup-go@v6 + with: + go-version: stable + - name: go test + run: go test -v ./... + # Build testdata and check that no git changes are needed, to ensure testdata is up to date + testdata: + name: ensure-testdata + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + - name: build testdata + run: | + make -C testdata docker + test -z "$(git status testdata --porcelain)" || (echo "please run 'make -C testdata docker' and submit your changes"; exit 1) diff --git a/cmd/stackwhere/collection_test.go b/cmd/stackwhere/collection_test.go new file mode 100644 index 0000000..faf002a --- /dev/null +++ b/cmd/stackwhere/collection_test.go @@ -0,0 +1,29 @@ +package main + +import ( + "testing" + + "github.com/cilium/stackwhere/internal/dwarf" +) + +func TestGetProgramStackUsage(t *testing.T) { + tree, err := dwarf.NewDWARFTree("../../testdata/basic.o") + if err != nil { + t.Fatalf("failed to parse DWARF data: %v", err) + } + + stackUsagePerProgram := map[string]int64{} + for _, prog := range tree.ByType(dwarf.TagSubprogram) { + if !isBPFProgram(prog) { + continue + } + + stackUsagePerProgram[prog.Name()] = getProgramStackUsage(prog) + } + + stackUsage := stackUsagePerProgram["cil_entry"] + expectedStackUsage := int64(56) + if stackUsage != expectedStackUsage { + t.Errorf("expected stack usage %d, got %d", expectedStackUsage, stackUsage) + } +} diff --git a/cmd/stackwhere/program_test.go b/cmd/stackwhere/program_test.go new file mode 100644 index 0000000..a1b4a74 --- /dev/null +++ b/cmd/stackwhere/program_test.go @@ -0,0 +1,73 @@ +package main + +import ( + "testing" + + "github.com/cilium/stackwhere/internal/dwarf" +) + +func TestGetStackSlotUsage(t *testing.T) { + tree, err := dwarf.NewDWARFTree("../../testdata/basic.o") + if err != nil { + t.Fatalf("failed to parse DWARF data: %v", err) + } + + stackUsage := getStackSlotUsage(tree, "cil_entry") + if len(stackUsage) != 3 { + t.Fatalf("expected 3 stack slots, got %d", len(stackUsage)) + } + + // Verify each stack slot group contains expected slots + slotGroups := []struct { + index int + expected map[string]int64 + }{ + { + index: 0, + expected: map[string]int64{ + "a": 32, + "b": 32, + "c": 32, + }, + }, + { + index: 1, + expected: map[string]int64{ + "two_inlined_a": 16, + "two_inlined_b": 16, + "two_inlined_c": 16, + }, + }, + { + index: 2, + expected: map[string]int64{ + "one_inlined_d": 8, + }, + }, + } + + for _, group := range slotGroups { + found := make(map[string]int64) + for _, slot := range stackUsage[group.index] { + found[slot.name] = slot.byteSize + } + + // Check all expected slots are present with correct size + for name, expectedSize := range group.expected { + actualSize, ok := found[name] + if !ok { + t.Errorf("slot group %d: expected slot %q not found", group.index, name) + continue + } + if actualSize != expectedSize { + t.Errorf("slot group %d: slot %q has size %d, expected %d", group.index, name, actualSize, expectedSize) + } + delete(found, name) + } + + // Check no unexpected slots are present + if len(found) > 0 { + t.Errorf("slot group %d: unexpected slots found: %v", group.index, found) + } + } +} diff --git a/testdata/Makefile b/testdata/Makefile new file mode 100644 index 0000000..46dc713 --- /dev/null +++ b/testdata/Makefile @@ -0,0 +1,31 @@ +CONTAINER_ENGINE ?= docker + +IMAGE := "ghcr.io/cilium/ebpf-builder" +VERSION := "1777990914" + +CLANG ?= clang-22 + +FLAGS := +# -g to enable DWARF + BTF +FLAGS += -g +# -O2 to enable optimization needed for BPF +FLAGS += -O2 +# -target bpf to specify the BPF target +FLAGS += -target bpf + +.PHONY: docker +docker: + $(CONTAINER_ENGINE) run --rm -v $(shell pwd):/src -w /src $(IMAGE):$(VERSION) make all + +TARGETS := \ + basic + +.PHONY: all +all: $(addsuffix .o,$(TARGETS)) + +%.o: %.c + $(CLANG) $(FLAGS) -c $< -o $@ + +.PHONY: clean +clean: + rm -f *.o diff --git a/testdata/basic.c b/testdata/basic.c new file mode 100644 index 0000000..142fee1 --- /dev/null +++ b/testdata/basic.c @@ -0,0 +1,97 @@ +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +/* Copyright Authors of Cilium */ + +#define __section(X) __attribute__((section(X), used)) +#define __always_inline inline __attribute__((always_inline)) + +struct __sk_buff +{ + unsigned long long dummy; // just to make sure the struct is not empty +}; + +// The object file should produce the following output from the test: +// TestGetStackSlotUsage expects 3 stack slot groups for cil_entry: +// Slot group 0: a (32 bytes), b (32 bytes), c (32 bytes) +// Slot group 1: two_inlined_a (16 bytes), two_inlined_b (16 bytes), two_inlined_c (16 bytes) +// Slot group 2: one_inlined_d (8 bytes) + +struct four +{ + unsigned long long a; + unsigned long long b; + unsigned long long c; + unsigned long long d; +}; + +struct two +{ + unsigned long long a; + unsigned long long b; +}; + +// An ASM block, with no actual instructions, but we tell the compiler that we need to have the +// address of x in a register. And the only way to get the address of x is to put it on the stack. +// This forces the compiler to put x on the stack, even if it could have otherwise optimized it away +// or kept it in registers. +#define force_on_stack(x) asm volatile("" ::"r"(&x)) + +void __always_inline inlined_d() +{ + unsigned long long one_inlined_d = 0; + force_on_stack(one_inlined_d); +} + +void __always_inline inlined_c() +{ + struct two two_inlined_c = {}; + force_on_stack(two_inlined_c); + // `two_inlined_c` is never used after this point, but `inlined_d` will use a new stack slot + // anyway because the "lifetime" of `two_inlined_c` ends at the end of the function. + // So the compiler will use more stack space here than technically needed. + inlined_d(); +} + +void __always_inline inlined_b() +{ + struct two two_inlined_b = {}; + force_on_stack(two_inlined_b); +} + +void __always_inline inlined_a() +{ + struct two two_inlined_a = {}; + force_on_stack(two_inlined_a); +} + +__section("tc") int cil_entry(struct __sk_buff *ctx) +{ + { + // a will live on the stack until the end of the scope, so inlined_a and inlined_b + // cannot reuse its stack space. But inlined_b can reuse the stack space of + // inlined_a. + struct four a = {}; + force_on_stack(a); + inlined_a(); + inlined_b(); + } + + // Variables in this scope can reuse the stack space of a, inlined_a, and inlined_b. + // So `b`, `two_inlined_a` and `two_inlined_b` will be placed on the same stack slots as + // those used above. + { + struct four b = {}; + force_on_stack(b); + inlined_a(); + inlined_b(); + } + + // `c` will fit over stack used by `a` and `b`. + { + struct four c = {}; + force_on_stack(c); + // `inlined_c` calls `inlined_d` before the end of its function, and thus `inlined_d` + // will use an additional stack slot. + inlined_c(); + } + return 0; +} diff --git a/testdata/basic.o b/testdata/basic.o new file mode 100644 index 0000000..b05bdd6 Binary files /dev/null and b/testdata/basic.o differ