Repository: tetratelabs/wazero
Pull request #612 (2022-06-01 04:40:25)

Narrows compiler support to operating systems currently known to work
This narrows to what the `internal/platform` package supports, which is
currently bound by Go SDK source that include `Mprotect` or windows.

Ex. `zsyscall_linux_amd64.go` includes `Mprotect`, but
`zsyscall_freebsd_amd64.go` does not.

This should prevent errors like below, by allowing `wazero.NewRuntime()`
to properly fallback to the interpreter.

```
.../mmap.go:74:16: undefined: syscall.Mprotect
```

A later change will implement FreeBSD. This is just here to ensure users
don't crash on unexpected OS.

See #607

mathetake: I believe this is not enough as Go's compiler resolves all symbols in the package regardless of the actual compiler pass
```
$  GOOS=freebsd go build ./...
# github.com/tetratelabs/wazero/internal/platform
internal/platform/mmap.go:55:16: undefined: syscall.Mprotect
```
mathetake: need platform/unsupported.go to define symbols 
mathetake: (maybe I'm wrong though :D)
codefromthecrypt: cool I can fix platform also!

Patch:
diff --git a/.github/workflows/commit.yaml b/.github/workflows/commit.yaml
index 78ecff68..7e6e5246 100644
--- a/.github/workflows/commit.yaml
+++ b/.github/workflows/commit.yaml
@@ -129,7 +129,7 @@ jobs:
 
       - name: Set up QEMU
         if: ${{ matrix.arch != 'amd64' }}
-        uses: docker/setup-qemu-action@v1
+        uses: docker/setup-qemu-action@v2
         with:
           platforms: ${{ matrix.arch }}
 
diff --git a/config_supported.go b/config_supported.go
index 48201f4c..862423ba 100644
--- a/config_supported.go
+++ b/config_supported.go
@@ -1,4 +1,11 @@
-//go:build amd64 || arm64
+// Note: The build constraints here are about the compiler, which is more
+// narrow than the architectures supported by the assembler.
+//
+// Constraints here must match platform.IsSupported.
+//
+// Meanwhile, users who know their runtime.GOOS can operate with the compiler
+// may choose to use NewRuntimeConfigCompiler explicitly.
+//go:build (amd64 || arm64) && (darwin || linux || windows)
 
 package wazero
 
diff --git a/config_unsupported.go b/config_unsupported.go
index 161eff38..ed958785 100644
--- a/config_unsupported.go
+++ b/config_unsupported.go
@@ -1,4 +1,5 @@
-//go:build !amd64 && !arm64
+// This is the opposite constraint of config_supported.go
+//go:build !(amd64 || arm64) || !(darwin || linux || windows)
 
 package wazero
 
diff --git a/internal/engine/compiler/arch.go b/internal/engine/compiler/arch.go
index e216231c..ce6c69f4 100644
--- a/internal/engine/compiler/arch.go
+++ b/internal/engine/compiler/arch.go
@@ -1,13 +1,8 @@
 package compiler
 
-var (
-	// newArchContext returns a new archContext which is architecture-specific type to be embedded in callEngine.
-	// This must be initialized in init() function in architecture-specific arch_*.go file which is guarded by build tag.
-	newArchContext func() archContext
-)
-
-// IsSupported returns whether the compiler is supported on this architecture.
-const IsSupported = isSupported
+// newArchContext returns a new archContext which is architecture-specific type to be embedded in callEngine.
+// This must be initialized in init() function in architecture-specific arch_*.go file which is guarded by build tag.
+var newArchContext func() archContext
 
 // nativecall is used by callEngine.execWasmFunction and the entrypoint to enter the compiled native code.
 // codeSegment is the pointer to the initial instruction of the compiled native code.
diff --git a/internal/engine/compiler/arch_amd64.go b/internal/engine/compiler/arch_amd64.go
index c2ead961..1454e2e0 100644
--- a/internal/engine/compiler/arch_amd64.go
+++ b/internal/engine/compiler/arch_amd64.go
@@ -4,8 +4,6 @@ import (
 	"github.com/tetratelabs/wazero/internal/wazeroir"
 )
 
-const isSupported = true
-
 // init initializes variables for the amd64 architecture
 func init() {
 	newArchContext = newArchContextImpl
diff --git a/internal/engine/compiler/arch_arm64.go b/internal/engine/compiler/arch_arm64.go
index da2e317b..dc7e3547 100644
--- a/internal/engine/compiler/arch_arm64.go
+++ b/internal/engine/compiler/arch_arm64.go
@@ -6,8 +6,6 @@ import (
 	"github.com/tetratelabs/wazero/internal/wazeroir"
 )
 
-const isSupported = true
-
 // init initializes variables for the arm64 architecture
 func init() {
 	newArchContext = newArchContextImpl
@@ -17,7 +15,7 @@ func init() {
 type archContext struct {
 	// compilerCallReturnAddress holds the absolute return address for nativecall.
 	// The value is set whenever nativecall is executed and done in compiler_arm64.s
-	// Native code can return back to the ce.execWasmFunction's main loop back by
+	// Native code can return to the ce.execWasmFunction's main loop back by
 	// executing "ret" instruction with this value. See arm64Compiler.exit.
 	// Note: this is only used by Compiler code so mark this as nolint.
 	compilerCallReturnAddress uint64 //nolint
diff --git a/internal/platform/mmap.go b/internal/platform/mmap.go
index 6c8bf7f4..9bb8aebf 100644
--- a/internal/platform/mmap.go
+++ b/internal/platform/mmap.go
@@ -1,10 +1,9 @@
-//go:build !windows
+// This uses syscall.Mprotect. Go's SDK only supports this on darwin and linux.
+//go:build darwin || linux
 
 package platform
 
-import (
-	"syscall"
-)
+import "syscall"
 
 func munmapCodeSegment(code []byte) error {
 	return syscall.Munmap(code)
diff --git a/internal/platform/mmap_unsupported.go b/internal/platform/mmap_unsupported.go
new file mode 100644
index 00000000..e1c2868d
--- /dev/null
+++ b/internal/platform/mmap_unsupported.go
@@ -0,0 +1,22 @@
+//go:build !(darwin || linux || windows)
+
+package platform
+
+import (
+	"fmt"
+	"runtime"
+)
+
+var errUnsupported = fmt.Errorf("mmap unsupported on GOOS=%s. Use interpreter instead.", runtime.GOOS)
+
+func munmapCodeSegment(code []byte) error {
+	panic(errUnsupported)
+}
+
+func mmapCodeSegmentAMD64(code []byte) ([]byte, error) {
+	panic(errUnsupported)
+}
+
+func mmapCodeSegmentARM64(code []byte) ([]byte, error) {
+	panic(errUnsupported)
+}
diff --git a/internal/platform/mmap_windows.go b/internal/platform/mmap_windows.go
index 60752d59..ecb40dd7 100644
--- a/internal/platform/mmap_windows.go
+++ b/internal/platform/mmap_windows.go
@@ -1,3 +1,5 @@
+//go:build windows
+
 package platform
 
 import (
diff --git a/internal/platform/platform.go b/internal/platform/platform.go
index 289374dd..fda01a19 100644
--- a/internal/platform/platform.go
+++ b/internal/platform/platform.go
@@ -9,6 +9,23 @@ import (
 	"runtime"
 )
 
+// IsSupported is exported for tests and includes constraints here and also the assembler.
+func IsSupported() bool {
+	switch runtime.GOOS {
+	case "darwin", "windows", "linux":
+	default:
+		return false
+	}
+
+	switch runtime.GOARCH {
+	case "amd64", "arm64":
+	default:
+		return false
+	}
+
+	return true
+}
+
 // MmapCodeSegment copies the code into the executable region and returns the byte slice of the region.
 //
 // See https://man7.org/linux/man-pages/man2/mmap.2.html for mmap API and flags.


Test patch:
diff --git a/.github/workflows/spectest.yaml b/.github/workflows/spectest.yaml
index 9dceecfa..9b003c6d 100644
--- a/.github/workflows/spectest.yaml
+++ b/.github/workflows/spectest.yaml
@@ -85,7 +85,7 @@ jobs:
 
       - name: Set up QEMU
         if: ${{ matrix.arch != 'amd64' }}
-        uses: docker/setup-qemu-action@v1
+        uses: docker/setup-qemu-action@v2
         with:
           platforms: ${{ matrix.arch }}
 
diff --git a/internal/engine/compiler/engine_test.go b/internal/engine/compiler/engine_test.go
index aa5d6176..7e5f70cb 100644
--- a/internal/engine/compiler/engine_test.go
+++ b/internal/engine/compiler/engine_test.go
@@ -8,6 +8,7 @@ import (
 	"testing"
 	"unsafe"
 
+	"github.com/tetratelabs/wazero/internal/platform"
 	"github.com/tetratelabs/wazero/internal/testing/enginetest"
 	"github.com/tetratelabs/wazero/internal/testing/require"
 	"github.com/tetratelabs/wazero/internal/wasm"
@@ -175,7 +176,7 @@ func TestCompiler_ModuleEngine_Memory(t *testing.T) {
 
 // requireSupportedOSArch is duplicated also in the platform package to ensure no cyclic dependency.
 func requireSupportedOSArch(t *testing.T) {
-	if runtime.GOARCH != "amd64" && runtime.GOARCH != "arm64" {
+	if !platform.IsSupported() {
 		t.Skip()
 	}
 }
diff --git a/internal/integration_test/engine/adhoc_test.go b/internal/integration_test/engine/adhoc_test.go
index bf3b712d..00e5db44 100644
--- a/internal/integration_test/engine/adhoc_test.go
+++ b/internal/integration_test/engine/adhoc_test.go
@@ -11,7 +11,7 @@ import (
 
 	"github.com/tetratelabs/wazero"
 	"github.com/tetratelabs/wazero/api"
-	"github.com/tetratelabs/wazero/internal/engine/compiler"
+	"github.com/tetratelabs/wazero/internal/platform"
 	"github.com/tetratelabs/wazero/internal/testing/require"
 	"github.com/tetratelabs/wazero/internal/wasm"
 	"github.com/tetratelabs/wazero/sys"
@@ -47,7 +47,7 @@ var tests = map[string]func(t *testing.T, r wazero.Runtime){
 }
 
 func TestEngineCompiler(t *testing.T) {
-	if !compiler.IsSupported {
+	if !platform.IsSupported() {
 		t.Skip()
 	}
 	runAllTests(t, tests, wazero.NewRuntimeConfigCompiler())
diff --git a/internal/integration_test/engine/hammer_test.go b/internal/integration_test/engine/hammer_test.go
index c5c43fe3..f192fb58 100644
--- a/internal/integration_test/engine/hammer_test.go
+++ b/internal/integration_test/engine/hammer_test.go
@@ -6,7 +6,7 @@ import (
 
 	"github.com/tetratelabs/wazero"
 	"github.com/tetratelabs/wazero/api"
-	"github.com/tetratelabs/wazero/internal/engine/compiler"
+	"github.com/tetratelabs/wazero/internal/platform"
 	"github.com/tetratelabs/wazero/internal/testing/hammer"
 	"github.com/tetratelabs/wazero/internal/testing/require"
 	"github.com/tetratelabs/wazero/sys"
@@ -19,7 +19,7 @@ var hammers = map[string]func(t *testing.T, r wazero.Runtime){
 }
 
 func TestEngineCompiler_hammer(t *testing.T) {
-	if !compiler.IsSupported {
+	if !platform.IsSupported() {
 		t.Skip()
 	}
 	runAllTests(t, hammers, wazero.NewRuntimeConfigCompiler())
diff --git a/internal/platform/mmap_test.go b/internal/platform/mmap_test.go
index 156a6a66..6aeb7a6a 100644
--- a/internal/platform/mmap_test.go
+++ b/internal/platform/mmap_test.go
@@ -3,7 +3,6 @@ package platform
 import (
 	"crypto/rand"
 	"io"
-	"runtime"
 	"testing"
 
 	"github.com/tetratelabs/wazero/internal/testing/require"
@@ -12,7 +11,9 @@ import (
 var testCode, _ = io.ReadAll(io.LimitReader(rand.Reader, 8*1024))
 
 func Test_MmapCodeSegment(t *testing.T) {
-	requireSupportedOSArch(t)
+	if !IsSupported() {
+		t.Skip()
+	}
 
 	newCode, err := MmapCodeSegment(testCode)
 	require.NoError(t, err)
@@ -29,7 +30,9 @@ func Test_MmapCodeSegment(t *testing.T) {
 }
 
 func Test_MunmapCodeSegment(t *testing.T) {
-	requireSupportedOSArch(t)
+	if !IsSupported() {
+		t.Skip()
+	}
 
 	// Errors if never mapped
 	require.Error(t, MunmapCodeSegment(testCode))
@@ -48,10 +51,3 @@ func Test_MunmapCodeSegment(t *testing.T) {
 		require.EqualError(t, captured, "BUG: MunmapCodeSegment with zero length")
 	})
 }
-
-// requireSupportedOSArch is duplicated also in the compiler package to ensure no cyclic dependency.
-func requireSupportedOSArch(t *testing.T) {
-	if runtime.GOARCH != "amd64" && runtime.GOARCH != "arm64" {
-		t.Skip()
-	}
-}