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

Use alternative mechanisms to allow us to use the default new/delete implementations #101947

Merged
merged 13 commits into from
May 17, 2024

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented May 6, 2024

  • Set up the StressLog OOM logging in CoreCLR using set_new_handler.
  • Update CoreCLR's exception handling macros to handle std::bad_alloc and translate it to our pre-allocated OutOfMemoryException.

With these changes, we can switch to the global new and delete implementations provided by the C++ Standard Library.

Also remove _BLD_CLR define as we always define it in the product. Adjust the one test native binary that was using the headers without defining it.

@@ -27,9 +27,9 @@
// case we need to wrap memory allocations, in the latter there is no
// infrastructure to support this. Detect which way we're building and provide a
// very simple abstraction layer (handles allocating bytes only).
#ifdef _BLD_CLR
Copy link
Member

Choose a reason for hiding this comment

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

The comment above is stale after removing this ifdef.

}
else
{
throw std::bad_alloc();
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this makes me wonder - couldn't we just make this handler throw our OutOfMemoryException here instead of making all the changes in the exception handling macros?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't as the new handler is technically process-global. So with this change, we would technically report more OOMs (as we'd report an OOM with the stack from ICU for example) but we wouldn't break a C++ library that is expecting to catch std::bad_alloc.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, makes sense, thank you for the explanation.

Copy link
Member

Choose a reason for hiding this comment

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

We can't as the new handler is technically process-global.

Does that mean that some other library loaded in the process can break us, or vice versa?

Copy link
Member

Choose a reason for hiding this comment

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

I am wondering whether it would be better to drop this stresslogging. I do not think it is particularly important.

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 that is it not particularly important. Both from dump and live debugging, it will be clear that there was an OOM, so having it in the stresslog doesn't seem to provide any benefit.

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'll remove it from this path then.

src/coreclr/inc/stresslog.h Outdated Show resolved Hide resolved
src/coreclr/vm/ceemain.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/ceemain.cpp Outdated Show resolved Hide resolved
src/coreclr/inc/stresslog.h Outdated Show resolved Hide resolved
src/coreclr/inc/stresslog.h Outdated Show resolved Hide resolved
@@ -32,9 +32,10 @@ set(SOURCES

include_directories(../../../coreclr/pal/prebuilt/inc)

add_compile_definitions(SOS_INCLUDE)
Copy link
Member

Choose a reason for hiding this comment

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

This define can use a better name.

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'll come back to this and clean this up in a future PR.

@jkoritzinsky jkoritzinsky merged commit 4246ba1 into dotnet:main May 17, 2024
95 checks passed
@jkoritzinsky jkoritzinsky deleted the no-custom-new branch May 17, 2024 04:14
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jun 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants