no_special_abortOnCannotGrowMemory

[juj]

Remove special handling of function abortOnCannotGrowMemory() but make it a regular JS library function.

[kripken]

Nice!

Looks like some tests need updating. lgtm if that's just trivial stuff.

[PiotrSikora]

Note: this changed the signature of abortOnCannotGrowMemory from:

(import "env" "abortOnCannotGrowMemory" (func $abortOnCannotGrowMemory (result i32)))

to:

(import "env" "abortOnCannotGrowMemory" (func $abortOnCannotGrowMemory (param i32) (result i32)))

which is a bit problematic for non-JavaScript host runtimes that verify import/export signatures.

[juj]

What causes a problem with the change? The signature did change, but unless I did a mistake, all call sites to the function were updated, so call sites + declaration should match?

[PiotrSikora]

Non-JavaScript host runtimes have to re-implement exported Emscripten functions, e.g.:

• https://github.com/WAVM/WAVM/blob/master/Lib/Emscripten/Emscripten.cpp
• https://github.com/rianhunter/wasmjit/blob/master/src/wasmjit/emscripten_runtime.c
• https://github.com/wasmerio/wasmer/tree/master/lib/emscripten/src

since they cannot import generated JavaScript.

But because the signature changed, WASM modules compiled using Emscripten 1.38.26 are trying to import function with a signature that doesn't match the signature in those runtimes.

While I understand that this is not exactly your problem, it would be great if Emscripten wasn't introducing breaking changes that are hard to detect and/or recover from, i.e. adding and/or renaming functions is perfectly fine and easy to support (e.g. WAVM/WAVM@69d647e), but changing signatures is a bit more problematic.

[juj]

Thanks, did not know there was this kind of external use going on. Freezing the signatures of JS functions imported to JS is unfortunately not something that we could make a guarantee about, but I'll keep this use case in mind and go the renaming route whenever possible.

[PiotrSikora]

Yeah, I didn't mean freezing / providing stable ABI, but keeping in mind the other implementations. Thanks!

[kripken]

We have an ABI version, added by @rianhunter - I don't think we need to rename things (but we can if there's no reason not to), but we do need to update that metadata version, so that other embedders are not surprised.

@juj, please open a PR with an update to EMSCRIPTEN_METADATA_MINOR in tools/shared.py.

@PiotrSikora - sorry for missing this earlier!

[rianhunter]

EMSCRIPTEN_ABI_MINOR / EMSCRIPTEN_ABI_MAJOR specify the minimum version of the ABI the WASM binary requires. Since you changed the signature of abortOnCannotGrowMemory, you'd ordinarily bump EMSCRIPTEN_ABI_MAJOR since newer WASM binaries would not be compatible with older versions of the runtime. Preferably in the future, it would be better to change the function name, like abortOnCannotGrowMemoryV2 so you only need to bump EMSCRIPTEN_ABI_MINOR, and the 3rd party runtime would only need to provide a new function to maintain compatibility with both newer and older WASM binaries.

Since EMSCRIPTEN_ABI_MAJOR is currently zero, it's fine to making breaking ABI changes and only increment EMSCRIPTEN_ABI_MINOR since nothing is stable, but this habit should be tempered.