Skip to content

runtime (wasi): 64-bit integers require 8 byte alignment #2010

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
niaow opened this issue Jul 21, 2021 · 7 comments
Closed

runtime (wasi): 64-bit integers require 8 byte alignment #2010

niaow opened this issue Jul 21, 2021 · 7 comments

Comments

@niaow
Copy link
Member

niaow commented Jul 21, 2021

While working on asyncify support, I ran into a crash where wasmtime failed to store the timestamp. Interestingly, my debug code also caused it to reproduce on the dev branch.

This bug can be reproduced by attempting to print the address of the timestamp before passing it to clock_time_get:

func ticks() timeUnit {
	var nano uint64
	println(&nano)
	clock_time_get(0, timePrecisionNanoseconds, &nano)
	return timeUnit(nano)
}

The address (0x0001064c) appears to be 4-byte aligned. I don't think that is incorrect on 32-bit wasm?
However it breaks wasmtime:

[niaow@finch tinygo]$ build/tinygo build -target wasi -gc=leaking -opt=1 -o goroutines.wasm ./testdata/goroutines.go
[niaow@finch tinygo]$ WASMTIME_BACKTRACE_DETAILS=1 wasmtime goroutines.wasm 
main 1
sub 1
0x0001064c
Error: failed to run main module `goroutines.wasm`

Caused by:
    0: failed to invoke command default
    1: In func clock_time_get:write timestamp:
       wasm backtrace:
           0: 0x1cc2 - runtime.ticks
                           at /home/niaow/go/src/github.com/tinygo-org/tinygo/src/runtime/runtime_wasm_wasi.go:89:16
           1: 0x1f50 - runtime.addSleepTask
                           at /home/niaow/go/src/github.com/tinygo-org/tinygo/src/runtime/scheduler.go:92:14
           2: 0x2025 - time.Sleep
                           at /home/niaow/go/src/github.com/tinygo-org/tinygo/src/runtime/scheduler_any.go:10:14
           3: 0x24e6 - main.sub
                           at /home/niaow/go/src/github.com/tinygo-org/tinygo/testdata/goroutines.go:91:12
           4: 0x1ed9 - main.main
                           at /home/niaow/go/src/github.com/tinygo-org/tinygo/testdata/goroutines.go:11:2
           5: 0x1d47 - runtime.run$1
                           at /home/niaow/go/src/github.com/tinygo-org/tinygo/src/runtime/scheduler_any.go:21:11
           6: 0x1cf8 - runtime.run
                           at /home/niaow/go/src/github.com/tinygo-org/tinygo/src/runtime/scheduler_any.go:18:2
           7: 0x1ce5 - _start
                           at /home/niaow/go/src/github.com/tinygo-org/tinygo/src/runtime/runtime_wasm_wasi.go:21:5

Not entirely sure what is wrong yet.

@niaow
Copy link
Member Author

niaow commented Jul 21, 2021

Actually, nevermind. It does in fact need 8-byte alignment.

@niaow
Copy link
Member Author

niaow commented Jul 21, 2021

Note that this also does replicate under the conservative collector with another 4-byte-aligned address.

@niaow
Copy link
Member Author

niaow commented Jul 21, 2021

Reference: https://github.com/WebAssembly/WASI/blob/main/phases/snapshot/docs.md#-timestamp-u64 indicates that the alignment of a timestamp is 8 bytes.

@niaow niaow changed the title runtime (wasi): clock_time_get appears slightly broken runtime (wasi): clock_time_get requires 8 byte alignment Jul 21, 2021
@niaow niaow changed the title runtime (wasi): clock_time_get requires 8 byte alignment runtime (wasi): 64-bit integers require 8 byte alignment Jul 21, 2021
@aykevl
Copy link
Member

aykevl commented Jul 21, 2021

Huh, interesting. I can reproduce this with this simpler code:

package main

func main() {
	var nano uint64
	println(&nano)
}

@niaow
Copy link
Member Author

niaow commented Jul 21, 2021

Er, yeah I see that printing a value with 4-byte alignment. I don't think that most of the code expects anything to need higher alignment. The thing that surprised me was the crash.

@aykevl
Copy link
Member

aykevl commented Jul 21, 2021

Here is an ugly workaround:

diff --git a/src/runtime/gc_conservative.go b/src/runtime/gc_conservative.go
index e9682592..b6d18455 100644
--- a/src/runtime/gc_conservative.go
+++ b/src/runtime/gc_conservative.go
@@ -228,6 +228,7 @@ func setHeapEnd(newHeapEnd uintptr) {
 // This function can be called again when the heap size increases. The caller is
 // responsible for copying the metadata to the new location.
 func calculateHeapAddresses() {
+       heapStart = (heapStart + 7) &^ 7
        totalSize := heapEnd - heapStart
 
        // Allocate some memory to keep 2 bits of information about every block.

Related (possibly even a duplicate): #2009

@aykevl
Copy link
Member

aykevl commented Jul 21, 2021

@niaow thank you for reporting this issue, including the reproducer and especially the reference to the WASI standard. I consider this a duplicate of #2009 so I'll close this issue in favor of #2009.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants