From c07fd71e51fc76b27a368db77717e017e3c2de3d Mon Sep 17 00:00:00 2001 From: Randy Reddig Date: Sun, 19 Jan 2025 14:54:47 -0800 Subject: [PATCH 1/7] Makefile, testdata, tests: set up test for #284 --- Makefile | 4 ++ testdata/issues/issue284.wit | 9 ++++ testdata/issues/issue284.wit.json | 51 ++++++++++++++++++++ testdata/issues/issue284.wit.json.golden.wit | 9 ++++ tests/generated/issues/issue284/i/i.wit.go | 13 +++++ tests/generated/issues/issue284/w/w.wit.go | 4 ++ tests/issues/issue284/issue284_test.go | 51 ++++++++++++++++++++ tests/tests.go | 1 + 8 files changed, 142 insertions(+) create mode 100644 testdata/issues/issue284.wit create mode 100644 testdata/issues/issue284.wit.json create mode 100644 testdata/issues/issue284.wit.json.golden.wit create mode 100755 tests/generated/issues/issue284/i/i.wit.go create mode 100755 tests/generated/issues/issue284/w/w.wit.go create mode 100644 tests/issues/issue284/issue284_test.go diff --git a/Makefile b/Makefile index 954b76ef..5b0f0bb2 100644 --- a/Makefile +++ b/Makefile @@ -53,3 +53,7 @@ test: tinygo test -target=wasip1 $(GOTESTARGS) $(GOTESTMODULES) tinygo test -target=wasip2 $(GOTESTARGS) $(GOTESTMODULES) tinygo test -target=wasip2 $(GOTESTARGS) ./tests/... + +.PHONY: tests +tests: + tinygo test -target=wasip2 $(GOTESTARGS) ./tests/... diff --git a/testdata/issues/issue284.wit b/testdata/issues/issue284.wit new file mode 100644 index 00000000..aa6d6588 --- /dev/null +++ b/testdata/issues/issue284.wit @@ -0,0 +1,9 @@ +package issues:issue284; + +interface i { + type bool-s8-result = result; +} + +world w { + import i; +} diff --git a/testdata/issues/issue284.wit.json b/testdata/issues/issue284.wit.json new file mode 100644 index 00000000..ce539553 --- /dev/null +++ b/testdata/issues/issue284.wit.json @@ -0,0 +1,51 @@ +{ + "worlds": [ + { + "name": "w", + "imports": { + "interface-0": { + "interface": { + "id": 0 + } + } + }, + "exports": {}, + "package": 0 + } + ], + "interfaces": [ + { + "name": "i", + "types": { + "bool-s8-result": 0 + }, + "functions": {}, + "package": 0 + } + ], + "types": [ + { + "name": "bool-s8-result", + "kind": { + "result": { + "ok": "bool", + "err": "s8" + } + }, + "owner": { + "interface": 0 + } + } + ], + "packages": [ + { + "name": "issues:issue284", + "interfaces": { + "i": 0 + }, + "worlds": { + "w": 0 + } + } + ] +} \ No newline at end of file diff --git a/testdata/issues/issue284.wit.json.golden.wit b/testdata/issues/issue284.wit.json.golden.wit new file mode 100644 index 00000000..aa6d6588 --- /dev/null +++ b/testdata/issues/issue284.wit.json.golden.wit @@ -0,0 +1,9 @@ +package issues:issue284; + +interface i { + type bool-s8-result = result; +} + +world w { + import i; +} diff --git a/tests/generated/issues/issue284/i/i.wit.go b/tests/generated/issues/issue284/i/i.wit.go new file mode 100755 index 00000000..8c761f2f --- /dev/null +++ b/tests/generated/issues/issue284/i/i.wit.go @@ -0,0 +1,13 @@ +// Code generated by wit-bindgen-go. DO NOT EDIT. + +// Package i represents the imported interface "issues:issue284/i". +package i + +import ( + "go.bytecodealliance.org/cm" +) + +// BoolS8Result represents the result "issues:issue284/i#bool-s8-result". +// +// type bool-s8-result = result +type BoolS8Result cm.Result[bool, bool, int8] diff --git a/tests/generated/issues/issue284/w/w.wit.go b/tests/generated/issues/issue284/w/w.wit.go new file mode 100755 index 00000000..8d8f46ab --- /dev/null +++ b/tests/generated/issues/issue284/w/w.wit.go @@ -0,0 +1,4 @@ +// Code generated by wit-bindgen-go. DO NOT EDIT. + +// Package w represents the world "issues:issue284/w". +package w diff --git a/tests/issues/issue284/issue284_test.go b/tests/issues/issue284/issue284_test.go new file mode 100644 index 00000000..5ab63d56 --- /dev/null +++ b/tests/issues/issue284/issue284_test.go @@ -0,0 +1,51 @@ +package issue284 + +import ( + "fmt" + "testing" + + "tests/generated/issues/issue284/i" + + "go.bytecodealliance.org/cm" +) + +func TestIssue284(t *testing.T) { + tests := []struct { + isErr bool + ok bool + err int8 + }{ + {false, false, 0}, + {false, true, 0}, + {true, false, 0}, + {true, false, 1}, + {true, false, 5}, + {true, false, 126}, + {true, false, 127}, + } + for _, tt := range tests { + if !tt.isErr { + t.Run(fmt.Sprintf("cm.OK[i.BoolS8Result](%t)", tt.ok), func(t *testing.T) { + r := cm.OK[i.BoolS8Result](tt.ok) + ok, _, isErr := r.Result() + if isErr != tt.isErr { + t.Errorf("isErr == %t, expected %t", isErr, tt.isErr) + } + if ok != tt.ok { + t.Errorf("ok == %t, expected %t", ok, tt.ok) + } + }) + } else { + t.Run(fmt.Sprintf("cm.Err[i.BoolS8Result](%d)", tt.err), func(t *testing.T) { + r := cm.Err[i.BoolS8Result](tt.err) + _, err, isErr := r.Result() + if isErr != tt.isErr { + t.Errorf("isErr == %t, expected %t", isErr, tt.isErr) + } + if err != tt.err { + t.Errorf("err == %d, expected %d", err, tt.err) + } + }) + } + } +} diff --git a/tests/tests.go b/tests/tests.go index f6253207..7036058b 100644 --- a/tests/tests.go +++ b/tests/tests.go @@ -7,3 +7,4 @@ package tests //go:generate rm -rf ./generated/* //go:generate mkdir -p ./generated //go:generate go run go.bytecodealliance.org/cmd/wit-bindgen-go generate --versioned -o ./generated ../testdata/wasi/cli.wit.json +//go:generate go run go.bytecodealliance.org/cmd/wit-bindgen-go generate --versioned -o ./generated ../testdata/issues/issue284.wit From a1d6d8802e4577c9f1d9595e8e5f1582ed2065a8 Mon Sep 17 00:00:00 2001 From: Randy Reddig Date: Sun, 19 Jan 2025 15:53:29 -0800 Subject: [PATCH 2/7] wit/bindgen: use type aliases for variant shapes --- wit/bindgen/generator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wit/bindgen/generator.go b/wit/bindgen/generator.go index 220dcad3..553b42f4 100644 --- a/wit/bindgen/generator.go +++ b/wit/bindgen/generator.go @@ -1018,7 +1018,7 @@ func (g *generator) typeShape(file *gen.File, dir wit.Direction, t wit.Type) str } func (g *generator) typeDefShape(file *gen.File, dir wit.Direction, t *wit.TypeDef) string { - switch kind := t.Kind.(type) { + switch kind := t.Root().Kind.(type) { case wit.Type: return g.typeShape(file, dir, kind) case *wit.Variant: From c1473acbb8a77fc0649b89b6ffb9ec7b55b1f596 Mon Sep 17 00:00:00 2001 From: Randy Reddig Date: Sun, 19 Jan 2025 15:53:57 -0800 Subject: [PATCH 3/7] wit/bindgen: demote bool as a variant or result GC shape This fixes #284. --- wit/bindgen/abi.go | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/wit/bindgen/abi.go b/wit/bindgen/abi.go index 5b4f4d37..85fd28f4 100644 --- a/wit/bindgen/abi.go +++ b/wit/bindgen/abi.go @@ -6,7 +6,7 @@ import ( "go.bytecodealliance.org/wit" ) -// variantShape returns the type with the greatest size. +// variantShape returns the type with the greatest size that is not a bool. // If there are multiple types with the same size, it returns // the first type that contains a pointer. func variantShape(types []wit.Type) wit.Type { @@ -19,6 +19,12 @@ func variantShape(types []wit.Type) wit.Type { return -1 case a.Size() < b.Size(): return 1 + case !isBool(a) && isBool(b): + // bool cannot be used as variant shape + // See https://github.com/bytecodealliance/go-modules/issues/284 + return -1 + case isBool(a) && !isBool(b): + return 1 case wit.HasPointer(a) && !wit.HasPointer(b): return -1 case !wit.HasPointer(a) && wit.HasPointer(b): @@ -30,6 +36,16 @@ func variantShape(types []wit.Type) wit.Type { return types[0] } +func isBool(t wit.TypeDefKind) bool { + switch t := t.(type) { + case wit.Bool: + return true + case *wit.TypeDef: + return isBool(t.Root().Kind) + } + return false +} + // variantAlign returns the type with the largest alignment. func variantAlign(types []wit.Type) wit.Type { if len(types) == 0 { From fb0b3ac11527826cb4cabe503ccab1d3ed8e3e99 Mon Sep 17 00:00:00 2001 From: Randy Reddig Date: Sun, 19 Jan 2025 15:54:11 -0800 Subject: [PATCH 4/7] tests/generated: regenerate --- tests/generated/issues/issue284/i/i.wit.go | 2 +- tests/generated/wasi/sockets/v0.2.0/tcp/abi.go | 2 +- tests/generated/wasi/sockets/v0.2.0/tcp/tcp.wasm.go | 2 +- tests/generated/wasi/sockets/v0.2.0/tcp/tcp.wit.go | 2 +- tests/generated/wasi/sockets/v0.2.0/udp/abi.go | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/generated/issues/issue284/i/i.wit.go b/tests/generated/issues/issue284/i/i.wit.go index 8c761f2f..38ba1eb2 100755 --- a/tests/generated/issues/issue284/i/i.wit.go +++ b/tests/generated/issues/issue284/i/i.wit.go @@ -10,4 +10,4 @@ import ( // BoolS8Result represents the result "issues:issue284/i#bool-s8-result". // // type bool-s8-result = result -type BoolS8Result cm.Result[bool, bool, int8] +type BoolS8Result cm.Result[int8, bool, int8] diff --git a/tests/generated/wasi/sockets/v0.2.0/tcp/abi.go b/tests/generated/wasi/sockets/v0.2.0/tcp/abi.go index 2788dbeb..eb335d00 100755 --- a/tests/generated/wasi/sockets/v0.2.0/tcp/abi.go +++ b/tests/generated/wasi/sockets/v0.2.0/tcp/abi.go @@ -23,7 +23,7 @@ type TupleInputStreamOutputStreamShape struct { // IPSocketAddressShape is used for storage in variant or result types. type IPSocketAddressShape struct { _ cm.HostLayout - shape [unsafe.Sizeof(network.IPSocketAddress{})]byte + shape [unsafe.Sizeof(IPSocketAddress{})]byte } func lower_IPv4Address(v network.IPv4Address) (f0 uint32, f1 uint32, f2 uint32, f3 uint32) { diff --git a/tests/generated/wasi/sockets/v0.2.0/tcp/tcp.wasm.go b/tests/generated/wasi/sockets/v0.2.0/tcp/tcp.wasm.go index 56247525..d6e56c26 100755 --- a/tests/generated/wasi/sockets/v0.2.0/tcp/tcp.wasm.go +++ b/tests/generated/wasi/sockets/v0.2.0/tcp/tcp.wasm.go @@ -46,7 +46,7 @@ func wasmimport_TCPSocketKeepAliveCount(self0 uint32, result *cm.Result[uint32, //go:wasmimport wasi:sockets/tcp@0.2.0 [method]tcp-socket.keep-alive-enabled //go:noescape -func wasmimport_TCPSocketKeepAliveEnabled(self0 uint32, result *cm.Result[bool, bool, ErrorCode]) +func wasmimport_TCPSocketKeepAliveEnabled(self0 uint32, result *cm.Result[ErrorCode, bool, ErrorCode]) //go:wasmimport wasi:sockets/tcp@0.2.0 [method]tcp-socket.keep-alive-idle-time //go:noescape diff --git a/tests/generated/wasi/sockets/v0.2.0/tcp/tcp.wit.go b/tests/generated/wasi/sockets/v0.2.0/tcp/tcp.wit.go index ff669440..f43fd44f 100755 --- a/tests/generated/wasi/sockets/v0.2.0/tcp/tcp.wit.go +++ b/tests/generated/wasi/sockets/v0.2.0/tcp/tcp.wit.go @@ -285,7 +285,7 @@ func (self TCPSocket) KeepAliveCount() (result cm.Result[uint32, uint32, ErrorCo // keep-alive-enabled: func() -> result // //go:nosplit -func (self TCPSocket) KeepAliveEnabled() (result cm.Result[bool, bool, ErrorCode]) { +func (self TCPSocket) KeepAliveEnabled() (result cm.Result[ErrorCode, bool, ErrorCode]) { self0 := cm.Reinterpret[uint32](self) wasmimport_TCPSocketKeepAliveEnabled((uint32)(self0), &result) return diff --git a/tests/generated/wasi/sockets/v0.2.0/udp/abi.go b/tests/generated/wasi/sockets/v0.2.0/udp/abi.go index 7e44e2f2..3dfaa27f 100755 --- a/tests/generated/wasi/sockets/v0.2.0/udp/abi.go +++ b/tests/generated/wasi/sockets/v0.2.0/udp/abi.go @@ -11,7 +11,7 @@ import ( // IPSocketAddressShape is used for storage in variant or result types. type IPSocketAddressShape struct { _ cm.HostLayout - shape [unsafe.Sizeof(network.IPSocketAddress{})]byte + shape [unsafe.Sizeof(IPSocketAddress{})]byte } func lower_IPv4Address(v network.IPv4Address) (f0 uint32, f1 uint32, f2 uint32, f3 uint32) { From 80a57c7b3ccf1cc8823462ae7b4b7ee914269b0b Mon Sep 17 00:00:00 2001 From: Randy Reddig Date: Sun, 19 Jan 2025 16:23:24 -0800 Subject: [PATCH 5/7] cm: add tests for #284 --- cm/result_test.go | 96 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+) diff --git a/cm/result_test.go b/cm/result_test.go index 759c03ea..ee716bc6 100644 --- a/cm/result_test.go +++ b/cm/result_test.go @@ -1,6 +1,7 @@ package cm import ( + "fmt" "runtime" "testing" "unsafe" @@ -238,3 +239,98 @@ func TestIssue95BoolInt64(t *testing.T) { t.Errorf("*res.OK(): %v, expected %v", got, want) } } + +func TestIssue284(t *testing.T) { + // Using int8 instead of bool for shape works on Go and TinyGo. + // See https://github.com/bytecodealliance/go-modules/issues/284 + type BoolS8Result Result[int8, bool, int8] + + tests := []struct { + isErr bool + ok bool + err int8 + }{ + {false, false, 0}, + {false, true, 0}, + {true, false, 0}, + {true, false, 1}, + {true, false, 5}, + {true, false, 126}, + {true, false, 127}, + } + for _, tt := range tests { + if !tt.isErr { + t.Run(fmt.Sprintf("OK[BoolS8Result](%t)", tt.ok), func(t *testing.T) { + r := OK[BoolS8Result](tt.ok) + ok, _, isErr := r.Result() + if isErr != tt.isErr { + t.Errorf("isErr == %t, expected %t", isErr, tt.isErr) + } + if ok != tt.ok { + t.Errorf("ok == %t, expected %t", ok, tt.ok) + } + }) + } else { + t.Run(fmt.Sprintf("Err[BoolS8Result](%d)", tt.err), func(t *testing.T) { + r := Err[BoolS8Result](tt.err) + _, err, isErr := r.Result() + if isErr != tt.isErr { + t.Errorf("isErr == %t, expected %t", isErr, tt.isErr) + } + if err != tt.err { + t.Errorf("err == %d, expected %d", err, tt.err) + } + }) + } + } +} + +func TestIssue284NotTinyGo(t *testing.T) { + if runtime.Compiler == "tinygo" { + return + } + + // Using bool as result shape changes how [OK] returns the result by value. + // This works on Go, but breaks on TinyGo / LLVM. + // See https://github.com/bytecodealliance/go-modules/issues/284 + type BoolS8Result Result[bool, bool, int8] + + tests := []struct { + isErr bool + ok bool + err int8 + }{ + {false, false, 0}, + {false, true, 0}, + {true, false, 0}, + {true, false, 1}, + {true, false, 5}, + {true, false, 126}, + {true, false, 127}, + } + for _, tt := range tests { + if !tt.isErr { + t.Run(fmt.Sprintf("OK[BoolS8Result](%t)", tt.ok), func(t *testing.T) { + r := OK[BoolS8Result](tt.ok) + ok, _, isErr := r.Result() + if isErr != tt.isErr { + t.Errorf("isErr == %t, expected %t", isErr, tt.isErr) + } + if ok != tt.ok { + t.Errorf("ok == %t, expected %t", ok, tt.ok) + } + }) + } else { + t.Run(fmt.Sprintf("Err[BoolS8Result](%d)", tt.err), func(t *testing.T) { + r := Err[BoolS8Result](tt.err) + _, err, isErr := r.Result() + if isErr != tt.isErr { + t.Errorf("isErr == %t, expected %t", isErr, tt.isErr) + } + if err != tt.err { + t.Errorf("err == %d, expected %d", err, tt.err) + } + }) + } + } +} From b53357335e0c7882c59448be04761108ff243feb Mon Sep 17 00:00:00 2001 From: Randy Reddig Date: Sun, 19 Jan 2025 16:26:36 -0800 Subject: [PATCH 6/7] CHANGELOG: add line for #284 --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index dbd87a60..1509f5c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), - Breaking: generated `*.wasm.go` files will now have correct WIT kebab-case base name. Interfaces or worlds with `-` in their name will require removal of the previous `*.wasm.go` files. - Dropped support for TinyGo v0.32.0. +### Fixed + +- [#284](https://github.com/bytecodealliance/go-modules/issues/284): do not use `bool` for `variant` or `result` GC shapes. TinyGo returns `result` and `variant` values with `bool` as 0 or 1, which breaks the memory representation of tagged unions (variants). + ## [v0.5.0] — 2024-12-14 ### Changed From 8cf144f167caeabeeeddd766a8f9014981667d9f Mon Sep 17 00:00:00 2001 From: Randy Reddig Date: Mon, 27 Jan 2025 08:13:49 -0800 Subject: [PATCH 7/7] testdata, tests: remove test case for #284 This is tested in package cm. --- testdata/issues/issue284.wit | 9 ---- testdata/issues/issue284.wit.json | 51 -------------------- testdata/issues/issue284.wit.json.golden.wit | 9 ---- tests/issues/issue284/issue284_test.go | 51 -------------------- tests/tests.go | 1 - 5 files changed, 121 deletions(-) delete mode 100644 testdata/issues/issue284.wit delete mode 100644 testdata/issues/issue284.wit.json delete mode 100644 testdata/issues/issue284.wit.json.golden.wit delete mode 100644 tests/issues/issue284/issue284_test.go diff --git a/testdata/issues/issue284.wit b/testdata/issues/issue284.wit deleted file mode 100644 index aa6d6588..00000000 --- a/testdata/issues/issue284.wit +++ /dev/null @@ -1,9 +0,0 @@ -package issues:issue284; - -interface i { - type bool-s8-result = result; -} - -world w { - import i; -} diff --git a/testdata/issues/issue284.wit.json b/testdata/issues/issue284.wit.json deleted file mode 100644 index ce539553..00000000 --- a/testdata/issues/issue284.wit.json +++ /dev/null @@ -1,51 +0,0 @@ -{ - "worlds": [ - { - "name": "w", - "imports": { - "interface-0": { - "interface": { - "id": 0 - } - } - }, - "exports": {}, - "package": 0 - } - ], - "interfaces": [ - { - "name": "i", - "types": { - "bool-s8-result": 0 - }, - "functions": {}, - "package": 0 - } - ], - "types": [ - { - "name": "bool-s8-result", - "kind": { - "result": { - "ok": "bool", - "err": "s8" - } - }, - "owner": { - "interface": 0 - } - } - ], - "packages": [ - { - "name": "issues:issue284", - "interfaces": { - "i": 0 - }, - "worlds": { - "w": 0 - } - } - ] -} \ No newline at end of file diff --git a/testdata/issues/issue284.wit.json.golden.wit b/testdata/issues/issue284.wit.json.golden.wit deleted file mode 100644 index aa6d6588..00000000 --- a/testdata/issues/issue284.wit.json.golden.wit +++ /dev/null @@ -1,9 +0,0 @@ -package issues:issue284; - -interface i { - type bool-s8-result = result; -} - -world w { - import i; -} diff --git a/tests/issues/issue284/issue284_test.go b/tests/issues/issue284/issue284_test.go deleted file mode 100644 index 5ab63d56..00000000 --- a/tests/issues/issue284/issue284_test.go +++ /dev/null @@ -1,51 +0,0 @@ -package issue284 - -import ( - "fmt" - "testing" - - "tests/generated/issues/issue284/i" - - "go.bytecodealliance.org/cm" -) - -func TestIssue284(t *testing.T) { - tests := []struct { - isErr bool - ok bool - err int8 - }{ - {false, false, 0}, - {false, true, 0}, - {true, false, 0}, - {true, false, 1}, - {true, false, 5}, - {true, false, 126}, - {true, false, 127}, - } - for _, tt := range tests { - if !tt.isErr { - t.Run(fmt.Sprintf("cm.OK[i.BoolS8Result](%t)", tt.ok), func(t *testing.T) { - r := cm.OK[i.BoolS8Result](tt.ok) - ok, _, isErr := r.Result() - if isErr != tt.isErr { - t.Errorf("isErr == %t, expected %t", isErr, tt.isErr) - } - if ok != tt.ok { - t.Errorf("ok == %t, expected %t", ok, tt.ok) - } - }) - } else { - t.Run(fmt.Sprintf("cm.Err[i.BoolS8Result](%d)", tt.err), func(t *testing.T) { - r := cm.Err[i.BoolS8Result](tt.err) - _, err, isErr := r.Result() - if isErr != tt.isErr { - t.Errorf("isErr == %t, expected %t", isErr, tt.isErr) - } - if err != tt.err { - t.Errorf("err == %d, expected %d", err, tt.err) - } - }) - } - } -} diff --git a/tests/tests.go b/tests/tests.go index 7036058b..f6253207 100644 --- a/tests/tests.go +++ b/tests/tests.go @@ -7,4 +7,3 @@ package tests //go:generate rm -rf ./generated/* //go:generate mkdir -p ./generated //go:generate go run go.bytecodealliance.org/cmd/wit-bindgen-go generate --versioned -o ./generated ../testdata/wasi/cli.wit.json -//go:generate go run go.bytecodealliance.org/cmd/wit-bindgen-go generate --versioned -o ./generated ../testdata/issues/issue284.wit