[VL] Fix StdMemoryAllocator::allocateZeroFilled byte accounting#11855
[VL] Fix StdMemoryAllocator::allocateZeroFilled byte accounting#11855LuciferYang wants to merge 3 commits intoapache:mainfrom
Conversation
…led fix Keep only the test that directly validates the nmemb * size accounting bug; remove three unrelated general-purpose allocator tests.
There was a problem hiding this comment.
Pull request overview
Fixes incorrect memory-usage accounting in StdMemoryAllocator::allocateZeroFilled by tracking nmemb * size bytes (matching the actual calloc allocation size), and adds a unit test to prevent regression.
Changes:
- Correct
StdMemoryAllocator::allocateZeroFilledbyte tracking fromsizetonmemb * size. - Add
StdMemoryAllocator.allocateZeroFilledAccountingunit test validating the accounting behavior. - Register the new test in
cpp/core/tests/CMakeLists.txt.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cpp/core/memory/MemoryAllocator.cc | Fixes allocateZeroFilled byte accounting to reflect the true allocation size. |
| cpp/core/tests/MemoryAllocatorTest.cc | Adds a regression test for allocateZeroFilled byte accounting. |
| cpp/core/tests/CMakeLists.txt | Adds the new memory allocator test target to the CMake test list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| *out = std::calloc(nmemb, size); | ||
| if (*out == nullptr) { | ||
| return false; | ||
| } | ||
| bytes_ += size; | ||
| bytes_ += nmemb * size; |
There was a problem hiding this comment.
nmemb * size is computed in int64_t, which can overflow and cause undefined behavior (and incorrect accounting) for large allocations even if calloc succeeds. Consider computing the total bytes using checked multiplication (e.g., via __int128/overflow check) and GLUTEN_CHECK that it fits in int64_t before updating bytes_.
| void* buf = nullptr; | ||
| bool ok = allocator.allocateZeroFilled(10, 64, &buf); | ||
| ASSERT_TRUE(ok); | ||
| ASSERT_NE(buf, nullptr); | ||
| ASSERT_EQ(allocator.getBytes(), 640); | ||
|
|
||
| allocator.free(buf, 640); |
There was a problem hiding this comment.
The test hard-codes 640 in multiple places. To avoid drift if the inputs change, compute an expectedBytes = 10 * 64 once and use it for the accounting assertion and for free(...) (and assert that free returns true to validate the API contract).
| void* buf = nullptr; | |
| bool ok = allocator.allocateZeroFilled(10, 64, &buf); | |
| ASSERT_TRUE(ok); | |
| ASSERT_NE(buf, nullptr); | |
| ASSERT_EQ(allocator.getBytes(), 640); | |
| allocator.free(buf, 640); | |
| const size_t expectedBytes = 10 * 64; | |
| void* buf = nullptr; | |
| bool ok = allocator.allocateZeroFilled(10, 64, &buf); | |
| ASSERT_TRUE(ok); | |
| ASSERT_NE(buf, nullptr); | |
| ASSERT_EQ(allocator.getBytes(), expectedBytes); | |
| ASSERT_TRUE(allocator.free(buf, expectedBytes)); |
What changes are proposed in this pull request?
Fix incorrect byte accounting in
StdMemoryAllocator::allocateZeroFilled. The method callsstd::calloc(nmemb, size)which allocatesnmemb * sizebytes, butbytes_was only incremented bysizeinstead ofnmemb * size.This is consistent with
ListenableMemoryAllocator::allocateZeroFilled, which already correctly trackssize * nmemb.How was this patch tested?
Added a unit test
StdMemoryAllocator.allocateZeroFilledAccountingthat verifiesallocateZeroFilled(10, 64, &buf)correctly tracks 640 bytes.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code