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 __cxa_init_primary_exception #23

Merged
merged 4 commits into from Oct 11, 2023

Conversation

itrofimow
Copy link
Contributor

@itrofimow itrofimow commented Aug 28, 2023

Aims to resolve #22

Somewhat related libc++ patch: https://reviews.llvm.org/D158620

@itrofimow itrofimow marked this pull request as ready for review August 28, 2023 03:47
@davidchisnall
Copy link
Collaborator

That looks plausible. Can you add a few tests? I’d like to see, at least, that this exception can be thrown and caught and that the destructor runs in the right place.

};

template <typename Ex>
ExceptionPtr MakeExceptionPtr(Ex e) {
Copy link
Contributor Author

@itrofimow itrofimow Aug 28, 2023

Choose a reason for hiding this comment

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

@itrofimow
Copy link
Contributor Author

itrofimow commented Aug 28, 2023

@davidchisnall added a std::exception_ptr-alike test.
Could you please have a look?

edit: a guidance on where define LIBCXXRT_HAS_INIT_PRIMARY_EXCEPTION should be put would be appreciated

Copy link
Collaborator

@davidchisnall davidchisnall left a comment

Choose a reason for hiding this comment

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

Looks great, thank you.

@emaste , do you think we could sneak this into FreeBSD 14.0? We can do the libc++ updates a bit later, but it would be nice to have the new APIs in so that external things can depend on them, even if the base system libc++ doesn’t yet.

@davidchisnall
Copy link
Collaborator

I’m not sure of the best place to define LIBCXXRT_HAS_INIT_PRIMARY_EXCEPTION. Unfortunately, because the public API is standardised, a lot of consumers provide their own prototypes and don’t actually include any of our headers. It’s probably better for them to use a does-it-link test than rely on our headers.

@emaste
Copy link
Member

emaste commented Aug 29, 2023

@emaste , do you think we could sneak this into FreeBSD 14.0?

I think there's still time, we can sync this repo into FreeBSD main and MFC from there.

@davidchisnall
Copy link
Collaborator

[ Rebased to pick up the fix to make FreeBSD CI work ]

@itrofimow
Copy link
Contributor Author

Hi! Is there anything holding this PR back from being merged?

I would like to try to make use of these changes in llvm/llvm-project#65534, and it would be a bit more convenient to do so with master branch of libcxxrt rather than with a fork

@emaste
Copy link
Member

emaste commented Oct 11, 2023

@davidchisnall do you want to merge this into this libcxxrt repo? I can do a vendor import into FreeBSD from there

@davidchisnall davidchisnall merged commit 03c83f5 into libcxxrt:master Oct 11, 2023
6 checks passed
@davidchisnall
Copy link
Collaborator

Sorry, I thought I set it to merge when the CI finished.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: __cxa_init_primary_exception
3 participants