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.

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

crt1-command.c: always call wasi_proc_exit for _REENTRANT #367

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

crt1-command.c: always call wasi_proc_exit for _REENTRANT #367

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5b0a926
crt1-command.c: always call wasi_proc_exit for _REENTRANT
yamt Dec 20, 2022
43cc8ff
crt1-commant.c: add a comment
yamt Dec 20, 2022
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
13 changes: 13 additions & 0 deletions libc-bottom-half/crt/crt1-command.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,25 @@ void _start(void) {
// arguments and calls `__main_argv_argc`.
int r = __main_void();

// The following code is basically an inlined copy of exit().
// It's mandated by the C standard:
//
// > If the return type of the main function is a type compatible
// > with int, a return from the initial call to the main function
// > is equivalent to calling the exit function with the value
// > returned by the main function as its argument,

// Call atexit functions, destructors, stdio cleanup, etc.
__wasm_call_dtors();

#ifdef _REENTRANT
// `__wasi_proc_exit` terminates sibling threads.
__wasi_proc_exit(r);
Copy link
Member

Choose a reason for hiding this comment

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

I would expect the host runtime to take care of this, even we just return.. the host should terminate all threads from the outside, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i don't expect so because:

  • at wasi level, nothing wrong with making the main thread returns while keeping other threads running.
  • it's also consistent with what happens when other thread (wasi_thread_start) returns.
  • it's simpler to have less special-casing for main thread.

Copy link
Member

Choose a reason for hiding this comment

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

If "exit and keep threads running" is something that we would want at the wasi level why wouldn't we also want it at the libc level? Can we agree on what they both want and just return from main here?

Another way of putting it, assuming returning from main without __wasi_proc_exit is a useful thing to do, do we not want to allow wasi-libc users to do it?

Copy link
Member

Choose a reason for hiding this comment

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

I agree. Also, in the future when we migrate off of instance-per-thread, threads running after main returns is potentially awkward, because a main function could be called by a caller that isn't expecting the instance to keep running after main returns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, threads running after main thread exits is actually quite normal as far as i know.
such applications achieve that by calling pthread_exit, though.

as far as C is our concern, maybe there is not much differences between either approaches.
however, IMO, there is little point to make the main thread special in this regard.

I agree. Also, in the future when we migrate off of instance-per-thread, threads running after main returns is potentially awkward, because a main function could be called by a caller that isn't expecting the instance to keep running after main returns.

it's normal for unix waitpid etc to wait for all threads exit, not only the main thread.
i guess embedder apis should follow the semantics.

Copy link
Member

@sbc100 sbc100 Dec 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

__wasi_proc_exit is implemented like a trap, with a stack unwind in some runtimes, and for small short-lived instances this is sometimes noticeable in performance. Letting _start return to its outside-world caller is faster.

That seems like an implementation choice isn't it? There isn't anything about proc_exit that requires a trace is there? Also this is something that happens exactly once per instance.. are you sure it makes a measurable difference to real programs? ...also if there is a cost is seems odd to make only non-zero returning programs pay that cost. If true, that kind of sounds like an argument for a __wasi_set_exit_code API ..

On Linux, there is no caller for _start to return to, so calling the _exit function is required for exiting the process.

Copy link
Member

Choose a reason for hiding this comment

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

__wasi_proc_exit is implemented like a trap, with a stack unwind in some runtimes, and for small short-lived instances this is sometimes noticeable in performance. Letting _start return to its outside-world caller is faster.

That seems like an implementation choice isn't it? There isn't anything about proc_exit that requires a trace is there? Also this is something that happens exactly once per instance.. are you sure it makes a measurable difference to real programs? ...also if there is a cost is seems odd to make only non-zero returning programs pay that cost. If true, that kind of sounds like an argument for a __wasi_set_exit_code API ..

Actually a better solution might be to have _start return the exit code .. then we could simply delete this call.

On Linux, there is no caller for _start to return to, so calling the _exit function is required for exiting the process.

Copy link
Member

Choose a reason for hiding this comment

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

__wasi_proc_exit is implemented like a trap, with a stack unwind in some runtimes, and for small short-lived instances this is sometimes noticeable in performance. Letting _start return to its outside-world caller is faster.

That seems like an implementation choice isn't it? There isn't anything about proc_exit that requires a trace is there? Also this is something that happens exactly once per instance.. are you sure it makes a measurable difference to real programs? ...also if there is a cost is seems odd to make only non-zero returning programs pay that cost. If true, that kind of sounds like an argument for a __wasi_set_exit_code API ..

_start returning is just a normal function return operation. But proc_exit is an unwind stack frames operation, which in some settings involves a fair amount of work. Wasm instances don't require starting up new OS processes; they can be very cheap.

Actually a better solution might be to have _start return the exit code .. then we could simply delete this call.

Yes, returning a value would have been a better way to do it. In the preview2 work we've already moved to having main have a return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

__wasi_proc_exit is implemented like a trap, with a stack unwind in some runtimes, and for small short-lived instances this is sometimes noticeable in performance. Letting _start return to its outside-world caller is faster.

That seems like an implementation choice isn't it? There isn't anything about proc_exit that requires a trace is there? Also this is something that happens exactly once per instance.. are you sure it makes a measurable difference to real programs? ...also if there is a cost is seems odd to make only non-zero returning programs pay that cost. If true, that kind of sounds like an argument for a __wasi_set_exit_code API ..

_start returning is just a normal function return operation. But proc_exit is an unwind stack frames operation, which in some settings involves a fair amount of work. Wasm instances don't require starting up new OS processes; they can be very cheap.

do you have a specific implementation in your mind?
for that implementation, rewinding a single small function (_start) is so heavy?
if so, i suppose it's a problem of that specific implementation.

Copy link
Member

Choose a reason for hiding this comment

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

The size of the function doesn't change anything. In implementations where proc_exit calls _Unwind_Resume it sometimes has to do a fair amount of work. You're right that that's a cost of such implementations. And we are working on moving to return values which will obviate this whole situation. So the question here is what we want the behavior of proc_exit and wasi_thread_exit to be; see my comment below.

#else
// If main exited successfully, just return, otherwise call
// `__wasi_proc_exit`.
if (r != 0) {
__wasi_proc_exit(r);
}
#endif
}