Add functions for access emscripten stack layout information #11437
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
sbc100
changed the title
kripken
approved these changes
Jun 17, 2020
tests/core/test_stack_get_free.c
Outdated
|
|
||
| uintptr_t prevFree = emscripten_stack_get_free(); | ||
| for(int i = 0; i < 10; ++i) { | ||
| void *p = alloca(emscripten_random() >= 0 ? 256*1024 : 255 * 1024); |
There was a problem hiding this comment.
the use of random here seems odd. is this to confuse the optimizer?
(also, spaces around * look inconsistent)
There was a problem hiding this comment.
I just copied this test from the other PR :) But yes I agree its strange.. @juj, why this?
This change adds emscripten_stack_get_base/free/end/current. This is a cut down version of #11162 which uses wasm assembly rather than relying on binaryen changes. Also remove old emscripten_get_stack_base/emscripten_get_stack_top from src/library.js. emscripten_get_stack_top never worked anyway since its was not marked as asm and therefore not referencing the arm-internal version of STACKTOP.
juj
reviewed
Jul 28, 2020
| if | ||
| call emscripten_stack_get_end | ||
| global.set __stack_end | ||
| end_if |
There was a problem hiding this comment.
Hey, thanks for working on this!
Having this in a .s file is a cool approach. However, this if branch makes this suboptimal compared to the solution that was presented in #11162 + WebAssembly/binaryen#2849 that did not need the branch :( (the original solution was just a single sub.
Any change we could get rid of this comparison?
There was a problem hiding this comment.
Sure, that sounds perfectly reasonable. Should be trivial change. I'm OK doing it, or you do you want to?
There was a problem hiding this comment.
Thanks, if you have the cycles, that would be much appreciated!
Closed
sbc100
pushed a commit
that referenced
this pull request
Jul 31, 2020
This is a followup on #11437 which added this API. Sadly today the linker doesn't know the final memory layout so until we fix #11773 this extra `init` function is needed. The alternative, which is to modify binaryen and emscripten to inject the stack end value post-link is IMHO way to much complexity to justify the convenience of not having the call this init function. Once we fix #11773 the init function can become uneeded/no-op.
sbc100
pushed a commit
that referenced
this pull request
Aug 5, 2020
This is a followup on #11437 which added this API. Sadly today the linker doesn't know the final memory layout so until we fix #11773 this extra `init` function is needed. The alternative, which is to modify binaryen and emscripten to inject the stack end value post-link is IMHO way to much complexity to justify the convenience of not having the call this init function. Once we fix #11773 the init function can become uneeded/no-op.
sbc100
pushed a commit
that referenced
this pull request
Aug 6, 2020
This is a followup on #11437 which added this API. Sadly today the linker doesn't know the final memory layout so until we fix #11773 this extra `init` function is needed. The alternative, which is to modify binaryen and emscripten to inject the stack end value post-link is IMHO way to much complexity to justify the convenience of not having the call this init function. Once we fix #11773 the init function can become uneeded/no-op.
sbc100
pushed a commit
that referenced
this pull request
Aug 6, 2020
This is a followup on #11437 which added this API. Sadly today the linker doesn't know the final memory layout so until we fix #11773 this extra `init` function is needed. The alternative, which is to modify binaryen and emscripten to inject the stack end value post-link is IMHO way to much complexity to justify the convenience of not having the call this init function. Once we fix #11773 the init function can become uneeded/no-op.
sbc100
pushed a commit
that referenced
this pull request
Aug 6, 2020
This is a followup on #11437 which added this API. Sadly today the linker doesn't know the final memory layout so until we fix #11773 this extra `init` function is needed. The alternative, which is to modify binaryen and emscripten to inject the stack end value post-link is IMHO way to much complexity to justify the convenience of not having the call this init function. Once we fix #11773 the init function can become uneeded/no-op.
sbc100
added a commit
that referenced
this pull request
Aug 7, 2020
This is a followup on #11437 which added this API. Sadly today the linker doesn't know the final memory layout so until we fix #11773 this extra `init` function is needed. The alternative, which is to modify binaryen and emscripten to inject the stack end value post-link is IMHO way to much complexity to justify the convenience of not having the call this init function. Once we fix #11773 the init function can become unneeded/no-op.
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.
This change adds emscripten_stack_get_base/free/end/current.
This is a cut down version of #11162 which uses wasm assembly
rather than relying on binaryen changes.