library_async.js cleanup #11867
library_async.js cleanup #11867
Conversation
|
Great! Would you mind splitting the fastcomp deletion into its own PR and referencing #11860? |
|
Okay, I'll leave this PR for the minor cleanups and create a new one with fastcomp removal in a few minutes. |
src/library_async.js
Outdated
| @@ -20,228 +20,6 @@ mergeInto(LibraryManager.library, { | |||
| }, | |||
|
|
|||
| #if ASYNCIFY | |||
There was a problem hiding this comment.
Maybe we can also just move this into an assert at the top of the file, and always conditionally include this in src/modules.js?
There was a problem hiding this comment.
What do we do about the stub functions then?
There was a problem hiding this comment.
Oh I didn't see those. I guess we still need it then.
There was a problem hiding this comment.
Hmm.. can't we just remove those and let them turn into linker errors?
There was a problem hiding this comment.
Hm, it looks like src/modules.js already does this, actually: https://github.com/emscripten-core/emscripten/blob/master/src/modules.js#L136
I guess this means the stub functions are dead code then? Or are they still needed in case someone does -lasync.js or -s AUTO_JS_LIBRARIES without -s ASYNCIFY? A link-time error might be better in the -lasync.js case, and library_async.js could just be excluded from AUTO_JS_LIBRARIES... But I'm not sure if that might break anything.
There was a problem hiding this comment.
AUTO_JS_LIBRARIES is on by default, and only normally disabled as part of -s STRICT, so those stubs are still normally available to non ASYNCIFY users... but I'm not sure they should be. Such users should probably just get link error if they reference those functions. Although I could be missing some reason why a non-ASYNCIFY need those symbols to exist at link time?
There was a problem hiding this comment.
I have implemented stub removal in a separate commit for now 269a6e9, let's see if any tests fail. The only potential reason I see for keeping those symbols around is that maybe some user code happens to reference them in a dead code path that emscripten can't eliminate for some reason? But I really don't know. Maybe @kripken has something to say about this one.
There was a problem hiding this comment.
Yeah If we do that it can be followup for sure. Lets land this change as a small cleanup.
There was a problem hiding this comment.
Hm, I actually made the commit on this branch, but I guess I can split it off too if you want.
Akaricchi
force-pushed
the
async-cleanup
branch
from
ad8cfac to
269a6e9
Compare
* Fix subtle regression in emscripten_scan_registers introduced by emscripten-core#9859 * Replace some hardcoded offsets with C_STRUCT references * Remove spurious parameters from stub functions
Akaricchi
force-pushed
the
async-cleanup
branch
from
269a6e9 to
3b4caec
Compare
src/library_async.js
Outdated
| @@ -8,6 +8,10 @@ | |||
| // Async support via ASYNCIFY | |||
| // | |||
|
|
|||
| if (!ASYNCIFY) { | |||
| throw "To link with the async library you must use -s ASYNCIFY"; | |||
| } | |||
There was a problem hiding this comment.
I think you can do:
assert(ASYNCIFY, "To link with the async library you must use -s ASYNCIFY");
There was a problem hiding this comment.
Actually isn't it:
#if !ASYNCIFY
assert(false, "To link with the async library you must use -s ASYNCIFY");
#endif
|
The thing we haven't considered is code that uses |
…ng to library_async.js" This reverts commit 3b4caec.
Follow up to #11868
emscripten_scan_registersintroduced by Add new Fibers context-switching API #9859C_STRUCTreferences