-
Notifications
You must be signed in to change notification settings - Fork 32
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
runtime: don't exit the main thread if a coexp attempted to gc it #362
Conversation
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 think the headline comment is misleading. It says we should never GC the main thread, which is obviously wrong. I think the mod is attempting to stop the recovery of the activation block for the main thread (which makes sense, because nobody activated it).
However, the big problem is that the ci fails. I put the patch into my (up to date) master branch on my local machine and it failed there too.
src/runtime/rmemmgt.r
Outdated
@@ -1535,6 +1535,8 @@ static void cofree() | |||
|
|||
xep = *ep; | |||
*ep = (*ep)->nextstk; | |||
if (xep->program == &rootpstate || xep->id == 1) |
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 causes build errors for some configurations.
…to gc it. Signed-off-by: Jafar Al-Gharaibeh <[email protected]>
f87e9ee
to
30ebc09
Compare
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.
Looks OK. I'm not sure if the FIXME describes what has been done (and should be removed) or if it is questioning the new code .
#endif /* NO_COEXPR_SEMAPHORE_FIX */ | ||
pthread_exit(0); | ||
} else if (cp->id == 1 && cp->id == curtstate->c->id) { | ||
/* FIXME: Main thread, should just return? */ |
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.
Should this FIXME be here? If yes, it's new code; why aren't we fixing it now?
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.
Left it there intentionally because I wasn't there if this has any side effects.
runtime: don't exit the main thread if a coexp attempted to gc it
No description provided.