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

cmd/bpf2go: fix s390x target #1312

Merged
merged 1 commit into from
Jan 24, 2024
Merged

cmd/bpf2go: fix s390x target #1312

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 10 additions & 16 deletions cmd/bpf2go/compile_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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
}
39 changes: 38 additions & 1 deletion cmd/bpf2go/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

With tgt.clang filling in the _GOARCH value #1307 will not be resolved.
While go tool dist list does not list s390 as supported architecture, internals of Go still are s390 aware and make a difference between s390 and s390x.
As tgt.clang should be s390 at this point, it will generate a file with the according name and Gos build constraints will not consider it for s390x.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, clang target is only ever bpfel or bpfeb. This will break if we ever need to support clang targets other than those two which then end up matching go constraints, but why would we?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

List of clang targets:

$ clang -print-targets

  Registered Targets:
    aarch64     - AArch64 (little endian)
    aarch64_32  - AArch64 (little endian ILP32)
    aarch64_be  - AArch64 (big endian)
    amdgcn      - AMD GCN GPUs
    arm         - ARM
    arm64       - ARM64 (little endian)
    arm64_32    - ARM64 (little endian ILP32)
    armeb       - ARM (big endian)
    avr         - Atmel AVR Microcontroller
    bpf         - BPF (host endian)
    bpfeb       - BPF (big endian)
    bpfel       - BPF (little endian)
    hexagon     - Hexagon
    lanai       - Lanai
    loongarch32 - 32-bit LoongArch
    loongarch64 - 64-bit LoongArch
    mips        - MIPS (32-bit big endian)
    mips64      - MIPS (64-bit big endian)
    mips64el    - MIPS (64-bit little endian)
    mipsel      - MIPS (32-bit little endian)
    msp430      - MSP430 [experimental]
    nvptx       - NVIDIA PTX 32-bit
    nvptx64     - NVIDIA PTX 64-bit
    ppc32       - PowerPC 32
    ppc32le     - PowerPC 32 LE
    ppc64       - PowerPC 64
    ppc64le     - PowerPC 64 LE
    r600        - AMD GPUs HD2XXX-HD6XXX
    riscv32     - 32-bit RISC-V
    riscv64     - 64-bit RISC-V
    sparc       - Sparc
    sparcel     - Sparc LE
    sparcv9     - Sparc V9
    systemz     - SystemZ
    thumb       - Thumb
    thumbeb     - Thumb (big endian)
    ve          - VE
    wasm32      - WebAssembly 32-bit
    wasm64      - WebAssembly 64-bit
    x86         - 32-bit X86: Pentium-Pro and above
    x86-64      - 64-bit X86: EM64T and AMD64
    xcore       - XCore

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry for coming back on this just now.
I tested this PR again with the issue described in #1307. And I think, I now also got the origin of my confusion. As I tested this PR with code with the linked repository from #1307, I did not clean up the and removed the checked-in compiled binary objects. So I ended up with a strange mix that led to my confusion.
Testing it again now, I can confirm, that this PR works as intended.

Copy link
Contributor

Choose a reason for hiding this comment

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

@msherif1234 I tested this PR by checking out this PR, compiling bpf2go and replace the bpfgo in the generate instruction in https://github.com/netobserv/netobserv-ebpf-agent/blob/504add4c32d1259afabe2d2ab24664a684109d5c/pkg/ebpf/tracer.go#L29 with the compiled version from this PR

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")
Expand All @@ -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,
Expand Down Expand Up @@ -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".
Expand Down
101 changes: 70 additions & 31 deletions cmd/bpf2go/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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"),
})
Expand All @@ -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)
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand Down Expand Up @@ -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()

Expand All @@ -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
}
2 changes: 1 addition & 1 deletion examples/uretprobe/bpf_bpfel_x86.go → examples/uretprobe/bpf_x86_bpfel.go
View file
Open in desktop

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

0 examples/uretprobe/bpf_bpfel_x86.o → examples/uretprobe/bpf_x86_bpfel.o
View file
Open in desktop
File renamed without changes.
Loading