-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Unify more of the minimal runtime and normal runtime initialization code. #23488
base: main
Are you sure you want to change the base?
Conversation
…ode. This is a first step in trying to unify more of the code used by minimal runtime and the normal runtime and also move all addOn* functions into a library. First, make the call to the static constructors use the same code for both modes. With this change, it becomes necessary to have an injection point that runs before ctors and after ctors. To achieved that, I've replaced the unused ATMAIN with new ATPOSTINIT code injection point. The new ATPOSTINIT is similar to the old ATMAIN in that it runs after the static constructors, but it now runs even if there is no main function. Second, the FS initialization is now split into two parts (similar to how it used to be before a refactoring). 1) atInit the std<X> stream are setup. This allows ctors to use printf etc. 2) atPostInit the FS is locked down as late as possible but before main is run. Third, switch embind code generation to use the POSTINIT so it can run after the ctors have setup the bindings. Fourth, minimal runtime changes slightly in that ATINIT runs before ctors. This makes it consistent with the normal mode, but there is now POSTINIT if anyone needs the old behavior.
8bd6c29
to
80e5cbf
Compare
This is a combination of my previous PR and Sam's PR to remove ATMAIN (#23478). Sam and I discussed that it is probably better to re-split the FS initialization that was done in a previous refactor (see comment above on this). |
src/lib/libfs.js
Outdated
@@ -36,8 +36,8 @@ addToLibrary({ | |||
addAtInit(` | |||
if (!Module['noFSInit'] && !FS.initialized) | |||
FS.init(); | |||
FS.ignorePermissions = false; | |||
`) | |||
`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just make this into a single line for consistency?
function addAtInit(code) { | ||
ATINITS.push(code); | ||
} | ||
|
||
export const ATPOSTINITS = []; | ||
|
||
// Add code to run after static constructors, but before main (if applicable). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks great! The only think i'm not sure about is the naming of this new list of actions.
Reading this comment made me think that since the primary defining factor seems to be related to static ctors, perhaps we should call it something with that in the name? ATCTORS
? ATPOSTCTORS
? I'm not sure I love either of those though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not crazy about the name. I think I had ATPOSTCTORS
in another version. That's probably the most clear.
src/parseTools_legacy.mjs
Outdated
@@ -42,7 +42,7 @@ const Runtime = { | |||
const runOnMainThread = runIfMainThread; | |||
|
|||
addToCompileTimeContext({ | |||
ATMAINS, | |||
ATPOSTINITS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can simply be removed since its already exported in parseTools.js
src/parseTools.mjs
Outdated
@@ -1086,6 +1094,7 @@ function ENVIRONMENT_IS_WORKER_THREAD() { | |||
addToCompileTimeContext({ | |||
ATEXITS, | |||
ATINITS, | |||
ATPOSTINITS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just not export the array itself?
I'm not sure library code needs to access the ATPOSTINITS does it?
This is a first step in trying to unify more of the code used by minimal runtime and the normal runtime and also move all addOn* functions into a library.
First, make the call to the static constructors use the same code for both modes. With this change, it becomes necessary to have an injection point that runs before ctors and after ctors. To achieve that, I've replaced the unused ATMAIN with new ATPOSTINIT code injection point. The new ATPOSTINIT is similar to the old ATMAIN in that it runs after the static constructors, but it now runs even if there is no main function.
Second, the FS initialization is now split into two parts (similar to how it used to be before a refactoring).
Third, switch embind code generation to use the POSTINIT so it can run after the ctors have setup the bindings.
Fourth, minimal runtime changes slightly in that ATINIT runs before ctors. This makes it consistent with the normal mode, but there is now POSTINIT if anyone needs the old behavior.