Repository: tetratelabs/wazero
Pull request #744 (2022-08-12 04:40:48)

compiler: drop higher bits on table index
as of #743 #742 , store into the value stack only stores lower 32-bits,
so we have to drop the higher 32-bits.

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>


Patch:
diff --git a/RATIONALE.md b/RATIONALE.md
index fd3852dd..1c2ee273 100644
--- a/RATIONALE.md
+++ b/RATIONALE.md
@@ -441,6 +441,36 @@ See
 * https://github.com/golang/go/blob/go1.19rc2/src/syscall/fs_js.go#L324
 * https://github.com/WebAssembly/wasi-libc/pull/214#issue-673090117
 
+### Why ignore the error returned by io.Reader when n > 1?
+
+Per https://pkg.go.dev/io#Reader, if we receive an error, any bytes read should
+be processed first. At the syscall abstraction (`fd_read`), the caller is the
+processor, so we can't process the bytes inline and also return the error (as
+`EIO`).
+
+Let's assume we want to return the bytes read on error to the caller. This
+implies we at least temporarily ignore the error alongside them. The choice
+remaining is whether to persist the error returned with the read until a
+possible next call, or ignore the error.
+
+If we persist an error returned, it would be coupled to a file descriptor, but
+effectively it is boolean as this case coerces to `EIO`. If we track a "last
+error" on a file descriptor, it could be complicated for a couple reasons
+including whether the error is transient or permanent, or if the error would
+apply to any FD operation, or just read. Finally, there may never be a
+subsequent read as perhaps the bytes leading up to the error are enough to
+satisfy the processor.
+
+This decision boils down to whether or not to track an error bit per file
+descriptor or not. If not, the assumption is that a subsequent operation would
+also error, this time without reading any bytes.
+
+The current opinion is to go with the simplest path, which is to return the
+bytes read and ignore the error the there were any. Assume a subsequent
+operation will err if it needs to. This helps reduce the complexity of the code
+in wazero and also accommodates the scenario where the bytes read are enough to
+satisfy its processor.
+
 ### FdPrestatDirName
 
 `FdPrestatDirName` is a WASI function to return the path of the pre-opened directory of a file descriptor.
