-
Notifications
You must be signed in to change notification settings - Fork 116
Add exception handling for new operator #791
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
|
Addresses #785 |
|
There are both throwing and non-throwing variants of the new operator: https://en.cppreference.com/w/cpp/memory/new/operator_new Is the idea that even for the throwing one, we will also actively avoid adding any additional branch in the fast path? |
Yes, the template parameter: template<bool ShouldThrow = true>Basically creates two versions of the failure continuation, one that throws, and one that doesn't. Then void* operator new(size_t size)
{
return snmalloc::alloc<snmalloc::Throw>(size);
}will throw without a branch, and void* operator new(size_t size, std::nothrow_t&)
{
return snmalloc::alloc<snmalloc::NoThrow>(size);
}will not throw. They will both call the function returned by
That is a good idea. I am not sure quite what the right default is, and what to leave in
What do you think? |
|
I also have no idea how I have broken the Bazel build again. |
|
|
The new operator in the snmalloc did not throw exceptions in the case of allocation failure. Moreover, it was not possible to override the behaviour of the failure of the new operator using the std::set_new_handler function. This commit adds the necessary code to the snmalloc new operator to throw std::bad_alloc when the allocation fails. It also allows the std::set_new_handler function to be used to set a custom handler for allocation failures.
This makes most components compilable without needing exceptions. The tails calls in the main code should mean we land on the exception path without any other snmalloc frames in the way.
|
@SchrodingerZhu, so after some experiments,
Then I believe we get valid exception behaviour, and the memory footprint does not increase. If we dynamically link libstdc++, then the memory footprint increases by 1-2Mb. I am tempted to just make this the default going forward. @davidchisnall pinging you, in case you have opinions on this. |
|
@SchrodingerZhu I think this is stable now. I found a few places where the codegen was not doing tail calls, but now I have it working nicely I think. Discovering the |
SchrodingerZhu
left a comment
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.
LGTM. I like the way how noexcept gets propagated.
The new operator in the snmalloc did not throw exceptions in the case of allocation failure. Moreover, it was not possible to override the behaviour of the failure of the new operator using the std::set_new_handler function.
This commit adds the necessary code to the snmalloc new operator to throw std::bad_alloc when the allocation fails. It also allows the std::set_new_handler function to be used to set a custom handler for allocation failures.
The PR also adds a new build target libsnmalloc-minimal.so that does not override
new. This has smaller dependencies.