Adds _initiialize for c-shared wasm with go

[juliens]

In the recently merged proposal for wasm export with the go compiler golang/go#65199, when you compile with wasm export and the build mode to c-sharedthere is no _startfunction exported but a _initializefunction.

As the startFunctions is verified before being called, it should not impact any body if the _initialize is not defined.

[achille-roussel]

If both are present, would it be more correct to run _initialize first?

[ncruces]

I'd say so, yes. Or make it an error, but that's probably harder (and harder to explain).

[ncruces]

Go implementing this is interesting, but this has been possible with (e.g.) wasi-sdk (or clang) with -mexec-model=reactor.

I support adding _initialize to the start function list, especially because for some modules, wasm-ctor-eval and wizer may make the _initialize disappear, so having to call it explicitly is less “ergonomic” I'd say.

[mathetake]

will this be a part of Go spec?

[ncruces]

AFAIK _initialize and _start are both part of the wasip1 ABI, and mutually exclusive.

I'm not sure what follows after wasip1. Like, should our wasip1 package handle this, rather than by default? If so, _start is also a wasip1 thing, AFAICT.

[anuraaga]

Just for some context, Node has two entry points corresponding to each of them. I found it to be sort of tedious

nodejs/node#44995

But one reason for it was that initialize functions can't return an error code.

It seems simplest for users still to just handle either transparently.

[ncruces]

OTOH supporting one and not the other seems weird.

OTOH maybe even including _start outside of WASI was a mistake and we don't want to go further down that path?

[mathetake]

yep that's exactly my thinking - I would rather remove _start in the first place

[ncruces]

Yeah, that's a breaking change though.
Maybe WASI could set this up, rather than default?

[mathetake]

a breaking change not in API, so I don't think that's something we promise to keep, and i think that's much clearer!

[evacchi]

I get the point, but that'd be still a change in behavior, though

[achille-roussel]

I would prefer we don't introduce breaking changes in the behavior.

A lot of users have indirect dependencies on wazero that they might not even know about. If they happen to upgrade wazero somewhat unintentionally and suddenly their software doesn't work anymore because _start isn't called automatically, they're in for going down a rabbit whole of understanding the root cause. I want to care for my fellow developers out there who are already dealing with tons of crap day to day, let us be on the side of making their lives easier.

That being said, since we already support automatically calling _start and I don't think we should drop it, I would still be in favor of automatically calling _initialize because otherwise we introduce an inconsistent experience where some modules will load "out of the box" with default configuration, and some others won't (c-shared & reactor). The cost is really low on Wazero here, if we can avoid answering GitHub issues and having to produce extra documentation to explain subtle implementation differences, it feels like we're doing the right thing?

I hope these perspectives are useful ✌️

[mathetake]

Do you want to fix the doc accordingly?

// WithStartFunctions configures the functions to call after the module is
// instantiated. Defaults to "_start".

// Instantiate instantiates a module from the WebAssembly binary (%.wasm)
// with default configuration, which notably calls the "_start" function,
// if it exists.

[mathetake]

on second though, this also means a breaking change right? before people manually call .ExportFunction("_initialize"), and this results in calling it twice

[mathetake]

i am inclined to avoid adding anything like _initialize at all

[achille-roussel]

You're right that it could result in _initialize being called twice, which might result in similar kinds of terrible bugs.

Considering what we discussed, I would agree that the best option at this time is probably to hold on making any changes and learn more about what problems users will face once they start using c-shared modules more.

[mathetake]

yep, agreed!

[ncruces]

on second though, this also means a breaking change right? before people manually call .ExportFunction("_initialize"), and this results in calling it twice

I would've thought before people would've called WithStartFunctions("_initialize") or WithStartFunctions("_initialize", "_start"), which this doesn't break at all.

Almost every change is a potential breaking change. Dropping _start is correct, but the potential of breakage is high (IMO). Adding _initilialize is consistent, and (again IMO) relatively safe. But do no harm is also reasonable.

I'd definitely vote for adding _initialize. My big fear there is not compatibility, is if _initialize will still make sense going forward, or if future WASI (preview 2, whatever) is going to come up with some other silly breakage and we accumulate cruft. Because they don't seem worried about it.

[achille-roussel]

@ncruces my intuition is also that adding _initialize would be the right trade off, but taking our time to build confidence doesn't hurt here. c-shared for WASM will be in Go 1.24 which is set to release on February next year, so let's use that time to reflect on it and gather more evidence?

[mathetake]

https://github.com/search?q=ExportedFunction%28%22_initialize%22%29+language%3AGo+&type=code people actually are doing ExportedFunction("_initialize")

[mathetake]

so my take here is having _start in the wazero core was totally mistake and garbage in my opinion, and people are relying on it is unfortunate (sysfs etc is also mistake to be honest). I agree that only having _start is inconsistent and desire to add _initialize to the list for consistency but that feels like adding additional garbage, so I would pretty much rather removing _start in the first place because this in anyway breaks somebody. So i am closing, and i really wish we hadn't added _start in the first place. Just forcing user to add one single line of WithStartFunction("_initialize") doesn't hurt at all in my opinion.

[anuraaga]

@achille-roussel Is it possible for the go compiler to register _initialiaze in the wasm start section in a idempotent way instead of requiring the runtime or user code to explicitly invoke it?

IIRC wasi-sdk does that for start but not reactor programs, but that's because no one actually cares about reactor mode (c-shared) since no money in it.

[achille-roussel]

It's an interesting idea, probably worth bringing up to Go in the form of either a discussion on #webassmebly in Slack, or a Github issue.

For full context, this was the original proposal golang/go#65199