unknown import: `env::sync/atomic.StoreUint32` has not been defined

[kyleconroy]

Using the latest dev, I'm unable to run code generated by protoc when it's compiled to WASM / WASI.

Error: failed to run main module `example.wasm`

Caused by:
    0: failed to instantiate "example.wasm"
    1: unknown import: `env::sync/atomic.StoreUint32` has not been defined
make: *** [run] Error 1

A full reproduction can be found here: https://github.com/kyleconroy/tinygo-protoc. You'll need to have wasmtime installed to run the example.wasm file.

[dgryski]

For me this also fails on native.

[dgryski]

Minimal reproducer:

From https://github.com/protocolbuffers/protobuf-go/blob/master/internal/impl/extension.go#L131

import (
	"sync/atomic"
)

var global uint32

func foo() {
	defer atomic.StoreUint32(&global, 1)

	atomic.StoreUint32(&global, 2)
}

func main() {
	foo()
	println(atomic.LoadUint32(&global))
}

[dgryski]

Potential solution: include a real copy of this function (instead of a compiler intrinsic) but marked as "always-inline"

[kyleconroy]

@dgryski I'd love to implement this, but I understood about one-fifth of your comment. If you have any pointers on where to get started, let me know.

[skabbes]

@kyleconroy I'm certainly not an expert - just feeling my way in the dark - but what I believe he means is that atomic.StoreUint32 is already implemented - you can see it here:

tinygo/compiler/atomic.go

Line 88 in 3883550

case "StoreInt32", "StoreInt64", "StoreUint32", "StoreUint64", "StoreUintptr", "StorePointer":

and I believe it calls this at runtime:

tinygo/src/runtime/atomics_critical.go

Line 123 in 3883550

func __atomic_store_4(ptr *uint32, val uint32, ordering uintptr) {

What I think is happening is that the call to atomic.StoreX is being directly rewritten to a llmv atomic store.

tinygo/compiler/compiler.go

Line 1446 in 3883550

case strings.HasPrefix(name, "sync/atomic."):

And the actual function for atomic.StoreUint32 is not being generated in the target runtime.

Maybe here is a good place to look:

tinygo/compiler/defer.go

Line 348 in 3883550

case *ssa.Function:

[skabbes]

OK, make a little progress at understanding this. The "defer" is the thing that is causing issues here. It seems that running defers, does not (and possibly cannot?) go through the same code as createFunctionCall.

Usually, there are no symbols emitted for sync/atomic.* since createFunctionCall rewrites them to llvm operations (and there are no calls to sync/atomic.* needed.

If you write the code like this (a function storeUint32 which is inlined, and defer that function instead of atomic.StoreUint32 directly), it works perfectly with tinygo.

package main

import (
	"sync/atomic"
)

var global uint32

//go:inline
func storeUint32(addr *uint32, val uint32) {
	atomic.StoreUint32(&global, 1)
}

func main() {
	defer storeUint32(&global, 1)
	atomic.StoreUint32(&global, 2)
	println(atomic.LoadUint32(&global))
}

So I think maybe writing src/sync/atomic and enabling it as an merged override. would work?

Or possibly just overwrite the symbol? Then this can be worked around for now it seems.
Like demonstrated here.

Here is an example:

// FILE: main.go
package main

import (
	"sync/atomic"
)

var global uint32

func foo() {
	defer atomic.StoreUint32(&global, 1)
	atomic.StoreUint32(&global, 2)
	println(atomic.LoadUint32(&global))
}

func main() {
	// should print:
	// 2
	// 1
	foo()
	println(atomic.LoadUint32(&global))
}


// FILE: tgfix.go
//go:build tinygo
// +build tinygo

package main

import (
	"sync/atomic"
	_ "unsafe" // needed for go:linkname
)

//go:linkname bugfixSyncAtomicStoreUint32 sync/atomic.StoreUint32
func bugfixSyncAtomicStoreUint32(addr *uint32, val uint32) {
	// this will get rewritten by tinygo into llvm atomic store
	atomic.StoreUint32(addr, val)
}

[kyleconroy]

@skabbes Changing the code isn't really an option, as it comes from deep inside the protocol buffer module.

So I think maybe writing src/sync/atomic and enabling it as an merged override. would work?

This doesn't work because there's no code inside src/sync/atomic. So we'd need to add this function into the atomic package, but I'm not sure how to implement it as a "real" function. The Go stdlib doesn't help, because it's implemented in assembly.

[kyleconroy]

Okay, I got this to compile and "run" by copying over sync/atomic/doc.go from the Go source code and making all the functions no-ops. You can see what the change looks like here: #2805

This is obviously wrong, as the implementation in src/sync/atomic/doc.go is a no-op. The small test case outputs 2 instead of 1.

[skabbes]

Changing the code isn't really an option, as it comes from deep inside the protocol buffer module.

I don't think you need to change the code, you just need to include that 1 extra file somewhere in your build (tgfix.go). (the PB generated code doesn't even need to be aware of it at all). TinyGo already knows about atomic.StoreUint32, and it also already works. But it doesn't need the assembly implementation at all because it simplifies any calls to it to just be direct LLVM instructions (EXCEPT if that call is deferred, it seems).

So...

but I'm not sure how to implement it as a "real" function

As odd as it sounds, I think you can implement it by having it call itself.

package atomic

//go:inline
func StoreUint32(addr *uint32, val uint32) {
    StoreUint32(addr, val);
}

The "inner" call will be overwritten by this code. And the "outer" call will just generate a function that allows deferral.

This is obviously really insane and fragile, but I Think a better technique would be to stub out the entire set of atomic operations with inline functions (similar to what I did above). And have the compiler only rewrite ones prefixed with tinygo_. For example.

package atomic

//go:inline
func StoreUint32(addr *uint32, val uint32) {
    tinygo_StoreUint32(addr, val);
}

And the corresponding change in compiler/atomic.go

[skabbes]

I think the only thing to check then is that atomic.* does not generate extra function calls (And the atomic store / load / compare do not generate extra function calls).

I think if you are using atomics, it is usually for performance reasons, so they tinygo maintainers probably want that to not generate extra function calls just to load or store an atomic var.

Actually, now that I think about it, perhaps a diff like this would be better.

(which is to say, keep all the previous behavior - AND allow tinygo_* to be rewritten appropriately. (that rewrite would be your implementation).

[kyleconroy]

Great idea using an unexported method. I updated my PR. If the tests pass I'll open it up for review.