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

Simplify our global new/delete implementations #101826

Merged
merged 9 commits into from
May 6, 2024

Conversation

jkoritzinsky
Copy link
Member

Simplify our global overrides of new/delete and remove quite a bit of dead code.

  • Stack Hashing to track allocations was a no-op as it depended on a debug-only .NET Framework component, so even when the code was enabled, it did nothing. Remove this code.
  • Our debug tagging doesn't get us much now that we have AddressSanitizer runs that can accurately detect exactly when heap corruption happens. Remove it. Fixes the symptom of superpmi tool crashing with "Heap contamination detected!" #101708.
  • The new(executable) variants of new are not used anywhere in the code-base and they won't be now that we support W/X.
  • Since VS2012, the Windows CRT has used the exact same allocation implementation that we were using (the process heap). Just use that and don't maintain the code ourselves.

I'm confident that this fixes #101708 completely as all of the C++ standard libraries we use call malloc/free under the hood the same way we do (and macOS's libc++ in particular does).

There are two follow-up changes we can make that will allow us to fully remove our global new/delete overrides:

  • 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 object.

I plan to explore these in future PRs.

….NET Framework component that doesn't ship.)
…le per platform (on Windows still use the process heap, when hosted notify StressLog about OOMs)
…lete/malloc/free. Use the CRT support instead of doing it manually.
Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

A few questions, LGTM otherwise. Thanks!

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks

@jkoritzinsky jkoritzinsky merged commit 1eb888b into dotnet:main May 6, 2024
86 of 89 checks passed
@jkoritzinsky jkoritzinsky deleted the simplify-new-delete branch May 6, 2024 20:36
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
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 6, 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.

superpmi tool crashing with "Heap contamination detected!"
2 participants