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.

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

[WIP] Avoid creating wrapper thunks or wasm exports #23411

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jan 15, 2025

In debug builds nothing really changes here since we exported functions still get statically assigned to debug wrappers/thunks.

However, after this change, in release builds, if you attempt to access _foo (internally) or Module['_foo'] externally before the module is loaded you will now get undefined.

Since it was always the case that calling these function would crash anyway, the only effected folks would be folks that take the reference early and then call later. e.g.

var myfunc = Module['_foo'];  // Run before module is loaded
// wait until loaded
myfunc();  // This will no longer work.

Fixes: #23339

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 15, 2025

Maybe this new behaviour should be behind -sSTRICT to avoid breaking folks?

In debug builds nothing really changes here since we exported functions
still get statically assigned to debug wrappers/thunks.

However, after this change, in release builds, if you attempt to access
`_foo` (internally) or `Module['_foo']` externally before the module
is loaded you will now get `undefined`.

Since it was always the case that calling these function would crash
anyway, the only effected folks would be folks that take the reference
early and then call later. e.g.

```
var myfunc = Module['_foo'];  // Run before module is loaded
// wait until loaded
myfunc();  // This will no longer work.
```

Fixes: emscripten-core#23339
@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 15, 2025

Or perhaps I can introduce a warning when folks do this, and let that cook for a few releases first?

@sbc100 sbc100 marked this pull request as draft January 15, 2025 00:52
@sbc100 sbc100 changed the title Avoid creating wrapper thunks or wasm exports [WIP] Avoid creating wrapper thunks or wasm exports Jan 15, 2025
@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 15, 2025

Marking as draft for now

@brendandahl
Copy link
Collaborator

It seems like if we do this, then debug builds should behave in a similar way. i.e. if you take a reference to a not ready function then that function should never work.

// Before the wasm module is loaded
var myFoo = Module['foo'];
Module['onRuntimeInitialized'] = () => {
  myFoo(); // <<< should not work in debug or opt builds
};

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 15, 2025

It seems like if we do this, then debug builds should behave in a similar way. i.e. if you take a reference to a not ready function then that function should never work.

// Before the wasm module is loaded
var myFoo = Module['foo'];
Module['onRuntimeInitialized'] = () => {
  myFoo(); // <<< should not work in debug or opt builds
};

Yes that is correct. The only difference I would propose is that maybe we can make the debug builds fail in a nicer way than the release builds:

  • release build: attempt to call undefined
  • debug build: abort("call to myFoo reference taken before module is ready")

That way at least we help the folks who were previously relying on this.

However, I'm not totally sure it worth breaking anybody here, which is why I marked this as a draft. Do many folks do you think would be broken if that above code (which works today) stops working?

@brendandahl
Copy link
Collaborator

I feel like most the code I've seen people usually call through the module object, but I've mainly looked at g3 code. We could try testing this in g3 too.

At least if it does break someone, the error is not a subtle one and should be relatively easy to fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stop using re-assignment thunks when WASM_ASYNC_COMPILATION is enabled.
2 participants