[WASM] __heap_base not always aligned

[fgsch]

While investigating a crash using #2003 and with @aykevl help, we found the heap start (__heap_base) is not always aligned.
For example building with:

tinygo build -interp-pass=none -target wasi -o main.wasm code.go

And setting https://github.com/tinygo-org/tinygo/blob/dev/src/runtime/gc_conservative.go#L240 to true, I get:

heapStart:         0x00013551
heapEnd:           0x00040000
total size:        0x0002caaf
metadata size:     0x00000b2a
metadataStart:     0x0003f4d6
# of blocks:       0x00002bf8
# of block states: 0x00002ca8

[aykevl]

This may be a LLVM bug, this might be a way to fix it:

diff --git a/lld/wasm/Writer.cpp b/lld/wasm/Writer.cpp
index 63e3dac5d298..5ceb4d76e981 100644
--- a/lld/wasm/Writer.cpp
+++ b/lld/wasm/Writer.cpp
@@ -312,6 +312,7 @@ void Writer::layoutMemory() {
     // Set `__heap_base` to directly follow the end of the stack or global data.
     // The fact that this comes last means that a malloc/brk implementation
     // can grow the heap at runtime.
+    memoryPtr = alignTo(memoryPtr, 4);
     log("mem: heap base   = " + Twine(memoryPtr));
     WasmSym::heapBase->setVA(memoryPtr);
   }

I haven't tested this patch yet (right now compiling LLVM).

[fgsch]

I can confirm the patch fixes this issue.

[niaow]

According to #2010, it appears that we may actually need 8 byte alignment.

[aykevl]

@fgsch thanks for verifying!

I've made a patch here: https://reviews.llvm.org/D106499, which aligns to 8 bytes.

[aykevl]

There are two workarounds from the TinyGo side:

• remove the --stack-first flag
• manually align heapStart

The second workaround looks like this:

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.

This is a pretty ugly workaround as-is, but we might need a cleaned-up version of this until the wasm-ld bug is fixed.

[aykevl]

I have pushed a change to LLVM to fix this, it should be part of LLVM 13: https://reviews.llvm.org/D106499

[fgsch]

@aykevl great news. In the meantime, should we get the workaround committed?

[aykevl]

Yes, but then it should be something like this to not impact other targets:

diff --git a/src/runtime/gc_conservative.go b/src/runtime/gc_conservative.go
index e9682592..935fa591 100644
--- a/src/runtime/gc_conservative.go
+++ b/src/runtime/gc_conservative.go
@@ -228,6 +228,10 @@ 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() {
+       if GOARCH == "wasm" {
+               const heapAlign = 16
+               heapStart = (heapStart + heapAlign - 1) &^ (heapAlign - 1)
+       }
        totalSize := heapEnd - heapStart
 
        // Allocate some memory to keep 2 bits of information about every block.

[aykevl]

Fix: #2014
The manual alignment of heapStart should be removed in the future to avoid the increase in code size.

[deadprogram]

Released with 0.20.0 so now closing. Thank you!