Repository: tetratelabs/wazero
Pull request #324 (2022-03-03 12:36:34)

Improves error using sign-extension-ops features


Patch:
diff --git a/internal/wasm/binary/code.go b/internal/wasm/binary/code.go
index 41117a54..17ff1112 100644
--- a/internal/wasm/binary/code.go
+++ b/internal/wasm/binary/code.go
@@ -71,6 +71,9 @@ func decodeCode(r *bytes.Reader) (*wasm.Code, error) {
 		return nil, fmt.Errorf("read body: %w", err)
 	}
 
+	// TODO: Is there a way to efficiently break if FeatureSignExtensionOps=false and
+	// OpcodeI32Extend8S <= op && op <= OpcodeI64Extend32S?
+
 	if body[len(body)-1] != wasm.OpcodeEnd {
 		return nil, fmt.Errorf("expr not end with OpcodeEnd")
 	}
diff --git a/internal/wasm/features.go b/internal/wasm/features.go
index 3e74e204..ae449af8 100644
--- a/internal/wasm/features.go
+++ b/internal/wasm/features.go
@@ -19,6 +19,9 @@ const (
 	// FeatureMutableGlobal decides if parsing should succeed on internalwasm.GlobalType Mutable
 	// See https://github.com/WebAssembly/mutable-global
 	FeatureMutableGlobal Features = 1 << iota
+	// FeatureSignExtensionOps decides if parsing should succeed on internalwasm.GlobalType Mutable
+	// See https://github.com/WebAssembly/spec/blob/main/proposals/sign-extension-ops/Overview.md
+	FeatureSignExtensionOps Features = 1 << iota
 )
 
 // Set assigns the value for the given feature.
@@ -48,7 +51,11 @@ func (f Features) String() string {
 	case 0:
 		return ""
 	case FeatureMutableGlobal:
-		return "mutable-global" // match https://github.com/WebAssembly/mutable-global
+		// match https://github.com/WebAssembly/mutable-global
+		return "mutable-global"
+	case FeatureSignExtensionOps:
+		// match https://github.com/WebAssembly/spec/blob/main/proposals/sign-extension-ops/Overview.md
+		return "sign-extension-ops"
 	default:
 		return "undefined" // TODO: when there are multiple features join known ones on pipe (|)
 	}
diff --git a/internal/wasm/func_validation.go b/internal/wasm/func_validation.go
index a834c623..7b6502e8 100644
--- a/internal/wasm/func_validation.go
+++ b/internal/wasm/func_validation.go
@@ -814,6 +814,8 @@ func validateFunction(
 			// unreachable instruction is stack-polymorphic.
 			valueTypeStack.unreachable()
 		} else if op == OpcodeNop {
+		} else if OpcodeI32Extend8S <= op && op <= OpcodeI64Extend32S {
+			return fmt.Errorf("%s invalid as %s is not yet supported. See #66", InstructionName(op), FeatureSignExtensionOps)
 		} else {
 			return fmt.Errorf("invalid instruction 0x%x", op)
 		}
diff --git a/internal/wasm/instruction.go b/internal/wasm/instruction.go
index 0f4c54c0..de6ce584 100644
--- a/internal/wasm/instruction.go
+++ b/internal/wasm/instruction.go
@@ -214,6 +214,14 @@ const (
 	OpcodeI64ReinterpretF64 Opcode = 0xbd
 	OpcodeF32ReinterpretI32 Opcode = 0xbe
 	OpcodeF64ReinterpretI64 Opcode = 0xbf
+
+	// Below are toggled with FeatureSignExtensionOps
+
+	OpcodeI32Extend8S  Opcode = 0xC0
+	OpcodeI32Extend16S Opcode = 0xC1
+	OpcodeI64Extend8S  Opcode = 0xC2
+	OpcodeI64Extend16S Opcode = 0xC3
+	OpcodeI64Extend32S Opcode = 0xC4
 )
 
 var instructionNames = [256]string{
@@ -389,6 +397,13 @@ var instructionNames = [256]string{
 	OpcodeI64ReinterpretF64: "i64.reinterpret_f64",
 	OpcodeF32ReinterpretI32: "f32.reinterpret_i32",
 	OpcodeF64ReinterpretI64: "f64.reinterpret_i64",
+
+	// Below are toggled with FeatureSignExtensionOps
+	OpcodeI32Extend8S:  "i32.extend8_s",
+	OpcodeI32Extend16S: "i32.extend16_s",
+	OpcodeI64Extend8S:  "i64.extend8_s",
+	OpcodeI64Extend16S: "i64.extend16_s",
+	OpcodeI64Extend32S: "i64.extend32_s",
 }
 
 // InstructionName returns the instruction corresponding to this binary Opcode.
diff --git a/internal/wasm/store.go b/internal/wasm/store.go
index 60d22062..7a9d8eab 100644
--- a/internal/wasm/store.go
+++ b/internal/wasm/store.go
@@ -790,11 +790,6 @@ func executeConstExpression(globals []*GlobalInstance, expr *ConstantExpression)
 	return
 }
 
-// Only used in spectests.
-func (s *Store) AliasModuleInstance(src, dst string) {
-	s.moduleInstances[dst] = s.moduleInstances[src]
-}
-
 func (s *Store) getTypeInstances(ts []*FunctionType) ([]*TypeInstance, error) {
 	ret := make([]*TypeInstance, len(ts))
 	for i, t := range ts {


Test patch:
diff --git a/internal/wasm/features_test.go b/internal/wasm/features_test.go
index 12d62223..fca50ba5 100644
--- a/internal/wasm/features_test.go
+++ b/internal/wasm/features_test.go
@@ -58,6 +58,7 @@ func TestFeatures_String(t *testing.T) {
 	}{
 		{name: "none", feature: 0, expected: ""},
 		{name: "mutable-global", feature: FeatureMutableGlobal, expected: "mutable-global"},
+		{name: "sign-extension-ops", feature: FeatureSignExtensionOps, expected: "sign-extension-ops"},
 		{name: "undefined", feature: 1 << 63, expected: "undefined"},
 	}
 
@@ -77,6 +78,7 @@ func TestFeatures_Require(t *testing.T) {
 	}{
 		{name: "none", feature: 0, expectedErr: "feature mutable-global is disabled"},
 		{name: "mutable-global", feature: FeatureMutableGlobal},
+		{name: "sign-extension-ops", feature: FeatureSignExtensionOps, expectedErr: "feature mutable-global is disabled"},
 		{name: "undefined", feature: 1 << 63, expectedErr: "feature mutable-global is disabled"},
 	}
 
diff --git a/internal/wasm/func_validation_test.go b/internal/wasm/func_validation_test.go
index 5160d1d4..6773e5f2 100644
--- a/internal/wasm/func_validation_test.go
+++ b/internal/wasm/func_validation_test.go
@@ -36,3 +36,41 @@ func TestValidateFunction_valueStackLimit(t *testing.T) {
 		require.Equal(t, expMsg, err.Error())
 	})
 }
+
+func TestValidateFunction_FeatureSignExtensionOps(t *testing.T) {
+	// TODO: actually support, guarded by FeatureSignExtensionOps flag which defaults to false #66
+	tests := []struct {
+		input       Opcode
+		expectedErr string
+	}{
+		{
+			input:       OpcodeI32Extend8S,
+			expectedErr: "i32.extend8_s invalid as sign-extension-ops is not yet supported. See #66",
+		},
+		{
+			input:       OpcodeI32Extend16S,
+			expectedErr: "i32.extend16_s invalid as sign-extension-ops is not yet supported. See #66",
+		},
+		{
+			input:       OpcodeI64Extend8S,
+			expectedErr: "i64.extend8_s invalid as sign-extension-ops is not yet supported. See #66",
+		},
+		{
+			input:       OpcodeI64Extend16S,
+			expectedErr: "i64.extend16_s invalid as sign-extension-ops is not yet supported. See #66",
+		},
+		{
+			input:       OpcodeI64Extend32S,
+			expectedErr: "i64.extend32_s invalid as sign-extension-ops is not yet supported. See #66",
+		},
+	}
+
+	for _, tt := range tests {
+		tc := tt
+
+		t.Run(InstructionName(tc.input), func(t *testing.T) {
+			err := validateFunction(&FunctionType{}, []byte{tc.input}, nil, nil, nil, nil, nil, nil, 0)
+			require.EqualError(t, err, tc.expectedErr)
+		})
+	}
+}
diff --git a/tests/spectest/spec_test.go b/tests/spectest/spec_test.go
index 7dc12b36..04a96ddf 100644
--- a/tests/spectest/spec_test.go
+++ b/tests/spectest/spec_test.go
@@ -119,6 +119,16 @@ func (c command) String() string {
 	return "{" + msg + "}"
 }
 
