Skip to content
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

Crash with -target=wasi and -gc=leaking #2041

Closed
ckaznocha opened this issue Aug 12, 2021 · 10 comments
Closed

Crash with -target=wasi and -gc=leaking #2041

ckaznocha opened this issue Aug 12, 2021 · 10 comments
Labels
bug Something isn't working wasm WebAssembly

Comments

@ckaznocha
Copy link

ckaznocha commented Aug 12, 2021

I'm experiencing some runtime issues with tinygo produced wasi with the leaking gc. I think I've been able to distill the issue down to its core.

> ./tinygo version
tinygo version 0.20.0-dev-5e5ce98d darwin/amd64 (using go version go1.16.6 and LLVM version 11.0.0)
> wasmtime --version
wasmtime 0.29.0

Source code:

package main

import (
	"fmt"
	"sync"
)

var (
	once  sync.Once
	ready = make(chan struct{})
)

func main() {
	once.Do(func() {
		close(ready)
	})

	<-ready

	fmt.Println()
}

Expected output:

Actual output (modified to remove the full file paths):

> ./tinygo build -target=wasi -gc=leaking -o test.wasm && WASMTIME_BACKTRACE_DETAILS=1 wasmtime test.wasm
Error: failed to run main module `test.wasm`

Caused by:
    0: failed to invoke command default
    1: wasm trap: unreachable
       wasm backtrace:
           0: 0x4a87 - (*internal/task.Task).setReturnPtr
                           at {src_path}/github.com/tinygo-org/tinygo/src/internal/task/task_coroutine.go:51:4
                     - main.main
                           at {src_path}/github.com/fastly/ckaznocha/test/main.go:14:9
           1: 0x452f - (*internal/task.Task).Resume
                           at {src_path}/github.com/tinygo-org/tinygo/src/internal/task/task_coroutine.go:23:12
                     - runtime.scheduler
                           at {src_path}/github.com/tinygo-org/tinygo/src/runtime/scheduler.go:171:11
                     - runtime.run
                           at {src_path}/github.com/tinygo-org/tinygo/src/runtime/scheduler_any.go:24:11
                     - _start
                           at {src_path}/github.com/tinygo-org/tinygo/src/runtime/runtime_wasm_wasi.go:21:5

A few observations:

  • The program runs correctly with -opt=1
  • The program runs correctly with the conservative gc but that causes other issues for me.
  • The program runs correctly if I change fmt.Println() to log.Println()
@deadprogram deadprogram added the wasm WebAssembly label Aug 16, 2021
@dgryski
Copy link
Member

dgryski commented Sep 1, 2021

I've been looking into this with an execution trace from wasm3-strace and a decompilation of the wasm output from wasm-decompile.

The unreachable code is from this line:

function main_main_resume(a:int) {
  var b:int_ptr = a[8]:int;
  if (a[40]:ubyte) goto B_c;
  if (b) goto B_b;
  goto B_a;
  label B_c:
  if (eqz(b)) goto B_a;
  b[4] = a[9]:int;
  Push(b);
  return ;
  label B_b:
  unreachable;  <--- HERE
  unreachable;
  label B_a:
  runtime_nilPanic();
  unreachable;
}

The equivalent code compiled with -opt 0

function main_main_resume(a:int) {
  var b:int = 0;
  if (a[40]:ubyte) goto B_a;
  Lcoro_devirt_trigger(0);
  b = d_b[22]:int;
  var c:int = a[8]:int;
  setReturnPtr(c, a + 41, a, a);
  runtime_chanRecv(b, a + 32, a + 8, a, c);
  a[40]:byte = 1;
  b = 255;
  label B_a:
  if (b) goto B_b;
  returnTo(a[8]:int, a[9]:int, a, a);
  runtime_free(a, a, a);
  label B_b:
}

The wasm3-strace output for this section (of the optimized code):

    main.main.resume (i32: 66064) { <-- a is address 66064
      load.i32  0x10230 = 65992 <-- this a[8]:int = hex(66064 + 8*32) = 0x10230 
      load.i32  0x10238 = 0 <-- this is a[40]:ubyte = hex(66064 + 40) = 0x10238
    } !trap = [trap] unreachable executed

I will attach these full files, or maybe stuff them in a gist.

Update: https://gist.github.com/dgryski/0be7ddd4b13ad5ed2fc8bb201dee5ae9

@deadprogram deadprogram added the bug Something isn't working label Sep 1, 2021
@dgryski
Copy link
Member

dgryski commented Sep 1, 2021

Smallest reproducer so far:

package main

import "time"

var ch = make(chan struct{})

func foo() {
	time.Sleep(1)
	close(ch)
}

func main() {
	foo()
	<-ch
}

Seems to be something related to the chan struct{} and the coroutine scheduler. Removing time.Sleep(1) causes this to work, and changing the channel type to chan int also causes this to work. Moving the time.Sleep call out of foo into main or to after the close also removes the crash.

@dgryski
Copy link
Member

dgryski commented Sep 4, 2021

Current theory (via @aykevl):

The LLVM coroutine passes convert stack allocations (alloca) into a single object. So what was previously a regular object with start/end lifetimes, is now placed in a bigger object. Before this transformation, the lifetimes are correct: they only apply to that particular alloca. Afterwards, they aren't an alloca anymore and therefore apply to the whole object - which contains much more than just that alloca.

@aykevl
Copy link
Member

aykevl commented Sep 8, 2021

When I compile with LLVM assertions enabled, I get the following crash:

tinygo: /home/ayke/src/github.com/tinygo-org/tinygo/llvm-project/llvm/include/llvm/Support/OptimizedStructLayout.h:52: llvm::OptimizedStructLayoutField::OptimizedStructLayoutField(const void *, uint64_t, llvm::Align, uint64_t): Assertion `Size > 0 && "adding an empty field to the layout"' failed.

So it appears there is an internal error.

@aykevl
Copy link
Member

aykevl commented Sep 8, 2021

Looks like it's fixed with this patch:

diff --git a/compiler/llvmutil/llvm.go b/compiler/llvmutil/llvm.go
index 21bc6c67..32691466 100644
--- a/compiler/llvmutil/llvm.go
+++ b/compiler/llvmutil/llvm.go
@@ -35,7 +35,14 @@ func CreateTemporaryAlloca(builder llvm.Builder, mod llvm.Module, t llvm.Type, n
        ctx := t.Context()
        targetData := llvm.NewTargetData(mod.DataLayout())
        i8ptrType := llvm.PointerType(ctx.Int8Type(), 0)
-       alloca = CreateEntryBlockAlloca(builder, t, name)
+       allocType := t
+       if targetData.TypeAllocSize(t) == 0 {
+               allocType = mod.Context().Int8Type()
+       }
+       alloca = CreateEntryBlockAlloca(builder, allocType, name)
+       if allocType != t {
+               alloca = builder.CreateBitCast(alloca, llvm.PointerType(t, 0), "")
+       }
        bitcast = builder.CreateBitCast(alloca, i8ptrType, name+".bitcast")
        size = llvm.ConstInt(ctx.Int64Type(), targetData.TypeAllocSize(t), false)
        builder.CreateCall(getLifetimeStartFunc(mod), []llvm.Value{size, bitcast}, "")

@dgryski
Copy link
Member

dgryski commented Sep 8, 2021

Looks like this makes zero-byte allocations struct{} one byte instead?

@aykevl
Copy link
Member

aykevl commented Sep 8, 2021

Yeah. I'm now looking into a better fix for this. But it only increases my suspicion that this is actually a case of a LLVM bug that was fixed in LLVM 12: https://bugs.llvm.org/show_bug.cgi?id=49916 (fixed here: https://reviews.llvm.org/D101841).

@aykevl
Copy link
Member

aykevl commented Sep 8, 2021

I made a better fix for the reproducer in #2041 (comment), but this variant still crashes with the same LLVM bug (send instead of receive):

package main

import "time"

var ch = make(chan struct{})

func foo() {
	time.Sleep(1)
	close(ch)
}

func main() {
	foo()
	ch <- struct{}{}
}

@aykevl
Copy link
Member

aykevl commented Sep 8, 2021

Fix: #2100

@deadprogram deadprogram added the next-release Will be part of next release label Sep 9, 2021
@deadprogram
Copy link
Member

Released with 0.20.0 so now closing. Thank you!

@deadprogram deadprogram removed the next-release Will be part of next release label Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working wasm WebAssembly
Projects
None yet
Development

No branches or pull requests

4 participants