-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set what were default values for Web platform linker flags -sSTACK_SIZE
and -sDEFAULT_PTHREAD_STACK_SIZE
#86036
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So from what I can tell, where we break is here:
extern zipFile ZEXPORT zipOpen2(const char *pathname, int append, zipcharpc* globalcomment, zlib_filefunc_def* pzlib_filefunc32_def) {
if (pzlib_filefunc32_def != NULL)
{
zlib_filefunc64_32_def zlib_filefunc64_32_def_fill;
fill_zlib_filefunc64_32_def_from_filefunc32(&zlib_filefunc64_32_def_fill,pzlib_filefunc32_def);
return zipOpen3(pathname, append, globalcomment, &zlib_filefunc64_32_def_fill); // <-- here
}
else
return zipOpen3(pathname, append, globalcomment, NULL);
}
Without these changes the values on stack in WASM when this call happens are
$var0: i32 {value: 24090136}
$var1: i32 {value: 0}
$var2: i32 {value: 0}
$var3: i32 {value: 63768}
$var4: i32 {value: 63696}
And with these changes its like this
$var0: i32 {value: 49976928}
$var1: i32 {value: 0}
$var2: i32 {value: 0}
$var3: i32 {value: 5241112}
$var4: i32 {value: 5241040}
So it seems we're allocating exactly what the amount of STACK_SIZE
specified for these last two variables. One of which seems to be the zlib_filefunc64_32_def
struct, which is a collection of function pointers. Seems like 64 kb just doesn't fit this amount of pointers. But I guess 5 mb may be a bit too much and we can tweak and reduce the number if needed.
All that said, this fixes the issue in my testing.
The fix is good, but the problem is that it's going to break compiling with Emscripten < 3.1.30, since it doesn't know those linker options. I tried with 3.1.18:
We should add the options conditionally based on the Emscripten version. |
Fix: diff --git a/platform/web/detect.py b/platform/web/detect.py
index ddfd3515f9..c2c1b28766 100644
--- a/platform/web/detect.py
+++ b/platform/web/detect.py
@@ -186,10 +186,15 @@ def configure(env: "Environment"):
env["LIBPREFIXES"] = ["$LIBPREFIX"]
env["LIBSUFFIXES"] = ["$LIBSUFFIX"]
+ # Get version info for checks below.
+ cc_version = get_compiler_version(env)
+ cc_semver = (cc_version["major"], cc_version["minor"], cc_version["patch"])
+
env.Prepend(CPPPATH=["#platform/web"])
env.Append(CPPDEFINES=["WEB_ENABLED", "UNIX_ENABLED"])
- env.Append(LINKFLAGS=["-s", "STACK_SIZE=5MB"])
+ if cc_semver >= (3, 1, 27):
+ env.Append(LINKFLAGS=["-s", "STACK_SIZE=5MB"])
if env["opengl3"]:
env.AppendUnique(CPPDEFINES=["GLES3_ENABLED"])
@@ -205,14 +210,11 @@ def configure(env: "Environment"):
env.Append(CPPDEFINES=["PTHREAD_NO_RENAME"])
env.Append(CCFLAGS=["-s", "USE_PTHREADS=1"])
env.Append(LINKFLAGS=["-s", "USE_PTHREADS=1"])
- env.Append(LINKFLAGS=["-s", "DEFAULT_PTHREAD_STACK_SIZE=2MB"])
+ if cc_semver >= (3, 1, 30):
+ env.Append(LINKFLAGS=["-s", "DEFAULT_PTHREAD_STACK_SIZE=2MB"])
env.Append(LINKFLAGS=["-s", "PTHREAD_POOL_SIZE=8"])
env.Append(LINKFLAGS=["-s", "WASM_MEM_MAX=2048MB"])
- # Get version info for checks below.
- cc_version = get_compiler_version(env)
- cc_semver = (cc_version["major"], cc_version["minor"], cc_version["patch"])
-
if env["lto"] != "none":
# Workaround https://github.com/emscripten-core/emscripten/issues/19781.
if cc_semver >= (3, 1, 42) and cc_semver < (3, 1, 46): |
Prior to 3.1.25 it was called |
e7c53f8
to
1917b6a
Compare
Updated with my diff + @YuriSizov's suggestion. |
platform/web/detect.py
Outdated
if cc_semver >= (3, 1, 30): | ||
env.Append(LINKFLAGS=["-s", "DEFAULT_PTHREAD_STACK_SIZE=2MB"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this may not need to be version specific, they just changed the default in 3.1.30 but the flag existed before, so we can always set it.
- Set `-sSTACK_SIZE` to what it was before emscripten 3.1.27. It was renamed in 3.1.25 so also set `-sTOTAL_SIZE` for older versions for consistency. - Set `-sDEFAULT_PTHREAD_STACK_SIZE` to what it was before 3.1.30. Co-authored-by: Rémi Verschelde <[email protected]>
1917b6a
to
8e5fbd4
Compare
Thanks! |
Cherry-picked for 4.2.1. |
-sSTACK_SIZE
and -sDEFAULT_PTHREAD_STACK_SIZE
-sSTACK_SIZE
and -sDEFAULT_PTHREAD_STACK_SIZE
Cherry-picked for 4.1.4. |
Cherry-picked for 3.6. |
Cherry-picked for 3.5.4. |
Emscripten did some changes regarding some linkerflags.
-sSTACK_SIZE
from 5MB to 64KB and-sDEFAULT_PTHREAD_STACK_SIZE
from 2MB to 64KB on 3.1.27.-sDEFAULT_PTHREAD_STACK_SIZE
to match-sSTACK_SIZE
on 3.1.30.This PR sets back both linkerflags to their original value for now on.
Fixes #85564