Skip to content

JS type legalization is still performed regardless of LEGALIZE_JS_FFI #21592

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
sbc100 wants to merge 1 commit into from
Closed

JS type legalization is still performed regardless of LEGALIZE_JS_FFI #21592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 16 additions & 9 deletions tools/emscripten.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,10 @@ def finalize_wasm(infile, outfile, js_syms):
# if we don't need to modify the wasm, don't tell finalize to emit a wasm file
modify_wasm = False

# If we are doing any JS type legalization (needed for passing i64 values).
# This is only fully disabled when WASM_BIGINT is available.
legalizing = True

if settings.WASM2JS:
# wasm2js requires full legalization (and will do extra wasm binary
# later processing later anyhow)
Expand All @@ -497,6 +501,8 @@ def finalize_wasm(infile, outfile, js_syms):
args.append('-g')
if settings.WASM_BIGINT:
args.append('--bigint')
legalizing = False

if settings.DYNCALLS:
# we need to add all dyncalls to the wasm
modify_wasm = True
Expand All @@ -507,19 +513,20 @@ def finalize_wasm(infile, outfile, js_syms):
args.append('--dyncalls-i64')
# we need to add some dyncalls to the wasm
modify_wasm = True

if settings.AUTODEBUG:
# In AUTODEBUG mode we want to delay all legalization until later. This is hack
# to force wasm-emscripten-finalize not to do any legalization at all.
args.append('--bigint')
else:
if settings.LEGALIZE_JS_FFI:
# When we dynamically link our JS loader adds functions from wasm modules to
Copy link
Collaborator Author

@sbc100 sbc100 Mar 22, 2024

Choose a reason for hiding this comment

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

This comment was removed becasue it basically duplicate of the one in js_legalization_pass_flags

# the table. It must add the original versions of them, not legalized ones,
# so that indirect calls have the right type, so export those.
args += building.js_legalization_pass_flags()
modify_wasm = True
else:
args.append('--no-legalize-javascript-ffi')
legalizing = False

if not settings.LEGALIZE_JS_FFI:
Copy link
Member

@kripken kripken Mar 22, 2024

Choose a reason for hiding this comment

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

Can we not use settings.LEGALIZE_JS_FFI where the new legalizing variable is set? That is, I'm not sure why a new variable is needed here.

Copy link
Collaborator Author

@sbc100 sbc100 Mar 22, 2024

Choose a reason for hiding this comment

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

The reason is that LEGALIZE_JS_FFI does not mean "don't do any legalization at all", it just means "do partial legalization".

See https://github.com/WebAssembly/binaryen/blob/2471301a5209724b1ea32fab36b13410e96c0af9/src/tools/wasm-emscripten-finalize.cpp#L262-L267

At least today.

We could change that but I think we can/should consider that separately.

args.append('--no-legalize-javascript-ffi')

if legalizing:
args += building.js_legalization_pass_flags()
modify_wasm = True

if settings.SIDE_MODULE:
args.append('--side-module')
if settings.STACK_OVERFLOW_CHECK >= 2:
Expand Down
8 changes: 5 additions & 3 deletions tools/link.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1495,6 +1495,11 @@ def phase_linker_setup(options, state, newargs):

if settings.WASM_BIGINT:
settings.LEGALIZE_JS_FFI = 0
else:
# These symbols are needed for the JS API legalization pass in emscripten-wasm-finalize
# Legalization is only *fully* disabled when bigint is available. LEGALIZE_JS_FFI=0 does not
# disable all legalization.
settings.REQUIRED_EXPORTS += ['__get_temp_ret', '__set_temp_ret']

if settings.SINGLE_FILE:
settings.GENERATE_SOURCE_MAP = 0
Expand All @@ -1519,9 +1524,6 @@ def phase_linker_setup(options, state, newargs):
if settings.AUTODEBUG:
settings.REQUIRED_EXPORTS += ['setTempRet0']

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

if settings.SPLIT_MODULE and settings.ASYNCIFY == 2:
settings.DEFAULT_LIBRARY_FUNCS_TO_INCLUDE += ['_load_secondary_module']

Expand Down