-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Fix STACKTOP/STACK_MAX variables for relocatable #8434
Fix STACKTOP/STACK_MAX variables for relocatable #8434
Conversation
Looks good, thanks! |
@@ -484,7 +484,7 @@ def create_module_asmjs(function_table_sigs, metadata, | |||
asm_runtime_thread_local_vars = create_asm_runtime_thread_local_vars() | |||
|
|||
stack = '' | |||
if not (shared.Settings.WASM and shared.Settings.SIDE_MODULE): | |||
if not shared.Settings.RELOCATABLE and not (shared.Settings.WASM and shared.Settings.SIDE_MODULE): |
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.
IIUC this is redundant.
If we apply De Morgan's here I a little clearer to see:
if shared.Settings.RELOCATABLE or (shared.Settings.WASM and shared.Settings.SIDE_MODULE):
Since RELOCATABLE is always set for SIDE_MODULE the second clause is redundant I think?
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.
In theory, we may want to allow non-relocatable dynamic modules (i.e., loadable at runtime, but at known-beforehand locations in memory/table). At least it would be nice to not rule that out for now, I think. So checking for relocation separately makes sense in my opinion.
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.
Yeah, that's how my original working diff was. I changed it to mirror the logic in the other part of the diff when I realized that other change was necessary.
…core#8434)" This reverts commit a365dba.
…core#8434)" This reverts commit a365dba.
STACKTOP
andSTACK_MAX
were being output twice in relocatable modules. Fixes #8433