Remove assert for __get_temp_ret() and __set_temp_ret() after Binaryen JS FFI legalizer pass

[juj]

Binaryen JS FFI legalizer pass seems to be dropping __get_temp_ret() and __set_temp_ret() if they are unused, so do not emit assert() for their existence. Fixes #25549.

I am not 100% sure about this, but this did fix the test wasm2js0.test_autodebug_wasm to pass.

[sbc100]

Can you add wasm2js0.test_autodebug_wasm to the core3+extras list in .circleci/config.yml?

We run all the tests in wasm2js1 mode in CI.. i wonder why opt level zero would mean these exports don't exist but they do exist at opt level 1 and above? I would imagine it would be other way around.

[sbc100]

I think the run_binaryen_command wrapper is supposed to take are of adding this (or not) at the appropriate times.

[juj]

It is/was not happening at least for this FFI JS run pass. Adding -g manually here does help the output retain debug info.

[juj]

Can you add wasm2js0.test_autodebug_wasm to the core3+extras list in .circleci/config.yml?

Check.

i wonder why opt level zero would mean these exports don't exist but they do exist at opt level 1 and above? I would imagine it would be other way around.

I don't think they exist in any wasm2js* level after the JS FFI legalization wasm-opt pass runs (they exist in the output before the legalization wasm-opt run, but not after). But wasm2js0 is the only build that has assert()s that the exports would exist. Which is why it only trips up in debug builds.

[sbc100]

We have this code elsewhere:

  if settings.LEGALIZE_JS_FFI:                                                                            
    settings.REQUIRED_EXPORTS += ['__get_temp_ret', '__set_temp_ret']  

Which means wasm-ld will always export them when LEGALIZE_JS_FFI.

If they are then removed at some point (for example by DCE) then that should be not different any other removed exports. i.e. the list of exports here should already take into account any DCE.

[juj]

then that should be not different any other removed exports

They still go to the list of exports to generate assert()s for, which is why the test case fails. I don't know why they aren't removed there, except to hypothesize that is because they are removed by a separate FFI JS wasm-opt execution, rather than the general wasm-opt code optimization pass.

[sbc100]

Indeed, but I think we should understand why we need a special case for them here, since we have many other symbols that get added to settings.REQUIRED_EXPORTS but that are potentially unused and get DCE'd.

I'd rather not make special case here just for these symbols if it can be avoided.

[juj]

The reason is that it is the later phase_binaryen JS FFI legalizer call pass that optimizes those out, rather than the wasm finalizer metadce optimizer in phase_emscript that generally DCEs imports/exports.

That is:

In the emscript phase:

When finalize_wasm wasm-opt pass is called here, the output Wasm module still exports __get_temp_ret, which is correctly recorded out in metadata, and accurate in the .wasm file.. it has that export.

The next phase after the emscript phase is the binaryen phase. That phase is what calls to wasm-opt:

'C:/emsdk/binaryen/main_vs2022_64bit_binaryen\bin\wasm-opt' --instrument-locals --log-execution --instrument-memory --legalize-js-interface --pass-arg=legalize-js-interface-exported-helpers 'C:\Users\clb\AppData\Local\Temp\emscripten_temp\test_autodebug.wasm' -o 'C:\Users\clb\AppData\Local\Temp\emscripten_temp\test_autodebug.wasm' --mvp-features --enable-bulk-memory --enable-bulk-memory-opt --enable-call-indirect-overlong --enable-multivalue --enable-mutable-globals --enable-nontrapping-float-to-int --enable-reference-types --enable-sign-ext

and legalizes the JS FFI interface.

This phase deletes the __get_temp_ret Wasm export in the test.

But the metadata is still from the earlier emscript phase, which has the __get_temp_ret Wasm export. So the later JS emit phase will incorrectly emit an assert for it.

[sbc100]

This sounds like where we should fix it then. I don't think the legalize phase should be going around deleting exports, unless we tell it to.

Free free to disable tests until we can address this in binaryen.

[sbc100]

Even if we remove the assert the resulting JS still thinks that these exports exist:

For example, I see the following in the generated code:

___get_temp_ret = createExportWrapper('__get_temp_ret', 0);                    
___set_temp_ret = createExportWrapper('__set_temp_ret', 1);   

Either we should remove them completely after wasm-opt removes them or better still, wasm-opt should probably leave them alone and let DCE processes handle removing then just like any other unused exports.

[sbc100]

OK, so here is the code in binaryen that deletes the exports:

https://github.com/WebAssembly/binaryen/blob/1e36622832c32ad335403065c322781982f1b55c/src/passes/LegalizeJSInterface.cpp#L165-L166

And we even mention that here:

emscripten/system/lib/compiler-rt/emscripten_tempret.s

Lines 21 to 23 in 7645977

	# These aliases exist for LegalizeJSInterface pass in binaryen
	# They get exported by emcc and the exports are then removed by the
	# binaryen pass

However, I think maybe the right solution is to leave those exports alone and have DCE delete.

If that seems like too much work I think we should just these elements from function_exports when we create it in the emscript function.

[juj]

Ok, I'll drop this for now, and skip the test. #26793

[juj]

Not needed anymore.