diff --git a/internal/engine/compiler/engine.go b/internal/engine/compiler/engine.go
index a73ab924..777e504f 100644
--- a/internal/engine/compiler/engine.go
+++ b/internal/engine/compiler/engine.go
@@ -796,7 +796,7 @@ func (ce *callEngine) builtinFunctionMemoryGrow(ctx context.Context, mem *wasm.M
 }
 
 func (ce *callEngine) builtinFunctionTableGrow(ctx context.Context, tables []*wasm.TableInstance) {
-	tableIndex := ce.popValue()
+	tableIndex := uint32(ce.popValue())
 	table := tables[tableIndex] // verified not to be out of range by the func validation at compilation phase.
 	num := ce.popValue()
 	ref := ce.popValue()
diff --git a/internal/engine/compiler/impl_arm64.go b/internal/engine/compiler/impl_arm64.go
index 928f360a..0a38e7d9 100644
--- a/internal/engine/compiler/impl_arm64.go
+++ b/internal/engine/compiler/impl_arm64.go
@@ -1362,12 +1362,11 @@ func (c *arm64Compiler) compilePick(o *wazeroir.OperationPick) error {
 	if pickTarget.onRegister() { // Copy the value to the pickedRegister.
 		switch pickTarget.valueType {
 		case runtimeValueTypeI32:
-			c.assembler.CompileRegisterToRegister(arm64.MOVD, pickTarget.register, pickedRegister)
+			c.assembler.CompileRegisterToRegister(arm64.MOVW, pickTarget.register, pickedRegister)
 		case runtimeValueTypeI64:
 			c.assembler.CompileRegisterToRegister(arm64.MOVD, pickTarget.register, pickedRegister)
 		case runtimeValueTypeF32:
-			// TODO: use 32-bit mov.
-			c.assembler.CompileRegisterToRegister(arm64.FMOVD, pickTarget.register, pickedRegister)
+			c.assembler.CompileRegisterToRegister(arm64.FMOVS, pickTarget.register, pickedRegister)
 		case runtimeValueTypeF64:
 			c.assembler.CompileRegisterToRegister(arm64.FMOVD, pickTarget.register, pickedRegister)
 		case runtimeValueTypeV128Lo:
@@ -4003,15 +4002,13 @@ func (c *arm64Compiler) compileLoadConditionalRegisterToGeneralPurposeRegister(l
 func (c *arm64Compiler) compileLoadValueOnStackToRegister(loc *runtimeValueLocation) {
 	switch loc.valueType {
 	case runtimeValueTypeI32:
-		// TODO: use 32-bit mov.
-		c.assembler.CompileMemoryToRegister(arm64.LDRD, arm64ReservedRegisterForStackBasePointerAddress,
+		c.assembler.CompileMemoryToRegister(arm64.LDRW, arm64ReservedRegisterForStackBasePointerAddress,
 			int64(loc.stackPointer)*8, loc.register)
 	case runtimeValueTypeI64:
 		c.assembler.CompileMemoryToRegister(arm64.LDRD, arm64ReservedRegisterForStackBasePointerAddress,
 			int64(loc.stackPointer)*8, loc.register)
 	case runtimeValueTypeF32:
-		// TODO: use 32-bit mov.
-		c.assembler.CompileMemoryToRegister(arm64.FLDRD, arm64ReservedRegisterForStackBasePointerAddress,
+		c.assembler.CompileMemoryToRegister(arm64.FLDRS, arm64ReservedRegisterForStackBasePointerAddress,
 			int64(loc.stackPointer)*8, loc.register)
 	case runtimeValueTypeF64:
 		c.assembler.CompileMemoryToRegister(arm64.FLDRD, arm64ReservedRegisterForStackBasePointerAddress,
@@ -4071,12 +4068,10 @@ func (c *arm64Compiler) compileReleaseAllRegistersToStack() (err error) {
 func (c *arm64Compiler) compileReleaseRegisterToStack(loc *runtimeValueLocation) {
 	switch loc.valueType {
 	case runtimeValueTypeI32:
-		// Use 64-bit mov as all the values are represented as uint64 in Go, so we have to clear out the higher bits.
-		c.assembler.CompileRegisterToMemory(arm64.STRD, loc.register, arm64ReservedRegisterForStackBasePointerAddress, int64(loc.stackPointer)*8)
+		c.assembler.CompileRegisterToMemory(arm64.STRW, loc.register, arm64ReservedRegisterForStackBasePointerAddress, int64(loc.stackPointer)*8)
 	case runtimeValueTypeI64:
 		c.assembler.CompileRegisterToMemory(arm64.STRD, loc.register, arm64ReservedRegisterForStackBasePointerAddress, int64(loc.stackPointer)*8)
 	case runtimeValueTypeF32:
-		// Use 64-bit mov as all the values are represented as uint64 in Go, so we have to clear out the higher bits.
 		c.assembler.CompileRegisterToMemory(arm64.FSTRS, loc.register, arm64ReservedRegisterForStackBasePointerAddress, int64(loc.stackPointer)*8)
 	case runtimeValueTypeF64:
 		c.assembler.CompileRegisterToMemory(arm64.FSTRD, loc.register, arm64ReservedRegisterForStackBasePointerAddress, int64(loc.stackPointer)*8)
diff --git a/wasi_snapshot_preview1/fs.go b/wasi_snapshot_preview1/fs.go
index 5a1866df..b05dabe9 100644
--- a/wasi_snapshot_preview1/fs.go
+++ b/wasi_snapshot_preview1/fs.go
@@ -386,6 +386,7 @@ var fdRead = wasm.NewGoFunc(
 	[]string{"fd", "iovs", "iovs_len", "result.size"},
 	func(ctx context.Context, mod api.Module, fd, iovs, iovsCount, resultSize uint32) Errno {
 		sysCtx := mod.(*wasm.CallContext).Sys
+		mem := mod.Memory()
 		reader := internalsys.FdReader(ctx, sysCtx, fd)
 		if reader == nil {
 			return ErrnoBadf
@@ -394,33 +395,53 @@ var fdRead = wasm.NewGoFunc(
 		var nread uint32
 		for i := uint32(0); i < iovsCount; i++ {
 			iovPtr := iovs + i*8
-			offset, ok := mod.Memory().ReadUint32Le(ctx, iovPtr)
+			offset, ok := mem.ReadUint32Le(ctx, iovPtr)
 			if !ok {
 				return ErrnoFault
 			}
-			l, ok := mod.Memory().ReadUint32Le(ctx, iovPtr+4)
+			l, ok := mem.ReadUint32Le(ctx, iovPtr+4)
 			if !ok {
 				return ErrnoFault
 			}
-			b, ok := mod.Memory().Read(ctx, offset, l)
+			b, ok := mem.Read(ctx, offset, l)
 			if !ok {
 				return ErrnoFault
 			}
-			n, err := reader.Read(b) // Note: n <= l
+
+			n, err := reader.Read(b)
 			nread += uint32(n)
-			if errors.Is(err, io.EOF) {
+
+			shouldContinue, errno := fdRead_shouldContinueRead(uint32(n), l, err)
+			if errno != 0 {
+				return errno
+			} else if !shouldContinue {
 				break
-			} else if err != nil {
-				return ErrnoIo
 			}
 		}
-		if !mod.Memory().WriteUint32Le(ctx, resultSize, nread) {
+		if !mem.WriteUint32Le(ctx, resultSize, nread) {
 			return ErrnoFault
 		}
 		return ErrnoSuccess
 	},
 )
 
+// fdRead_shouldContinueRead decides whether to continue reading the next iovec
+// based on the amount read (n/l) and a possible error returned from io.Reader.
+//
+// Note: When there are both bytes read (n) and an error, this continues.
+// See /RATIONALE.md "Why ignore the error returned by io.Reader when n > 1?"
+func fdRead_shouldContinueRead(n, l uint32, err error) (bool, Errno) {
+	if errors.Is(err, io.EOF) {
+		return false, 0 // EOF isn't an error, and we shouldn't continue.
+	} else if err != nil && n == 0 {
+		return false, ErrnoIo
+	} else if err != nil {
+		return false, 0 // Allow the caller to process n bytes.
+	}
+	// Continue reading, unless there's a partial read or nothing to read.
+	return n == l && n != 0, 0
+}
+
 // fdReaddir is the WASI function named functionFdReaddir which reads directory
 // entries from a directory.
 //


Test patch:
diff --git a/internal/engine/compiler/engine_test.go b/internal/engine/compiler/engine_test.go
index 176a4934..c22dc094 100644
--- a/internal/engine/compiler/engine_test.go
+++ b/internal/engine/compiler/engine_test.go
@@ -306,3 +306,22 @@ func TestEngine_Cachedcodes(t *testing.T) {
 	_, ok = e.getCodes(m)
 	require.False(t, ok)
 }
+
+func TestCallEngine_builtinFunctionTableGrow(t *testing.T) {
+	ce := &callEngine{
+		valueStack: []uint64{
+			0xff, // pseudo-ref
+			1,    // num
+			// Table Index = 0 (lower 32-bits), but the higher bits (32-63) are all sets,
+			// which happens if the previous value on that stack location was 64-bit wide.
+			0xffffffff << 32,
+		},
+		valueStackContext: valueStackContext{stackPointer: 3},
+	}
+
+	table := &wasm.TableInstance{References: []wasm.Reference{}, Min: 10}
+	ce.builtinFunctionTableGrow(context.Background(), []*wasm.TableInstance{table})
+
+	require.Equal(t, 1, len(table.References))
+	require.Equal(t, uintptr(0xff), table.References[0])
+}
diff --git a/wasi_snapshot_preview1/fs_test.go b/wasi_snapshot_preview1/fs_test.go
index fa15b9a4..a98c7878 100644
--- a/wasi_snapshot_preview1/fs_test.go
+++ b/wasi_snapshot_preview1/fs_test.go
@@ -615,6 +615,91 @@ func Test_fdRead_Errors(t *testing.T) {
 	}
 }
 
+func Test_fdRead_shouldContinueRead(t *testing.T) {
+	tests := []struct {
+		name          string
+		n, l          uint32
+		err           error
+		expectedOk    bool
+		expectedErrno Errno
+	}{
+		{
+			name: "break when nothing to read",
+			n:    0,
+			l:    0,
+		},
+		{
+			name: "break when nothing read",
+			n:    0,
+			l:    4,
+		},
+		{
+			name: "break on partial read",
+			n:    3,
+			l:    4,
+		},
+		{
+			name:       "continue on full read",
+			n:          4,
+			l:          4,
+			expectedOk: true,
+		},
+		{
+			name: "break on EOF on nothing to read",
+			err:  io.EOF,
+		},
+		{
+			name: "break on EOF on nothing read",
+			l:    4,
+			err:  io.EOF,
+		},
+		{
+			name: "break on EOF on partial read",
+			n:    3,
+			l:    4,
+			err:  io.EOF,
+		},
+		{
+			name: "break on EOF on full read",
+			n:    4,
+			l:    4,
+			err:  io.EOF,
+		},
+		{
+			name:          "return ErrnoIo on error on nothing to read",
+			err:           io.ErrClosedPipe,
+			expectedErrno: ErrnoIo,
+		},
+		{
+			name:          "return ErrnoIo on error on nothing read",
+			l:             4,
+			err:           io.ErrClosedPipe,
+			expectedErrno: ErrnoIo,
+		},
+		{ // Special case, allows processing data before err
+			name: "break on error on partial read",
+			n:    3,
+			l:    4,
+			err:  io.ErrClosedPipe,
+		},
+		{ // Special case, allows processing data before err
+			name: "break on error on full read",
+			n:    4,
+			l:    4,
+			err:  io.ErrClosedPipe,
+		},
+	}
+	for _, tt := range tests {
+		tc := tt
+
+		t.Run(tc.name, func(t *testing.T) {
+			ok, errno := fdRead_shouldContinueRead(tc.n, tc.l, tc.err)
+			require.Equal(t, tc.expectedOk, ok)
+			require.Equal(t, tc.expectedErrno, errno)
+		})
+	}
+}
+
 // Test_fdReaddir only tests it is stubbed for GrainLang per #271
 func Test_fdReaddir(t *testing.T) {
 	log := requireErrnoNosys(t, functionFdReaddir, 0, 0, 0, 0, 0)