Repository: tetratelabs/wazero (go)

Issue:
Compiler benchmarks panic due to a segmentation fault
**Describe the bug**

Running the `BenchmarkCompiler_compileMemoryCopy` benchmarks panic due to a segmentation fault.

**To Reproduce**

```
$ go test -run '^$' -bench BenchmarkCompiler_compileMemoryCopy -benchmem -count 10 ./internal/engine/compiler
```

**Expected behavior**

The benchmark should run to completion.

**Screenshots**

```
unexpected fault address 0x140000f4000
fatal error: fault
[signal SIGBUS: bus error code=0x1 addr=0x140000f4000 pc=0x140000f4000]

goroutine 8 [running]:
runtime.throw({0x102e09f2b?, 0x174?})
	/usr/local/go/src/runtime/panic.go:1047 +0x40 fp=0x140000d3c70 sp=0x140000d3c40 pc=0x102bf3d80
runtime.sigpanic()
	/usr/local/go/src/runtime/signal_unix.go:834 +0x10c fp=0x140000d3cb0 sp=0x140000d3c70 pc=0x102c0a92c
github.com/tetratelabs/wazero/internal/engine/compiler.(*compilerEnv).execBench(0x1400000c090, 0x1400006cc80, {0x140000f4000, 0x174, 0x14000033d78?})
	/Users/achilleroussel/go/src/github.com/tetratelabs/wazero/internal/engine/compiler/compiler_bench_test.go:114 +0x6c fp=0x140000d3d00 sp=0x140000d3cc0 pc=0x102da9acc
github.com/tetratelabs/wazero/internal/engine/compiler.BenchmarkCompiler_compileMemoryCopy.func1(0x1400006cc80)
	/Users/achilleroussel/go/src/github.com/tetratelabs/wazero/internal/engine/compiler/compiler_bench_test.go:53 +0x594 fp=0x140000d3f10 sp=0x140000d3d00 pc=0x102da9224
testing.(*B).runN(0x1400006cc80, 0x1)
	/usr/local/go/src/testing/benchmark.go:193 +0x128 fp=0x140000d3f90 sp=0x140000d3f10 pc=0x102c82478
testing.(*B).run1.func1()
	/usr/local/go/src/testing/benchmark.go:233 +0x50 fp=0x140000d3fd0 sp=0x140000d3f90 pc=0x102c82a90
runtime.goexit()
	/usr/local/go/src/runtime/asm_arm64.s:1172 +0x4 fp=0x140000d3fd0 sp=0x140000d3fd0 pc=0x102c26e34
created by testing.(*B).run1
	/usr/local/go/src/testing/benchmark.go:226 +0x8c
```

**Environment**
- Go version: 1.20
- wazero Version: 4ceef7b24533725f2ef0d406c3baf633d99eca32
- Host architecture: amd64, arm64
- Runtime mode: compiler

**Additional context**

I tested on both linux/amd64 and darwin/arm64, both panicked in the same way.

Pull request description:
compiler: fix benchmarks in compiler, engine; add to Makefile, update API calls
Some benchmarks were not running in CI. We enable them,
add them to the Makefile, and ensure that all are up-to-date.

Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>

---

looking into this. Essentially, the panic is due to codeSegment not being mmap'd; so this is enough:

```
func (j *compilerEnv) execBench(b *testing.B, codeSegment []byte) {
	executable := requireExecutable(codeSegment)
	b.StartTimer()
```

(unless you want to measure the mmap overhead too)

once this is done, there is another panic due to the change to how `functionDefintition`s are now loaded; looking into that too. Will open a PR.

as for running this in the CI, shall we add this to the Makefile?

```
bench:
	@go test -run=NONE -benchmem -bench=. ./internal/engine/compiler/...
	@go test -run=NONE -benchmem -bench=. ./internal/integration_test/bench/...
	@go test -benchmem -bench=. ./internal/integration_test/vs/... $(ensureCompilerFastest)

```

Fixes #1464.


Patch:
diff --git a/.github/workflows/commit.yaml b/.github/workflows/commit.yaml
index 2a4f7f7b..cce3748b 100644
--- a/.github/workflows/commit.yaml
+++ b/.github/workflows/commit.yaml
@@ -169,7 +169,7 @@ jobs:
           go-version: ${{ env.GO_VERSION }}
           cache: true
 
-      - run: make bench.check
+      - run: make bench
 
   # This ensures that internal/integration_test/fuzz is runnable, and is not intended to
   # run full-length fuzzing while trying to find low-hanging frontend bugs.
diff --git a/Makefile b/Makefile
index 3c142682..bc053f52 100644
--- a/Makefile
+++ b/Makefile
@@ -24,11 +24,7 @@ go_test_options ?= -timeout 300s
 ensureCompilerFastest := -ldflags '-X github.com/tetratelabs/wazero/internal/integration_test/vs.ensureCompilerFastest=true'
 .PHONY: bench
 bench:
-	@go test -run=NONE -benchmem -bench=. ./internal/integration_test/bench/...
-	@go test -benchmem -bench=. ./internal/integration_test/vs/... $(ensureCompilerFastest)
-
-.PHONY: bench.check
-bench.check:
+	@go test -run=NONE -benchmem -bench=. ./internal/engine/compiler/...
 	@go build ./internal/integration_test/bench/...
 	@# Don't use -test.benchmem as it isn't accurate when comparing against CGO libs
 	@for d in vs/time vs/wasmedge vs/wasmer vs/wasmtime ; do \
diff --git a/internal/platform/mmap_linux.go b/internal/platform/mmap_linux.go
new file mode 100644
index 00000000..55906e82
--- /dev/null
+++ b/internal/platform/mmap_linux.go
@@ -0,0 +1,76 @@
+package platform
+
+import (
+	"math/bits"
+	"os"
+	"sort"
+	"strconv"
+	"strings"
+	"syscall"
+)
+
+const (
+	// https://man7.org/linux/man-pages/man2/mmap.2.html
+	__MAP_HUGE_SHIFT = 26
+	__MAP_HUGETLB    = 0x40000
+)
+
+var hugePagesConfigs []hugePagesConfig
+
+type hugePagesConfig struct {
+	size int
+	flag int
+}
+
+func (hpc *hugePagesConfig) match(size int) bool {
+	return (size & (hpc.size - 1)) == 0
+}
+
+func init() {
+	dirents, err := os.ReadDir("/sys/kernel/mm/hugepages/")
+	if err != nil {
+		return
+	}
+
+	for _, dirent := range dirents {
+		name := dirent.Name()
+		if !strings.HasPrefix(name, "hugepages-") {
+			continue
+		}
+		if !strings.HasSuffix(name, "kB") {
+			continue
+		}
+		n, err := strconv.ParseUint(name[10:len(name)-2], 10, 64)
+		if err != nil {
+			continue
+		}
+		if bits.OnesCount64(n) != 1 {
+			continue
+		}
+		n *= 1024
+		hugePagesConfigs = append(hugePagesConfigs, hugePagesConfig{
+			size: int(n),
+			flag: int(bits.TrailingZeros64(n)<<__MAP_HUGE_SHIFT) | __MAP_HUGETLB,
+		})
+	}
+
+	sort.Slice(hugePagesConfigs, func(i, j int) bool {
+		return hugePagesConfigs[i].size > hugePagesConfigs[j].size
+	})
+}
+
+func mmapCodeSegment(size, prot int) ([]byte, error) {
+	flags := syscall.MAP_ANON | syscall.MAP_PRIVATE
+
+	for _, hugePagesConfig := range hugePagesConfigs {
+		if hugePagesConfig.match(size) {
+			b, err := syscall.Mmap(-1, 0, size, prot, flags|hugePagesConfig.flag)
+			if err != nil {
+				continue
+			}
+			return b, nil
+		}
+	}
+
+	return syscall.Mmap(-1, 0, size, prot, flags)
+}
diff --git a/internal/platform/mmap_other.go b/internal/platform/mmap_other.go
new file mode 100644
index 00000000..620cdd3c
--- /dev/null
+++ b/internal/platform/mmap_other.go
@@ -0,0 +1,18 @@
+// This uses syscall.Mprotect. Go's SDK only supports this on darwin and linux.
+//go:build darwin || freebsd
+
+package platform
+
+import "syscall"
+
+func mmapCodeSegment(size, prot int) ([]byte, error) {
+	return syscall.Mmap(
+		-1,
+		0,
+		size,
+		prot,
+		// Anonymous as this is not an actual file, but a memory,
+		// Private as this is in-process memory region.
+		syscall.MAP_ANON|syscall.MAP_PRIVATE,
+	)
+}
diff --git a/internal/platform/mmap.go b/internal/platform/mmap_unix.go
similarity index 58%
rename from internal/platform/mmap.go
rename to internal/platform/mmap_unix.go
index 870528db..cd6f5112 100644
--- a/internal/platform/mmap.go
+++ b/internal/platform/mmap_unix.go
@@ -1,4 +1,3 @@
-// This uses syscall.Mprotect. Go's SDK only supports this on darwin and linux.
 //go:build darwin || linux || freebsd
 
 package platform
