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.

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

Add at_quick_exit() / quick_exit() #10

Closed
wants to merge 1 commit into from
Closed

Add at_quick_exit() / quick_exit() #10

wants to merge 1 commit into from

Conversation

thrimbor
Copy link
Member

This implements at_quick_exit() and quick_exit(). Both are C11 functions that are required by libc++.

No "xbox"-prefix for this commit since it's a generic implementation.

include/stdlib.h Outdated Show resolved Hide resolved
@JayFoxRox
Copy link
Member

What are the plans for C11 adoption upstream? Would it make sense to send our code there (and then move it downstream)?

@thrimbor
Copy link
Member Author

What are the plans for C11 adoption upstream? Would it make sense to send our code there (and then move it downstream)?

To my understanding upstream doesn't actively pursue C11 support atm, but is open to adding parts of it when requested.

I do actually plan to submit this upstream, too, but figured that merging it here would be the more quick way on the way to libc++ - rebasing to upstream if/when merged there could then happen together with a clean-up of the dlmalloc-changes.

static void checkhandler( void )
{
int i;
for ( i = 0; i < 31; ++i )
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason why there are flags[0 to 31], but only flags[0 to 30] are being tested?

Copy link
Member Author

Choose a reason for hiding this comment

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

The testcase is almost an exact copy of the one from at_exit.c, and I assume it was done this way by mistake. I changed the loop here and it seems to still work fine.

Copy link
Member

@JayFoxRox JayFoxRox Sep 5, 2019

Choose a reason for hiding this comment

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

I had thought so; please also fix / report issue upstream (before resolving conversation / merging PR - so we don't forget about it)

#ifndef REGTEST

/* TODO - "...except that a function is called after any previously registered
functions that had already been called at the time it was registered."
Copy link
Member

Choose a reason for hiding this comment

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

Context for this quote would help

Copy link
Member Author

Choose a reason for hiding this comment

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

It's from the C standard (copied from exit.c), from the section where quick_exit and exit are explained. I added a small note.

Copy link
Member

Choose a reason for hiding this comment

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

What is there left TODO then? Is this about the case where functions are registered from within the callbacks? I'm still not sure what is meant by that comment.

Copy link

Choose a reason for hiding this comment

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

If I may? I added that comment to my implementation of exit.c to remind myself that I am not 100% sure I understood the meaning of the standard here, and that I want (TODO) to go back and double-check I did not miss something subtle.

The comment just got copied to quick_exit.c, as the implementations are virtually identical.

EXIT_SUCCESS and EXIT_FAILURE above.)
quick_exit() does not return.
*/
_PDCLIB_PUBLIC void quick_exit( int status );
Copy link
Member

Choose a reason for hiding this comment

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

nit: It's weird how none of the functions listed here seem to use _PDCLIB_NORETURN; we can probably change this in a follow-up across the codebase / upstream.

Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed as well and plan to mention this upstream.

Copy link
Member

Choose a reason for hiding this comment

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

Would be good to have this reported before resolving this conversation / finishing this PR, because we'll probably just forget about it otherwise.

@thrimbor
Copy link
Member Author

The submitted functionality has been implemented upstream. I'm closing this in favor of an update-PR I plan to submit soon, so we can benefit from the additional improvements made upstream.

@thrimbor thrimbor closed this Sep 30, 2019
thrimbor pushed a commit that referenced this pull request Mar 6, 2020
…anks to sam-itt for pointing it out (GitHub issue #10).

git-svn-id: svn://rootdirectory.ddns.net/pdclib/trunk@863 bcf39385-58cc-4174-9fcf-14f50f90dd47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants