Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Do we still need EMIT_EMSCRIPTEN_METADATA? #12231

Closed
sbc100 opened this issue Sep 15, 2020 · 8 comments · Fixed by #15319
Closed

Do we still need EMIT_EMSCRIPTEN_METADATA? #12231

sbc100 opened this issue Sep 15, 2020 · 8 comments · Fixed by #15319

Comments

@sbc100
Copy link
Collaborator

sbc100 commented Sep 15, 2020

When this was originally conceived it was necessary because the embedder required
access to information about the memory and table layout that was not otherwise
available. Emscripten would extra this information during wasm-emscripten-finalize
but other tools couldn't do that.

Since then we have moved away from a model that required the embedder to know
the memory layout and sbrk() and malloc() are now always fully internal to the wasm
module.

For this reason I'm proposing the remove the memory layout information from the
metadata section (because actually we are moving to world emscripten doesn't know
this statically either), and possible the entire EMIT_EMSCRIPTEN_METADATA
option?
@sbc100 sbc100 mentioned this issue Sep 15, 2020
Closed
@sbc100
Copy link
Collaborator Author

sbc100 commented Sep 15, 2020

@rianhunter what do you think?

@sbc100 sbc100 mentioned this issue Sep 18, 2020
Closed
@sbc100 sbc100 mentioned this issue Oct 20, 2020
Closed
@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 21, 2020

@AndrewScheidecker: Apparently wavm is relying (or reportedly relying) on emscripten_metadata. Can we work to remove this restriction?

Do you know of any remaining reasons why you still need the emscripten_metadata today? I'm hoping modern emscripten-built standalone wasm modules don't need this to be executed.

sbc100 added a commit that referenced this issue Oct 29, 2020
@sbc100
Read staticBump from dylink section
23c5e74 
This allows us to stop depending wasm-emscripten-finalize to
write staticBump which in turn will allow us to stop exporting
`__data_end`.

As a side effect of this the staticBump information is no
longer available in the normal linking mode which means we
can't include dynamic_base in EMSCRIPTEN_METADATA anymore.  One
more nail in the coffin for EMSCRIPTEN_METADATA. See #12231
@sbc100 sbc100 mentioned this issue Oct 29, 2020
Merged
sbc100 added a commit that referenced this issue Oct 29, 2020
@sbc100
Read staticBump from dylink section
228419c 
This allows us to stop depending wasm-emscripten-finalize to
write staticBump which in turn will allow us to stop exporting
`__data_end`.

As a side effect of this the staticBump information is no
longer available in the normal linking mode which means we
can't include dynamic_base in EMSCRIPTEN_METADATA anymore.  One
more nail in the coffin for EMSCRIPTEN_METADATA. See #12231
sbc100 added a commit that referenced this issue Oct 29, 2020
@sbc100
Read static data size from dylink section
7d07270 
This allows us to stop depending wasm-emscripten-finalize to
write staticBump which in turn will allow us to stop exporting
`__data_end`, once we update binaryen.

As a side effect of this the static bump information is no
longer available in the normal linking mode which means we
can't include dynamic_base in EMSCRIPTEN_METADATA anymore.  One
more nail in the coffin for EMSCRIPTEN_METADATA. See #12231
sbc100 added a commit that referenced this issue Oct 29, 2020
@sbc100
Read static data size from dylink section
ff503a6 
This allows us to stop depending wasm-emscripten-finalize to
write staticBump which in turn will allow us to stop exporting
`__data_end`, once we update binaryen.