@@ -8,6 +7,11 @@ import (
 	"unsafe"
 )
 
+const (
+	mmapProtAMD64 = syscall.PROT_READ | syscall.PROT_WRITE | syscall.PROT_EXEC
+	mmapProtARM64 = syscall.PROT_READ | syscall.PROT_WRITE
+)
+
 func munmapCodeSegment(code []byte) error {
 	return syscall.Munmap(code)
 }
@@ -15,20 +19,8 @@ func munmapCodeSegment(code []byte) error {
 // mmapCodeSegmentAMD64 gives all read-write-exec permission to the mmap region
 // to enter the function. Otherwise, segmentation fault exception is raised.
 func mmapCodeSegmentAMD64(size int) ([]byte, error) {
-	buf, err := syscall.Mmap(
-		-1,
-		0,
-		size,
-		// The region must be RWX: RW for writing native codes, X for executing the region.
-		syscall.PROT_READ|syscall.PROT_WRITE|syscall.PROT_EXEC,
-		// Anonymous as this is not an actual file, but a memory,
-		// Private as this is in-process memory region.
-		syscall.MAP_ANON|syscall.MAP_PRIVATE,
-	)
-	if err != nil {
-		return nil, err
-	}
-	return buf, err
+	// The region must be RWX: RW for writing native codes, X for executing the region.
+	return mmapCodeSegment(size, mmapProtAMD64)
 }
 
 // mmapCodeSegmentARM64 cannot give all read-write-exec permission to the mmap region.
@@ -36,20 +28,8 @@ func mmapCodeSegmentAMD64(size int) ([]byte, error) {
 // to the region so that we can write contents at call-sites. Callers are responsible to
 // execute MprotectRX on the returned buffer.
 func mmapCodeSegmentARM64(size int) ([]byte, error) {
-	buf, err := syscall.Mmap(
-		-1,
-		0,
-		size,
-		// The region must be RW: RW for writing native codes.
-		syscall.PROT_READ|syscall.PROT_WRITE,
-		// Anonymous as this is not an actual file, but a memory,
-		// Private as this is in-process memory region.
-		syscall.MAP_ANON|syscall.MAP_PRIVATE,
-	)
-	if err != nil {
-		return nil, err
-	}
-	return buf, err
+	// The region must be RW: RW for writing native codes.
+	return mmapCodeSegment(size, mmapProtARM64)
 }
 
 // MprotectRX is like syscall.Mprotect, defined locally so that freebsd compiles.
diff --git a/internal/platform/mremap_linux.go b/internal/platform/mremap_linux.go
deleted file mode 100644
index 77bfec41..00000000
--- a/internal/platform/mremap_linux.go
+++ /dev/null
@@ -1,43 +0,0 @@
-package platform
-
-import (
-	"syscall"
-	"unsafe"
-)
-
-const (
-	__MREMAP_MAYMOVE = 1
-)
-
-func remapCodeSegmentAMD64(code []byte, size int) ([]byte, error) {
-	return remapCodeSegment(code, size)
-}
-
-func remapCodeSegmentARM64(code []byte, size int) ([]byte, error) {
-	return remapCodeSegment(code, size)
-}
-
-func remapCodeSegment(code []byte, size int) ([]byte, error) {
-	p, err := mremap(*(*unsafe.Pointer)(unsafe.Pointer(&code)), len(code), size, __MREMAP_MAYMOVE)
-	if err != nil {
-		return nil, err
-	}
-	return unsafe.Slice((*byte)(p), size), nil
-}
-
-//go:nosplit
-func mremap(oldAddr unsafe.Pointer, oldSize, newSize, flags int) (unsafe.Pointer, error) {
-	p, _, err := syscall.Syscall6(
-		syscall.SYS_MREMAP,
-		uintptr(oldAddr),
-		uintptr(oldSize),
-		uintptr(newSize),
-		uintptr(flags),
-		uintptr(0),
-		uintptr(0),
-	)
-	if err != 0 {
-		return nil, err
-	}
-	return unsafe.Pointer(p), nil
-}
diff --git a/internal/platform/mremap_other.go b/internal/platform/mremap_other.go
index ea3dbdfc..5be501c1 100644
--- a/internal/platform/mremap_other.go
+++ b/internal/platform/mremap_other.go
@@ -1,4 +1,4 @@
-//go:build !linux
+//go:build !(darwin || linux || freebsd)
 
 package platform
 
@@ -12,21 +12,6 @@ func remapCodeSegmentAMD64(code []byte, size int) ([]byte, error) {
 	return b, nil
 }
 
-// mustMunmapCodeSegment panics instead of returning an error to the
-// application.
-//
-// # Why panic?
-//
-// It is less disruptive to the application to leak the previous block if it
-// could be unmapped than to leak the new block and return an error.
-// Realistically, either scenarios are pretty hard to debug, so we panic. There
-// is less as the dominant runtime.GOOS (linux) doesn't use this anyway.
-func mustMunmapCodeSegment(code []byte) {
-	if err := munmapCodeSegment(code); err != nil {
-		panic(err)
-	}
-}
-
 func remapCodeSegmentARM64(code []byte, size int) ([]byte, error) {
 	b, err := mmapCodeSegmentARM64(size)
 	if err != nil {
diff --git a/internal/platform/mremap_unix.go b/internal/platform/mremap_unix.go
new file mode 100644
index 00000000..5017aaf7
--- /dev/null
+++ b/internal/platform/mremap_unix.go
@@ -0,0 +1,21 @@
+//go:build darwin || linux || freebsd
+
+package platform
+
+func remapCodeSegmentAMD64(code []byte, size int) ([]byte, error) {
+	return remapCodeSegment(code, size, mmapProtAMD64)
+}
+
+func remapCodeSegmentARM64(code []byte, size int) ([]byte, error) {
+	return remapCodeSegment(code, size, mmapProtARM64)
+}
+
+func remapCodeSegment(code []byte, size, prot int) ([]byte, error) {
+	b, err := mmapCodeSegment(size, prot)
+	if err != nil {
+		return nil, err
+	}
+	copy(b, code)
+	mustMunmapCodeSegment(code)
+	return b, nil
+}
diff --git a/internal/platform/platform.go b/internal/platform/platform.go
index 46f69991..2a0b78de 100644
--- a/internal/platform/platform.go
+++ b/internal/platform/platform.go
@@ -71,3 +71,17 @@ func MunmapCodeSegment(code []byte) error {
 	}
 	return munmapCodeSegment(code)
 }
+
+// mustMunmapCodeSegment panics instead of returning an error to the
+// application.
+//
+// # Why panic?
+//
+// It is less disruptive to the application to leak the previous block if it
+// could be unmapped than to leak the new block and return an error.
+// Realistically, either scenarios are pretty hard to debug, so we panic.
+func mustMunmapCodeSegment(code []byte) {
+	if err := munmapCodeSegment(code); err != nil {
+		panic(err)
+	}
+}
diff --git a/internal/sysfs/datasync_unsupported.go b/internal/sysfs/datasync_unsupported.go
index 91a2acf2..4261f565 100644
--- a/internal/sysfs/datasync_unsupported.go
+++ b/internal/sysfs/datasync_unsupported.go
@@ -9,5 +9,5 @@ import (
 
 func datasync(f *os.File) syscall.Errno {
 	// Attempt to sync everything, even if we only need to sync the data.
-	return sync(f)
+	return fsync(f)
 }
diff --git a/internal/sysfs/osfile.go b/internal/sysfs/osfile.go
index df39d16b..ab1cd2e1 100644
--- a/internal/sysfs/osfile.go
+++ b/internal/sysfs/osfile.go
@@ -209,7 +209,7 @@ func (f *osFile) Truncate(size int64) (errno syscall.Errno) {
 
 // Sync implements the same method as documented on api.File
 func (f *osFile) Sync() syscall.Errno {
-	return sync(f.file)
+	return fsync(f.file)
 }
 
 // Datasync implements the same method as documented on api.File
diff --git a/internal/sysfs/sync.go b/internal/sysfs/sync.go
index edb673c1..ba336c8d 100644
--- a/internal/sysfs/sync.go
+++ b/internal/sysfs/sync.go
@@ -9,6 +9,6 @@ import (
 	"github.com/tetratelabs/wazero/internal/platform"
 )
 
-func sync(f *os.File) syscall.Errno {
+func fsync(f *os.File) syscall.Errno {
 	return platform.UnwrapOSError(f.Sync())
 }
diff --git a/internal/sysfs/sync_windows.go b/internal/sysfs/sync_windows.go
index 6a30957a..e83dc304 100644
--- a/internal/sysfs/sync_windows.go
+++ b/internal/sysfs/sync_windows.go
@@ -7,7 +7,7 @@ import (
 	"github.com/tetratelabs/wazero/internal/platform"
 )
 
-func sync(f *os.File) syscall.Errno {
+func fsync(f *os.File) syscall.Errno {
 	errno := platform.UnwrapOSError(f.Sync())
 	// Coerce error performing stat on a directory to 0, as it won't work
 	// on Windows.


Test patch:
diff --git a/internal/engine/compiler/compiler_bench_test.go b/internal/engine/compiler/compiler_bench_test.go
index 88b76462..c5e455b0 100644
--- a/internal/engine/compiler/compiler_bench_test.go
+++ b/internal/engine/compiler/compiler_bench_test.go
@@ -6,6 +6,7 @@ import (
 	"testing"
 	"unsafe"
 
+	"github.com/tetratelabs/wazero/internal/testing/require"
 	"github.com/tetratelabs/wazero/internal/wasm"
 	"github.com/tetratelabs/wazero/internal/wazeroir"
 )
@@ -28,7 +29,7 @@ func BenchmarkCompiler_compileMemoryCopy(b *testing.B) {
 				compiler := newCompiler()
 				compiler.Init(&wasm.FunctionType{}, &wazeroir.CompilationResult{HasMemory: true}, false)
 				err := compiler.compilePreamble()
-				requireNoError(b, err)
+				require.NoError(b, err)
 
 				var destOffset, sourceOffset uint32
 				if !overlap {
@@ -38,17 +39,17 @@ func BenchmarkCompiler_compileMemoryCopy(b *testing.B) {
 				}
 
 				err = compiler.compileConstI32(operationPtr(wazeroir.NewOperationConstI32(destOffset)))
-				requireNoError(b, err)
+				require.NoError(b, err)
 				err = compiler.compileConstI32(operationPtr(wazeroir.NewOperationConstI32(sourceOffset)))
-				requireNoError(b, err)
+				require.NoError(b, err)
 				err = compiler.compileConstI32(operationPtr(wazeroir.NewOperationConstI32(size)))
-				requireNoError(b, err)
+				require.NoError(b, err)
 				err = compiler.compileMemoryCopy()
-				requireNoError(b, err)
+				require.NoError(b, err)
 				err = compiler.(compilerImpl).compileReturnFunction()
-				requireNoError(b, err)
+				require.NoError(b, err)
 				code, _, err := compiler.compile()
-				requireNoError(b, err)
+				require.NoError(b, err)
 
 				env.execBench(b, code)
 
@@ -71,57 +72,56 @@ func BenchmarkCompiler_compileMemoryFill(b *testing.B) {
 		b.Run(fmt.Sprintf("%v", size), func(b *testing.B) {
 			env := newCompilerEnvironment()
 
-			mem := env.memory()
-			testMem := make([]byte, len(mem))
-			for i := 0; i < len(mem); i++ {
-				mem[i] = byte(i)
-				testMem[i] = byte(i)
-			}
-
 			compiler := newCompiler()
 			compiler.Init(&wasm.FunctionType{}, &wazeroir.CompilationResult{HasMemory: true}, false)
 
 			var startOffset uint32 = 100
 			var value uint8 = 5
 
-			err := compiler.compileConstI32(operationPtr(wazeroir.NewOperationConstI32(startOffset)))
-			requireNoError(b, err)
+			err := compiler.compilePreamble()
+			require.NoError(b, err)
+
+			err = compiler.compileConstI32(operationPtr(wazeroir.NewOperationConstI32(startOffset)))
+			require.NoError(b, err)
 			err = compiler.compileConstI32(operationPtr(wazeroir.NewOperationConstI32(uint32(value))))
-			requireNoError(b, err)
+			require.NoError(b, err)
 			err = compiler.compileConstI32(operationPtr(wazeroir.NewOperationConstI32(size)))
-			requireNoError(b, err)
+			require.NoError(b, err)
 			err = compiler.compileMemoryFill()
-			requireNoError(b, err)
+			require.NoError(b, err)
 			err = compiler.(compilerImpl).compileReturnFunction()
-			requireNoError(b, err)
+			require.NoError(b, err)
 			code, _, err := compiler.compile()
-			requireNoError(b, err)
+			require.NoError(b, err)
+
+			mem := env.memory()
+			testMem := make([]byte, len(mem))
+			for i := 0; i < len(mem); i++ {
+				mem[i] = byte(i)
+				testMem[i] = byte(i)
+			}
 
 			env.execBench(b, code)
 
 			for i := startOffset; i < startOffset+size; i++ {
 				testMem[i] = value
 			}
-			if !bytes.Equal(mem, testMem) {
-				b.FailNow()
+
+			for i := 0; i < len(mem); i++ {
+				require.Equal(b, mem[i], testMem[i], "mem != %d at offset %d", value, i)
 			}
 		})
 	}
 }
 
 func (j *compilerEnv) execBench(b *testing.B, codeSegment []byte) {
+	executable := requireExecutable(codeSegment)
 	b.StartTimer()
 	for i := 0; i < b.N; i++ {
 		nativecall(
-			uintptr(unsafe.Pointer(&codeSegment[0])),
+			uintptr(unsafe.Pointer(&executable[0])),
 			j.ce, j.moduleInstance,
 		)
 	}
 	b.StopTimer()
 }
-
-func requireNoError(b *testing.B, err error) {
-	if err != nil {
-		b.Fatal(err)
-	}
-}
diff --git a/internal/engine/compiler/engine_bench_test.go b/internal/engine/compiler/engine_bench_test.go
index b7132d64..991a98ac 100644
--- a/internal/engine/compiler/engine_bench_test.go
+++ b/internal/engine/compiler/engine_bench_test.go
@@ -22,7 +22,9 @@ func BenchmarkCallEngine_builtinFunctionFunctionListener(b *testing.B) {
 			index: 0,
 			parent: &compiledModule{
 				source: &wasm.Module{
-					FunctionDefinitionSection: []wasm.FunctionDefinition{{}},
+					TypeSection:     []wasm.FunctionType{{}},
+					FunctionSection: []wasm.Index{0},
+					CodeSection:     []wasm.Code{{Body: []byte{wasm.OpcodeEnd}}},
 				},
 			},
 		},
diff --git a/internal/platform/mmap_linux_test.go b/internal/platform/mmap_linux_test.go
new file mode 100644
index 00000000..5f4a6b79
--- /dev/null
+++ b/internal/platform/mmap_linux_test.go
@@ -0,0 +1,23 @@
+package platform
+
+import (
+	"os"
+	"testing"
+
+	"github.com/tetratelabs/wazero/internal/testing/require"
+)
+
+func TestHugePageConfigs(t *testing.T) {
+	dirents, err := os.ReadDir("/sys/kernel/mm/hugepages/")
+	require.NoError(t, err)
+	require.Equal(t, len(dirents), len(hugePagesConfigs))
+
+	for _, hugePagesConfig := range hugePagesConfigs {
+		require.NotEqual(t, 0, hugePagesConfig.size)
+		require.NotEqual(t, 0, hugePagesConfig.flag)
+	}
+
+	for i := 1; i < len(hugePagesConfigs); i++ {
+		require.True(t, hugePagesConfigs[i-1].size > hugePagesConfigs[i].size)
+	}
+}