Skip to content

Do not pass --enable-bulk-memory in Binaryen when -mno-bulk-memory is used #25466

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

Closed
wants to merge 4 commits into from
Closed

Do not pass --enable-bulk-memory in Binaryen when -mno-bulk-memory is used #25466

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
80e69d1
Do not pass --enable-bulk-memory in Binaryen when -mno-bulk-memory is…
juj Oct 2, 2025
2c07a3f
Avoid referring to settings.BULK_MEMORY. Add a test.
juj Oct 2, 2025
153d61d
Skip MEMORY64=1 mode
juj Oct 2, 2025
3967df6
Skip esm_integration.test_no_bulk_memory
juj Oct 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions test/test_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2757,6 +2757,22 @@ def test_copyop(self):
# (llvm-gcc copies items one by one).
self.do_core_test('test_copyop.cpp')

# Verify that when passing -mno-bulk-memory -mno-bulk-memory-opt, that the
# resulting .wasm module should not have the bulk-memory Data Count section.
@no_wasm2js('This test verifies .wasm module behavior')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an existing test_wasm_features test that is supposed to code this kind of thing. Maybe this can be added to that existing test? Or at least co-located with it and named similarly to it?

Copy link
Collaborator Author

@juj juj Oct 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I placed this in the core test suite, because it looks like this is optimization flag dependent. I.e. it does not fail in -O0, but fails in -Os and -O3. So testing different optimization modes seems in order.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe test_wasm_features should be in core for the same reasons then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe..

I'll close this out for now, I get an impression that the fix is in wrong place, and the test is in wrong place. I'll pick my battles to focus progress in my testing effort.

@no_esm_integration('TODO: Currently fails')
def test_no_bulk_memory(self):
if self.get_setting('MEMORY64') == 1:
self.skipTest('Wasm64 always has bulk-memory')
if self.get_setting('MEMORY64') == 2:
self.skipTest('TODO: Currently fails')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's any reason to support a configuration with memory64 but without bulk memory, so this probably doesn't need to be a TODO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That corresponds to the wasm64l suite, i.e. "2 is wasm64 for clang/lld but lowered to wasm32 in Binaryen (such that it can run on wasm32 engines, while internally using i64 pointers)."

To my understanding that should "technically" work?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, that could technically work. real memory64 without bulk could technically work too of course, but I've never heard of any implementation like that, nor can I think of any reason anyone would do that. Wasm64l is a very niche use case, but at least I can imagine why someone might use it in theory.


self.emcc(test_file('hello_world.c'), ['-mno-bulk-memory', '-mno-bulk-memory-opt'])

with webassembly.Module('a.out.wasm') as module:
for s in module.sections():
self.assertNotEqual(s.type, webassembly.SecType.DATACOUNT)

@parameterized({
'': ([],),
'bulkmem': (['-mbulk-memory'],),
Expand Down
7 changes: 7 additions & 0 deletions tools/emscripten.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
from tools import building
from tools import config
from tools import diagnostics
from tools import feature_matrix
from tools import js_manipulation
from tools import shared
from tools import utils
Expand Down Expand Up @@ -116,6 +117,12 @@ def update_settings_glue(wasm_file, metadata, base_metadata):

# start with the MVP features, and add any detected features.
settings.BINARYEN_FEATURES = ['--mvp-features'] + metadata.features
if not feature_matrix.caniuse(feature_matrix.Feature.BULK_MEMORY):
if '--enable-bulk-memory' in settings.BINARYEN_FEATURES:
settings.BINARYEN_FEATURES.remove('--enable-bulk-memory')
if '--enable-bulk-memory-opt' in settings.BINARYEN_FEATURES:
settings.BINARYEN_FEATURES.remove('--enable-bulk-memory-opt')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is handled elsewhere. In get_binaryen_passes we have:

  if not feature_matrix.caniuse(feature_matrix.Feature.BULK_MEMORY):                                      
    logger.debug('lowering bulk-memory feature due to incompatible target browser engines')               
    passes += ['--llvm-memory-copy-fill-lowering']

Which should lower it away... but maybe its not working as intended?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if that would only lower the instructions, but the extra Wasm sections still remain in the generated code, which results in pre-bulk memory browsers not being able to parse the .wasm module.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case maybe the lowing pass is broken!? Seems odd that we wouldn't have test coverage for that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What extra sections are you referring to? IIRC that only time llvm uses bulk memory is to copying and and filling withing the normal linear memory?

Or perhaps you mean the use of bulk memory in pthreads init code? Does pthreads not have a hard dependency on bulk memory? Are there targets that support pthreads/SAB but not bulk memory? IIRC there are not?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe.. The concrete error that Firefox 78 ESR then complains is that the generated .wasm file has a Data Count section, i.e. in the case of the above emcc cmdline, there are

Section TYPE starts at position 8
Section IMPORT starts at position 51
Section FUNCTION starts at position 60
Section TABLE starts at position 70
Section MEMORY starts at position 77
Section GLOBAL starts at position 85
Section EXPORT starts at position 95
Section ELEMENT starts at position 110
Section DATA_COUNT starts at position 121
Section CODE starts at position 124
Section DATA starts at position 1691

and Firefox 78 ESR croaks at byte 121.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any any case it doesn't look like this is the right place to try and to lowing passes / removing of features.

I think this should go with the existing lowering bulk-memory feature due to incompatible target browser engines line in get_binaryen_passes

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem to be a binaryen pass selection/activation problem. get_binaryen_passes already has

  if not feature_matrix.caniuse(feature_matrix.Feature.BULK_MEMORY):
    logger.debug('lowering bulk-memory feature due to incompatible target browser engines')
    passes += ['--llvm-memory-copy-fill-lowering']

but that doesn't seem to be enough.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But is there some other flag you can add to --llvm-memory-copy-fill-lowering to have binaryen not emit the data coune section?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kripken do you know that best place / way to do force the removal of the data count section?


if settings.ASYNCIFY == 2:
settings.BINARYEN_FEATURES += ['--enable-reference-types']

Expand Down