Update Emscripten to 2.0.9 upstream #760
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
For quite some time Themis has been pinning Emscripten to an outdated version of the "fastcomp" flavor because recent versions have been broken, and the new "upstream" flavor has been broken as well. However, Emscripten 2.0+ does not support the "fastcomp" flavor anymore so there is no hope for an fix. Eventually we'd have to migrate to the "upstream" flavor. I have tried out the lastest Emscripten 2.0.9 upstream and it seems to be working--after some further tweaks. There are no signs of the issues that were observed before and prevented Themis Core from being compiled. With the latest version is can be compiled and run. Update the Emscripten version we use on CI to 2.0.9. This is also the version that we'd be building packages with for the next release. Note that the version is pinned and will need to be updated later, we won't be tracking the "latest" tag for a while, until we're sure that WasmThemis works fine with Emscripten 2.0.
As noted in the linked issue, the upstream flavor of Emscripten does not yet support LLVM's stack protector so have it disabled. Otherwise linker will produce errors about missing symbols, because they're not there. Some day it may be implemented though so I leave a FIXME in the code.
Themis Core uses the allocate() function provided by Emscripten runtime for quick on-stack allocations. This is a semi-private function which has unstable API. With the new Emscripten version its API has changed, now it accepts a "slab" parameter which indicates the size of the stack that needs to be allocated--for the entire function. That is, if we allocate memory on stack, allocate() may be called only once.
RESERVED_FUNCTION_POINTERS does not have much effect on the "upstream" flavor but it was important for the "fastcomp" flavor. Now that since Emscripten 2.0.1 the "fastcomp" flavor is not supported anymore, this option has been a synonym for ALLOW_TABLE_GROWTH. Use the new option instead directly then.
For some reason wasm-ld likes to strip *all* (each and every) function of Themis Core from the resulting *.wasm file. While they are there in the object files and archives, linker's dead code elimination thinks they are not needed in the WebAssembly code. Normally this is prevented by EMSCRIPTEN_KEEPALIVE attribute--which we still have and it's still defined--but for some reason it does not work. As a temporary workaround, pass the LINKABLE option to the linker which disables dead code elimination and other link-time optimizations. This leads to somemwhat bigger WebAssembly file which may be a bit slower, but at least this prevents issues. It might be some transient issue with a toolchain so there is a FIXME in the code. We'd need to keep an eye on it for some future releases. However, I have not found any similar issues in Emscripten tracker.
ilammy
added
the
W-WasmThemis 🌐
|
@vixentael, there are some issues in internal tracker related to this upgrade. I guess then could be closed once this lands into master. @shadinua, please make sure that Build Bot uses the updated Emscripten toolchain for the next release. The new code does not work with the old Emscripten version, unfortunately. |
vixentael
approved these changes
Nov 29, 2020
| let token_length_ptr = libthemis.allocate(4, 'i32', libthemis.ALLOC_STACK) | ||
| /// allocate() with ALLOC_STACK cannot be called multiple times, | ||
| /// but we need two size_t values so allocate an array, of a sort. | ||
| let result_length_ptr = libthemis.allocate(new ArrayBuffer(2 * 4), libthemis.ALLOC_STACK) |
|
@ilammy shall we try to hotfix WASM package with these new changes? It gives no direct value to users, but will allow us to test release flow with the new Emscripten, while our brains are here. |
|
@ilammy shall we try to hotfix WASM package with these new changes?
Nah... I don't think it warrants an immediate release. I mean, it's been like that for over a half a year so it can wait a bit more. This is not a security update so there is no rush to push it out. And if anything, I'd like a major update of a dependency to be included in a major update of the library, not some random patch release.
I all we want to test is Build Bot automation to ensure that it can build proper binaries, I believe there would be a way to test the build only, without publishing anything. At worst, just try with the same package version and get rejected by npm at publishing stage.
|
shadinua
approved these changes
Nov 30, 2020
|
Merge 'em all! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For quite some time Themis has been pinning Emscripten to an outdated version of the
fastcompflavor because recent versions have been broken, and the newupstreamflavor has been broken as well.However, Emscripten 2.0+ does not support the
fastcompflavor anymore so there is no hope for an fix. Eventually we'd have to migrate to theupstreamflavor.I have tried out the latest Emscripten 2.0.9 upstream and it seems to be working—after some further tweaks. There are no signs of the issues that were observed before and prevented Themis Core from being compiled. With the latest version is can be compiled and run.
Update the Emscripten version we use on CI to 2.0.9. This is also the version that we'd be building packages with for the next release. Note that the version is pinned and will need to be updated later, we won't be tracking the
latesttag for a while, until we're sure that WasmThemis works fine with Emscripten 2.0.There are no changes to WasmThemis API, it's behind the scenes.
What needs to be changed for Emscripten 2.0
Disable stack protector for Emscript builds
As noted in emscripten-core/emscripten#9780, the upstream flavor of Emscripten does not yet support LLVM's stack protector so have it disabled. Otherwise linker will produce errors about missing symbols, because they're not there. Some day it may be implemented though so I leave a FIXME in the code.
Use new allocate() API of Emscripten
Themis Core uses the
allocate()function provided by Emscripten runtime for quick on-stack allocations. This is a semi-private function which has unstable API. With the new Emscripten version its API has changed, now it accepts a "slab" parameter which indicates the size of the stack that needs to be allocated—for the entire function. That is, if we allocate memory on stack,allocate()may be called only once.Stop using deprecated RESERVED_FUNCTION_POINTERS
RESERVED_FUNCTION_POINTERSdoes not have much effect on the upstream flavor but it was important for the fastcomp flavor. Now that since Emscripten 2.0.1 the fastcomp flavor is not supported anymore, this option has been a synonym forALLOW_TABLE_GROWTH. Use the new option instead directly then.Work around issues with dead code elimination
For some reason
wasm-ldlikes to strip all (each and every) function of Themis Core from the resulting *.wasm file. While they are there in the object files and archives, linker's dead code elimination thinks they are not needed in the WebAssembly code.Normally this is prevented by
EMSCRIPTEN_KEEPALIVEattribute—which we still have and it's still defined—but for some reason it does not work.As a temporary workaround, pass the
LINKABLEoption to the linker which disables dead code elimination and other link-time optimizations. This leads to somewhat bigger WebAssembly file which may be a bit slower, but at least this prevents issues.It might be some transient issue with a toolchain so there is a FIXME in the code. We'd need to keep an eye on it for some future releases. However, I have not found any similar issues in Emscripten tracker.
Checklist