-
Notifications
You must be signed in to change notification settings - Fork 3.4k
NO_EXIT_RUNTIME by default #5878
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
Conversation
…ad in the general case. show warnings in ASSERTIONS mode when exiting the runtime was necessary
…directly, it just happened to work because other things included it
…ful for configure/cmake, and easy to do with no overhead).
… to get the exit code from the running program, as if it were a shell command, not a browser app
src/library.js
Outdated
@@ -596,6 +596,11 @@ LibraryManager.library = { | |||
atexit__proxy: 'sync', | |||
atexit__sig: 'ii', | |||
atexit: function(func, arg) { | |||
#if ASSERTIONS | |||
#if NO_EXIT_RUNTIME == 1 | |||
Runtime.warnOnce('atexit() called, but NO_EXIT_RUNTIME, so atexits() will not be called. set NO_EXIT_RUNTIME to 0'); |
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 could probably use some more detail or a link to a doc. Also in C++, if the user has any static objects with destructors, then those destructors get registered with atexit under the hood. so the doc or message should probably mention that case explicitly.
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.
Good point, added a FAQ entry about that, and mentioned the FAQ in those messages.
#endif | ||
if (flush) { |
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 don't quite understand why we are doing this dance with module.print. If it's ok to just call the users's fflush
, why can't we just call it and let it flush for real?
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.
We don't normally even include fflush
anymore, we only include it in ASSERTIONS
builds. So we don't want to show the proper output here, we want to see if there would be a problem, had we not been in an ASSERTIONS
build. So we just check if flushing would have emitting something (and we do so very carefully, as shutting down the FS may throw an error if it hit an error previously or something like that).
I can add more of an explanation to the code?
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.
Reading that again it does seem less than obvious, good point. Added a comment.
…as we leave main(), then do not call exit, we are not exiting yet - code is yet to run later
This PR uncovered another existing bug, we used to call |
src/library.js
Outdated
@@ -596,6 +596,11 @@ LibraryManager.library = { | |||
atexit__proxy: 'sync', | |||
atexit__sig: 'ii', | |||
atexit: function(func, arg) { | |||
#if ASSERTIONS | |||
#if NO_EXIT_RUNTIME == 1 | |||
Runtime.warnOnce('atexit() called, but NO_EXIT_RUNTIME, so atexits() will not be called. set NO_EXIT_RUNTIME to 0 (see the FAQ)'); |
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 should probably say 'but NO_EXIT_RUNTIME is 1' (or 'is set') instead of just 'but NO_EXIT_RUNTIME'
src/postamble.js
Outdated
#if NO_EXIT_RUNTIME | ||
Module.printErr('exit(' + status + ') called, but compiled with NO_EXIT_RUNTIME, so halting execution but not exiting the runtime or preventing further async execution (build with NO_EXIT_RUNTIME=0, if you want a true shutdown)'); | ||
#else | ||
Module.printErr('exit(' + status + ') called, but noExitRuntime, so halting execution but not exiting the runtime or preventing further async execution (you can use emscripten_force_exit, if you want to force a true shutdown)'); |
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.
same here
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.
Thanks, done.
So, I am mostly convinced this is a good idea. I agree that most users would want this behavior and benefit from the size reduction. We currently (with or without this PR) have the situation that if you have NO_EXIT_RUNTIME there's no way to manually cause your destructors to run, right? (i.e. you want to shut down your module and flush, etc)? Has anyone complained about that? |
Yes, the mode does not support global destructor calling at all.
I can't remember any complaints. But to avoid confusion, we should warn about this in |
Added that. |
All feedback above should be addressed, and I have an offline lgtm from @juj, so I'll merge this as soon as tests pass. The last patch updates the metadce test, as after this change is is more effective, and the test expectations need to be updated. |
Oddly the pthreads part of I made it exit the runtime there, and now it passes. Is that ok? I also added a commit with a faq entry on this stuff, and a better warning message (that also mentions that emitting a newline is enough to flush std streams). |
Got on offline ok from @juj, and tests look good. |
This makes us not exit the runtime by default. That means we don't emit code for atexits and other things that happen when the runtime shuts down, like flushing the stdio streams. This is beneficial for 2 reasons:
main()
exits, so we set this flag to 1 for many tests, which this PR lets us remove.However, this is a breaking change. As already mentioned, the possible breakages are
printf("hello")
will notconsole.log
since there is no newline. Only when the streams are flushed would that be printed out. So this change would make us not emit that.atexit
s do not run.Both of those risks are mitigated in this PR:
ASSERTIONS
mode, check if there is unflushed stream output, and explain what to do if so.ASSERTIONS
mode, warn ifatexit
is called.This PR has a lot of test changes, some that simplify web tests - because the new default is better for the web - but others that add a param to a shell test - because the new default is less optimal in a shell environment. I think the risk here is lower than those shell tests indicate: we do test quite a lot of things in the shell, but just because it's convenient, not because that's what most users care about.
This PR uncovered 2 existing minor bugs:
NO_EXIT_RUNTIME
. Otherwise the main thread hangs waiting for them to complete. I think we never noticed this before since we never tested them withNO_EXIT_RUNTIME
.FORCE_FILESYSTEM
didn't actually do what the name suggests. Again, I think we just never tested it properly withNO_EXIT_RUNTIME
. Fixed in this PR.