As a side effect of this the static bump information is no
longer available in the normal linking mode which means we
can't include dynamic_base in EMSCRIPTEN_METADATA anymore.  One
more nail in the coffin for EMSCRIPTEN_METADATA. See #12231
sbc100 added a commit that referenced this issue Oct 29, 2020
@sbc100
Read static data size from dylink section (#12643)
b0e2a5f 
This allows us to stop depending wasm-emscripten-finalize to
write staticBump which in turn will allow us to stop exporting
`__data_end`, once we update binaryen.

As a side effect of this the static bump information is no
longer available in the normal linking mode which means we
can't include dynamic_base in EMSCRIPTEN_METADATA anymore.  One
more nail in the coffin for EMSCRIPTEN_METADATA. See #12231
@AndrewScheidecker
Copy link

I changed WAVM to not require emscripten_metadata. It didn't have a real need for it before, it was just using it to produce a user friendly error if STANDALONE_WASM wasn't used, or it had an unexpected ABI version.

@ERICXUCHI
Copy link

Now WAVM still needs -s EMIT_EMSCRIPTEN_METADATA=1 -s STANDALONE_WASM=1, it seems nothing changed.

@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 14, 2021

In what sense does it still need this setting? Does it stop working you don't specify this setting? How exactly?

@ERICXUCHI
Copy link

In what sense does it still need this setting? Does it stop working you don't specify this setting? How exactly?

wavm version
WAVM version 0.0.0-prerelease
WAVM_ENABLE_RUNTIME:           true
WAVM_ENABLE_STATIC_LINKING:    false
WAVM_ENABLE_ASAN:              false
WAVM_ENABLE_UBSAN:             false
WAVM_ENABLE_TSAN:              false
WAVM_ENABLE_LIBFUZZER:         false
WAVM_ENABLE_RELEASE_ASSERTS:   false
WAVM_ENABLE_UNWIND:            true


emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 2.0.31 (4fcbf0239ccca29771f9044c990b0d34fac6e2e7)
clang version 14.0.0 (https://github.com/llvm/llvm-project 8fa2394bad433558f3083cee158043e2fb66d781)
Target: wasm32-unknown-emscripten
Thread model: posix

We complie a helloworld.c by emscripten, then run the a.out.wasm by WAVM.
image
Something wrong:

Module appears to be an Emscripten module, but does not have an 'emscripten_metadata' section. WAVM only supports Emscripten modules compiled with '-s EMIT_EMSCRIPTEN_METADATA=1'.
If this is not an Emscripten module, please use '--abi=<ABI>' on the WAVM command line to specify the correct ABI.

After I add -s EMIT_EMSCRIPTEN_METADATA=1, then WAVM reports WAVM only supports Emscripten modules compiled with '-s STANDALONE_WASM=1'., after I add these two options, WAVM runs smoothly and print Hello World finally.

@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 14, 2021

Ah yes, it looks like wavm needs to have the check removed still.

@AndrewScheidecker can you reply here when the check is removed and then I can go ahead and remove the metadata stuff from the emscripten side?

@AndrewScheidecker
Copy link

Thanks for the report @ERICXUCHI. I previously made it work, but I missed an error if you don't explicitly specify an ABI on the wavm run command-line. WAVM/WAVM@9ffd3e2 should make it work without the metadata section and without specifying an ABI explicitly on the command-line.

sbc100 added a commit that referenced this issue Oct 18, 2021
@sbc100
Remove EMSCRIPTEN_METADATA settings
31c7a0f 
We have been warning about the deprecation of this setting
for about a year now (since 2.0.9).

As explained in #12231, the major use cases for this metadata
are no longer valid since the embedder should on longer need to
know the memory layout of the module (since the memory and table
are both created and setup within the wasm module these days).

Fixes: #12231
@sbc100 sbc100 mentioned this issue Oct 18, 2021
Merged
sbc100 added a commit that referenced this issue Oct 18, 2021
@sbc100
Remove EMSCRIPTEN_METADATA settings
8492391 
We have been warning about the deprecation of this setting
for about a year now (since 2.0.9).

As explained in #12231, the major use cases for this metadata
are no longer valid since the embedder should on longer need to
know the memory layout of the module (since the memory and table
are both created and setup within the wasm module these days).

Fixes: #12231
sbc100 added a commit that referenced this issue Oct 18, 2021
@sbc100
Remove EMIT_EMSCRIPTEN_METADATA settings
0facc66 
We have been warning about the deprecation of this setting
for about a year now (since 2.0.9).

As explained in #12231, the major use cases for this metadata
are no longer valid since the embedder should on longer need to
know the memory layout of the module (since the memory and table
are both created and setup within the wasm module these days).

Fixes: #12231
sbc100 added a commit that referenced this issue Oct 18, 2021
@sbc100
Remove EMIT_EMSCRIPTEN_METADATA settings
9fe5470 
We have been warning about the deprecation of this setting
for about a year now (since 2.0.9).

As explained in #12231, the major use cases for this metadata
are no longer valid since the embedder should on longer need to
know the memory layout of the module (since the memory and table
are both created and setup within the wasm module these days).

Fixes: #12231
sbc100 added a commit that referenced this issue Oct 18, 2021
@sbc100
Remove EMIT_EMSCRIPTEN_METADATA settings (#15319)
5a442a3 
We have been warning about the deprecation of this setting
for about a year now (since 2.0.9).

As explained in #12231, the major use cases for this metadata
are no longer valid since the embedder should on longer need to
know the memory layout of the module (since the memory and table
are both created and setup within the wasm module these days).

Fixes: #12231
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove EMIT_EMSCRIPTEN_METADATA settings emscripten-core/emscripten
3 participants
@sbc100 @AndrewScheidecker @ERICXUCHI