cmd/bpf2go: fix s390x target

The current format of output filenames for a specific GOARCH is

    <stem>_<clang target>_<linux target>.go

The s390x GOARCH has a linux target of "s390", which leads to
a file name like

    foo_bpfeb_s390.go

The problem is that "_s390" is interpreted as an additional build
constraint by the Go toolchain. In effect, the will only be included
in the build if GOARCH is s390 and s390x which is of course not possible.

Fix this by changing the output filename format and add some code which
removes files with the old naming schema. There is a small risk that this
might in the future delete files that we haven't written, but the chances
of that seem low enough.

Extend the bpf2go tests to ensure that we've generated the right build
tags for arm64, amd64 and s390x.

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>

Author: lmb

cmd/bpf2go/compile_test.go

@@ -12,7 +12,8 @@ import (
 const minimalSocketFilter = `__attribute__((section("socket"), used)) int main() { return 0; }`
 
 func TestCompile(t *testing.T) {
-	dir := mustWriteTempFile(t, "test.c", minimalSocketFilter)
+	dir := t.TempDir()
+	mustWriteFile(t, dir, "test.c", minimalSocketFilter)
 
 	var dep bytes.Buffer
 	err := compile(compileArgs{
@@ -46,7 +47,8 @@ func TestCompile(t *testing.T) {
 
 func TestReproducibleCompile(t *testing.T) {
 	clangBin := clangBin(t)
-	dir := mustWriteTempFile(t, "test.c", minimalSocketFilter)
+	dir := t.TempDir()
+	mustWriteFile(t, dir, "test.c", minimalSocketFilter)
 
 	err := compile(compileArgs{
 		cc:     clangBin,
@@ -84,7 +86,8 @@ func TestReproducibleCompile(t *testing.T) {
 }
 
 func TestTriggerMissingTarget(t *testing.T) {
-	dir := mustWriteTempFile(t, "test.c", `_Pragma(__BPF_TARGET_MISSING);`)
+	dir := t.TempDir()
+	mustWriteFile(t, dir, "test.c", `_Pragma(__BPF_TARGET_MISSING);`)
 
 	err := compile(compileArgs{
 		cc:     clangBin(t),
@@ -148,19 +151,10 @@ nothing:
 	}
 }
 
-func mustWriteTempFile(t *testing.T, name, contents string) string {
-	t.Helper()
-
-	tmp, err := os.MkdirTemp("", "bpf2go")
-	if err != nil {
-		t.Fatal(err)
-	}
-	t.Cleanup(func() { os.RemoveAll(tmp) })
-
-	tmpFile := filepath.Join(tmp, name)
+func mustWriteFile(tb testing.TB, dir, name, contents string) {
+	tb.Helper()
+	tmpFile := filepath.Join(dir, name)
 	if err := os.WriteFile(tmpFile, []byte(contents), 0660); err != nil {
-		t.Fatal(err)
+		tb.Fatal(err)
 	}
-
-	return tmp
 }

cmd/bpf2go/main.go

@@ -307,9 +307,17 @@ func (b2g *bpf2go) convert(tgt target, goarches []goarch) (err error) {
 	if outputStem == "" {
 		outputStem = strings.ToLower(b2g.identStem)
 	}
+
+	// The output filename must not match any of the following patterns:
+	//
+	//     *_GOOS
+	//     *_GOARCH
+	//     *_GOOS_GOARCH
+	//
+	// Otherwise it is interpreted as a build constraint by the Go toolchain.
 	stem := fmt.Sprintf("%s_%s", outputStem, tgt.clang)
 	if tgt.linux != "" {
-		stem = fmt.Sprintf("%s_%s_%s", outputStem, tgt.clang, tgt.linux)
+		stem = fmt.Sprintf("%s_%s_%s", outputStem, tgt.linux, tgt.clang)
 	}
 
 	objFileName := filepath.Join(b2g.outputDir, stem+".o")
@@ -333,6 +341,10 @@ func (b2g *bpf2go) convert(tgt target, goarches []goarch) (err error) {
 		cFlags = append(cFlags, "-D__TARGET_ARCH_"+tgt.linux)
 	}
 
+	if err := b2g.removeOldOutputFiles(outputStem, tgt); err != nil {
+		return fmt.Errorf("remove obsolete output: %w", err)
+	}
+
 	var dep bytes.Buffer
 	err = compile(compileArgs{
 		cc:     b2g.cc,
@@ -415,6 +427,31 @@ func (b2g *bpf2go) convert(tgt target, goarches []goarch) (err error) {
 	return nil
 }
 
+// removeOldOutputFiles removes output files generated by an old naming scheme.
+//
+// In the old scheme some linux targets were interpreted as build constraints
+// by the go toolchain.
+func (b2g *bpf2go) removeOldOutputFiles(outputStem string, tgt target) error {
+	if tgt.linux == "" {
+		return nil
+	}
+
+	stem := fmt.Sprintf("%s_%s_%s", outputStem, tgt.clang, tgt.linux)
+	for _, ext := range []string{".o", ".go"} {
+		filename := filepath.Join(b2g.outputDir, stem+ext)
+
+		if err := os.Remove(filename); errors.Is(err, os.ErrNotExist) {
+			continue
+		} else if err != nil {
+			return err
+		}
+
+		fmt.Fprintln(b2g.stdout, "Removed obsolete", filename)
+	}
+
+	return nil
+}
+
 type target struct {
 	// Clang arch string, used to define the clang -target flag, as per
 	// "clang -print-targets".

cmd/bpf2go/main_test.go

@@ -19,29 +19,22 @@ import (
 
 func TestRun(t *testing.T) {
 	clangBin := clangBin(t)
-	dir := mustWriteTempFile(t, "test.c", minimalSocketFilter)
+	dir := t.TempDir()
+	mustWriteFile(t, dir, "test.c", minimalSocketFilter)
 
-	cwd, err := os.Getwd()
-	if err != nil {
-		t.Fatal(err)
-	}
+	modRoot, err := filepath.Abs("../..")
+	qt.Assert(t, qt.IsNil(err))
 
-	modRoot := filepath.Clean(filepath.Join(cwd, "../.."))
 	if _, err := os.Stat(filepath.Join(modRoot, "go.mod")); os.IsNotExist(err) {
 		t.Fatal("No go.mod file in", modRoot)
 	}
 
-	tmpDir, err := os.MkdirTemp("", "bpf2go-module-*")
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer os.RemoveAll(tmpDir)
-
+	modDir := t.TempDir()
 	execInModule := func(name string, args ...string) {
 		t.Helper()
 
 		cmd := exec.Command(name, args...)
-		cmd.Dir = tmpDir
+		cmd.Dir = modDir
 		if out, err := cmd.CombinedOutput(); err != nil {
 			if out := string(out); out != "" {
 				t.Log(out)
@@ -62,8 +55,15 @@ func TestRun(t *testing.T) {
 		fmt.Sprintf("-replace=%s=%s", module, modRoot),
 	)
 
-	err = run(io.Discard, "foo", tmpDir, []string{
+	goarches := []string{
+		"amd64", // little-endian
+		"arm64",
+		"s390x", // big-endian
+	}
+
+	err = run(io.Discard, "main", modDir, []string{
 		"-cc", clangBin,
+		"-target", strings.Join(goarches, ","),
 		"bar",
 		filepath.Join(dir, "test.c"),
 	})
@@ -72,18 +72,24 @@ func TestRun(t *testing.T) {
 		t.Fatal("Can't run:", err)
 	}
 
-	for _, arch := range []string{
-		"amd64", // little-endian
-		"s390x", // big-endian
-	} {
+	mustWriteFile(t, modDir, "main.go",
+		`
+package main
+
+func main() {
+	var obj barObjects
+	println(obj.Main)
+}`)
+
+	for _, arch := range goarches {
 		t.Run(arch, func(t *testing.T) {
-			goBin := exec.Command("go", "build", "-mod=mod")
-			goBin.Dir = tmpDir
-			goBin.Env = append(os.Environ(),
+			goBuild := exec.Command("go", "build", "-mod=mod", "-o", "/dev/null")
+			goBuild.Dir = modDir
+			goBuild.Env = append(os.Environ(),
 				"GOOS=linux",
 				"GOARCH="+arch,
 			)
-			out, err := goBin.CombinedOutput()
+			out, err := goBuild.CombinedOutput()
 			if err != nil {
 				if out := string(out); out != "" {
 					t.Log(out)
@@ -112,7 +118,8 @@ func TestErrorMentionsEnvVar(t *testing.T) {
 }
 
 func TestDisableStripping(t *testing.T) {
-	dir := mustWriteTempFile(t, "test.c", minimalSocketFilter)
+	dir := t.TempDir()
+	mustWriteFile(t, dir, "test.c", minimalSocketFilter)
 
 	err := run(io.Discard, "foo", dir, []string{
 		"-cc", clangBin(t),
@@ -213,7 +220,7 @@ func TestCollectTargetsErrors(t *testing.T) {
 		target string
 	}{
 		{"unknown", "frood"},
-		{"no linux target", "mips64p32le"},
+		{"no linux target", "mipsle"},
 	}
 
 	for _, test := range tests {
@@ -228,7 +235,8 @@ func TestCollectTargetsErrors(t *testing.T) {
 }
 
 func TestConvertGOARCH(t *testing.T) {
-	tmp := mustWriteTempFile(t, "test.c",
+	tmp := t.TempDir()
+	mustWriteFile(t, tmp, "test.c",
 		`
 #ifndef __TARGET_ARCH_x86
 #error __TARGET_ARCH_x86 is not defined
@@ -432,22 +440,41 @@ func TestParseArgs(t *testing.T) {
 }
 
 func TestGoarches(t *testing.T) {
-	exe, err := exec.LookPath("go")
-	if errors.Is(err, exec.ErrNotFound) {
-		t.Skip("go binary is not in PATH")
-	}
-	qt.Assert(t, qt.IsNil(err))
+	exe := goBin(t)
 
 	for goarch := range targetByGoArch {
 		t.Run(string(goarch), func(t *testing.T) {
 			goEnv := exec.Command(exe, "env")
-			goEnv.Env = []string{"GOOS=linux", "GOARCH=" + string(goarch)}
+			goEnv.Env = []string{"GOROOT=/", "GOOS=linux", "GOARCH=" + string(goarch)}
 			output, err := goEnv.CombinedOutput()
 			qt.Assert(t, qt.IsNil(err), qt.Commentf("go output is:\n%s", string(output)))
 		})
 	}
 }
 
+func TestClangTargets(t *testing.T) {
+	exe := goBin(t)
+
+	clangTargets := map[string]struct{}{}
+	for _, tgt := range targetByGoArch {
+		clangTargets[tgt.clang] = struct{}{}
+	}
+
+	for target := range clangTargets {
+		for _, env := range []string{"GOOS", "GOARCH"} {
+			env += "=" + target
+			t.Run(env, func(t *testing.T) {
+				goEnv := exec.Command(exe, "env")
+				goEnv.Env = []string{"GOROOT=/", env}
+				output, err := goEnv.CombinedOutput()
+				t.Log("go output is:", string(output))
+				qt.Assert(t, qt.IsNotNil(err), qt.Commentf("No clang target should be a valid build constraint"))
+			})
+		}
+
+	}
+}
+
 func clangBin(t *testing.T) string {
 	t.Helper()
 
@@ -465,3 +492,15 @@ func clangBin(t *testing.T) string {
 	t.Log("Testing against", clang)
 	return clang
 }
+
+func goBin(t *testing.T) string {
+	t.Helper()
+
+	exe, err := exec.LookPath("go")
+	if errors.Is(err, exec.ErrNotFound) {
+		t.Skip("go binary is not in PATH")
+	}
+	qt.Assert(t, qt.IsNil(err))
+
+	return exe
+}