+func (c command) moduleName(lastInstantiatedModuleName string) string {
+	if c.Action.Module != "" {
+		// If the module name is specified for the command, it almost always starts with $, but
+		// it might be aliased as the one without $. To remove the necessity for "register"
+		// command, we always treat module name without $
+		return strings.TrimPrefix(c.Action.Module, "$")
+	}
+	return lastInstantiatedModuleName
+}
+
 func (c command) getAssertReturnArgs() []uint64 {
 	var args []uint64
 	for _, arg := range c.Action.Args {
@@ -301,8 +311,8 @@ func runTest(t *testing.T, newEngine func() wasm.Engine) {
 			store := wasm.NewStore(context.Background(), newEngine(), wasm.Features20191205)
 			addSpectestModule(t, store)
 
-			var lastInstanceName string
-			for _, c := range base.Commands {
+			var lastInstantiatedModuleName string
+			for i, c := range base.Commands {
 				t.Run(fmt.Sprintf("%s/line:%d", c.CommandType, c.Line), func(t *testing.T) {
 					msg := fmt.Sprintf("%s:%d %s", wastName, c.Line, c.CommandType)
 					switch c.CommandType {
@@ -313,23 +323,23 @@ func runTest(t *testing.T, newEngine func() wasm.Engine) {
 						mod, err := binary.DecodeModule(buf, wasm.Features20191205)
 						require.NoError(t, err, msg)
 
-						lastInstanceName = c.Name
-						if lastInstanceName == "" {
-							lastInstanceName = c.Filename
+						moduleName := c.Name
+						if moduleName == "" { // When "(module ...) directive doesn't have name.
+							if i+1 < len(base.Commands) && base.Commands[i+1].CommandType == "register" {
+								// If the next command is "(register ...)", we use that name for this module.
+								moduleName = base.Commands[i+1].As
+							} else {
+								// Otherwise, use the file name as the name.
+								moduleName = c.Filename
+							}
 						}
-						_, err = store.Instantiate(mod, lastInstanceName)
+						moduleName = strings.TrimPrefix(moduleName, "$")
+						_, err = store.Instantiate(mod, moduleName)
+						lastInstantiatedModuleName = moduleName
 						require.NoError(t, err)
 					case "register":
-						name := lastInstanceName
-						if c.Name != "" {
-							name = c.Name
-						}
-						store.AliasModuleInstance(name, c.As)
 					case "assert_return", "action":
-						moduleName := lastInstanceName
-						if c.Action.Module != "" {
-							moduleName = c.Action.Module
-						}
+						moduleName := c.moduleName(lastInstantiatedModuleName)
 						switch c.Action.ActionType {
 						case "invoke":
 							args, exps := c.getAssertReturnArgsExps()
@@ -380,10 +390,7 @@ func runTest(t *testing.T, newEngine func() wasm.Engine) {
 						require.NoError(t, err, msg)
 						requireInstantiationError(t, store, buf, msg)
 					case "assert_trap":
-						moduleName := lastInstanceName
-						if c.Action.Module != "" {
-							moduleName = c.Action.Module
-						}
+						moduleName := c.moduleName(lastInstantiatedModuleName)
 						switch c.Action.ActionType {
 						case "invoke":
 							args := c.getAssertReturnArgs()
@@ -405,10 +412,7 @@ func runTest(t *testing.T, newEngine func() wasm.Engine) {
 						require.NoError(t, err, msg)
 						requireInstantiationError(t, store, buf, msg)
 					case "assert_exhaustion":
-						moduleName := lastInstanceName
-						if c.Action.Module != "" {
-							moduleName = c.Action.Module
-						}
+						moduleName := c.moduleName(lastInstantiatedModuleName)
 						switch c.Action.ActionType {
 						case "invoke":
 							args := c.getAssertReturnArgs()