Skip to content

Fix emscripten_get_heap_max's return value for 4GB - #14088

Merged
sbc100 merged 3 commits into
mainfrom
max
May 4, 2021
Merged

Fix emscripten_get_heap_max's return value for 4GB#14088
sbc100 merged 3 commits into
mainfrom
max

Conversation

@kripken

@kripken kripken commented May 4, 2021

Copy link
Copy Markdown
Member

The old value here wrapped to 0.

@sbc100

sbc100 commented May 4, 2021

Copy link
Copy Markdown
Collaborator

(we don't have test for this?)

@kripken

kripken commented May 4, 2021

Copy link
Copy Markdown
Member Author

Adding a test for this (which I realize we can, as you said - we can't allocate it, but we can test that we report it as the max), I found a bunch of issues here, all related. Added to here. New description:

Fix emscripten_get_heap_max's return value for 4GB (do not wrap to 0, return one
wasm page size less).

-1 is no longer the default of MAXIMUM_MEMORY, and in fact it is not possible
to pass it in (!) since we fail to parse things like "-1" in a place we expect a memory
size. So that has just been broken. Remove it from the docs, and remove it from
the code, which also simplifies things.

@sbc100

sbc100 commented May 4, 2021

Copy link
Copy Markdown
Collaborator

OK to land this once the ff tests pass?

@sbc100
sbc100 merged commit f7b403d into main May 4, 2021
@sbc100
sbc100 deleted the max branch May 4, 2021 23:09
sbc100 added a commit that referenced this pull request Jul 12, 2021
Without this change any user of ALLOW_MEMORY_GROWTH
with asan would get a rather cryptic error message from
the linker:

 wasm-ld: error: initial memory too small, 278278624 bytes needed

This is because asan increases the memory requirements in
proportion to the max memory, and the max memory defaults
to 2Gb with `ALLOW_MEMORY_GROWTH` enabled.

Also, remove check for `MAXIMUM_MEMORY` == -1 which is no longer
a thing since #14088.

Fixes: #14621
sbc100 added a commit that referenced this pull request Jul 13, 2021
Without this change any user of ALLOW_MEMORY_GROWTH
with asan would get a rather cryptic error message from
the linker:

 wasm-ld: error: initial memory too small, 278278624 bytes needed

This is because asan increases the memory requirements in
proportion to the max memory, and the max memory defaults
to 2Gb with `ALLOW_MEMORY_GROWTH` enabled.

Also, remove check for `MAXIMUM_MEMORY` == -1 which is no longer
a thing since #14088.

Fixes: #14621
sbc100 added a commit that referenced this pull request Jul 13, 2021
Without this change any user of ALLOW_MEMORY_GROWTH
with asan would get a rather cryptic error message from
the linker:

 wasm-ld: error: initial memory too small, 278278624 bytes needed

This is because asan increases the memory requirements in
proportion to the max memory, and the max memory defaults
to 2Gb with `ALLOW_MEMORY_GROWTH` enabled.

Also, remove check for `MAXIMUM_MEMORY` == -1 which is no longer
a thing since #14088.

Fixes: #14621
sbc100 added a commit that referenced this pull request Jul 13, 2021
Without this change any user of ALLOW_MEMORY_GROWTH
with asan would get a rather cryptic error message from
the linker:

 wasm-ld: error: initial memory too small, 278278624 bytes needed

This is because asan increases the memory requirements in
proportion to the max memory, and the max memory defaults
to 2Gb with `ALLOW_MEMORY_GROWTH` enabled.

Also, remove check for `MAXIMUM_MEMORY` == -1 which is no longer
a thing since #14088.

Fixes: #14621
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants