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

[juj]

Do not pass --enable-bulk-memory in Binaryen when -mno-bulk-memory is used, or -sMIN_x_VERSION is too low to enable bulk memory.

Without this, running e.g.

emcc test\hello_world.c -o a.js -O3 -mno-bulk-memory -mno-bulk-memory-opt

would still produce a final output that has bulk memory enabled.

With this I was able to complete the browser suite with EMCC_CFLAGS=-mno-bulk-memory -mno-bulk-memory-opt.

[sbc100]

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?

[juj]

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.

[sbc100]

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

[sbc100]

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?

[juj]

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.

[sbc100]

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

[juj]

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.

[sbc100]

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

[sbc100]

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

[dschuff]

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.

[juj]

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?

[dschuff]

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.

[sbc100]

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?

[juj]

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.

[sbc100]

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

[juj]

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.