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

[Support] Use malloc instead of non-throwing new #92157

Merged
merged 2 commits into from
May 15, 2024

Conversation

AaronBallman
Copy link
Collaborator

When allocating a memory buffer, we use a non-throwing new so that we can explicitly handle memory buffers that are too large to fit into memory. However, when exceptions are disabled, LLVM installs a custom new handler
(

install_out_of_memory_new_handler();
) that explicitly crashes when we run out of memory
(
static void out_of_memory_new_handler() {
) and that means this particular out-of-memory situation cannot be gracefully handled.

This was discovered while working on #embed
(#68620) on Windows and resulted in a crash rather than the preprocessor issuing a diagnostic as expected.

This patch switches away from the non-throwing new to a call to malloc (and free), which will return a null pointer without calling a custom new handler. It is the only instance in Clang or LLVM that I could find which used a non-throwing new, so I did not think we would need anything more involved than this change.

Testing this would be highly platform dependent and so it does not come with test coverage. And because it doesn't change behavior that users are likely to be able to observe, it does not come with a release note.

When allocating a memory buffer, we use a non-throwing new so that we
can explicitly handle memory buffers that are too large to fit into
memory. However, when exceptions are disabled, LLVM installs a custom
new handler
(https://github.com/llvm/llvm-project/blob/90109d444839683b09f0aafdc50b749cb4b3203b/llvm/lib/Support/InitLLVM.cpp#L61)
that explicitly crashes when we run out of memory
(https://github.com/llvm/llvm-project/blob/de14b749fee41d4ded711e771e43043ae3100cb3/llvm/lib/Support/ErrorHandling.cpp#L188)
and that means this particular out-of-memory situation cannot be
gracefully handled.

This was discovered while working on #embed
(llvm#68620) on Windows and
resulted in a crash rather than the preprocessor issuing a diagnostic
as expected.

This patch switches away from the non-throwing new to a call to malloc
(and free), which will return a null pointer without calling a custom
new handler. It is the only instance in Clang or LLVM that I could find
which used a non-throwing new, so I did not think we would need
anything more involved than this change.

Testing this would be highly platform dependent and so it does not come
with test coverage. And because it doesn't change behavior that users
are likely to be able to observe, it does not come with a release note.
@llvmbot
Copy link
Collaborator

llvmbot commented May 14, 2024

@llvm/pr-subscribers-llvm-support

Author: Aaron Ballman (AaronBallman)

Changes

When allocating a memory buffer, we use a non-throwing new so that we can explicitly handle memory buffers that are too large to fit into memory. However, when exceptions are disabled, LLVM installs a custom new handler
(

install_out_of_memory_new_handler();
) that explicitly crashes when we run out of memory
(
static void out_of_memory_new_handler() {
) and that means this particular out-of-memory situation cannot be gracefully handled.

This was discovered while working on #embed
(#68620) on Windows and resulted in a crash rather than the preprocessor issuing a diagnostic as expected.

This patch switches away from the non-throwing new to a call to malloc (and free), which will return a null pointer without calling a custom new handler. It is the only instance in Clang or LLVM that I could find which used a non-throwing new, so I did not think we would need anything more involved than this change.

Testing this would be highly platform dependent and so it does not come with test coverage. And because it doesn't change behavior that users are likely to be able to observe, it does not come with a release note.


Full diff: https://github.com/llvm/llvm-project/pull/92157.diff

1 Files Affected:

  • (modified) llvm/lib/Support/MemoryBuffer.cpp (+2-2)
diff --git a/llvm/lib/Support/MemoryBuffer.cpp b/llvm/lib/Support/MemoryBuffer.cpp
index 4cc4fe019b75b..e30c3acdb8814 100644
--- a/llvm/lib/Support/MemoryBuffer.cpp
+++ b/llvm/lib/Support/MemoryBuffer.cpp
@@ -98,7 +98,7 @@ class MemoryBufferMem : public MB {
 
   /// Disable sized deallocation for MemoryBufferMem, because it has
   /// tail-allocated data.
-  void operator delete(void *p) { ::operator delete(p); }
+  void operator delete(void *p) { std::free(p); }
 
   StringRef getBufferIdentifier() const override {
     // The name is stored after the class itself.
@@ -315,7 +315,7 @@ WritableMemoryBuffer::getNewUninitMemBuffer(size_t Size,
   size_t RealLen = StringLen + Size + 1 + BufAlign.value();
   if (RealLen <= Size) // Check for rollover.
     return nullptr;
-  char *Mem = static_cast<char*>(operator new(RealLen, std::nothrow));
+  char *Mem = static_cast<char*>(std::malloc(RealLen));
   if (!Mem)
     return nullptr;
 

Copy link

github-actions bot commented May 14, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@cor3ntin
Copy link
Contributor

I think we want a comment in the code, it's pretty subtle. otherwise LGTM

Copy link
Member

@jyknight jyknight left a comment

Choose a reason for hiding this comment

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

Ugh, that's an annoying misuse of the new handler. :(

But if this is the only place it affects, this workaround SGTM (with added comment as mentioned).

@AaronBallman AaronBallman merged commit dceaa0f into llvm:main May 15, 2024
3 of 4 checks passed
@AaronBallman AaronBallman deleted the aballman-memory-buffer-oom branch May 15, 2024 14:55
mub-at-arm pushed a commit to mub-at-arm/llvm-project that referenced this pull request May 16, 2024
When allocating a memory buffer, we use a non-throwing new so that we
can explicitly handle memory buffers that are too large to fit into
memory. However, when exceptions are disabled, LLVM installs a custom
new handler

(https://github.com/llvm/llvm-project/blob/90109d444839683b09f0aafdc50b749cb4b3203b/llvm/lib/Support/InitLLVM.cpp#L61)
that explicitly crashes when we run out of memory

(https://github.com/llvm/llvm-project/blob/de14b749fee41d4ded711e771e43043ae3100cb3/llvm/lib/Support/ErrorHandling.cpp#L188)
and that means this particular out-of-memory situation cannot be
gracefully handled.

This was discovered while working on #embed
(llvm#68620) on Windows and
resulted in a crash rather than the preprocessor issuing a diagnostic as
expected.

This patch switches away from the non-throwing new to a call to malloc
(and free), which will return a null pointer without calling a custom
new handler. It is the only instance in Clang or LLVM that I could find
which used a non-throwing new, so I did not think we would need anything
more involved than this change.

Testing this would be highly platform dependent and so it does not come
with test coverage. And because it doesn't change behavior that users
are likely to be able to observe, it does not come with a release note.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants