[Support] Use malloc instead of non-throwing new #92157
Conversation
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.
|
@llvm/pr-subscribers-llvm-support Author: Aaron Ballman (AaronBallman) ChangesWhen 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 ( ) and that means this particular out-of-memory situation cannot be gracefully handled. This was discovered while working on #embed 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:
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;
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
I think we want a comment in the code, it's pretty subtle. otherwise LGTM |
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
(
llvm-project/llvm/lib/Support/InitLLVM.cpp
Line 61 in 90109d4
(
llvm-project/llvm/lib/Support/ErrorHandling.cpp
Line 188 in de14b74
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.