[GLUTEN-8476][VL] Fix allocate and free memory#8477
Conversation
jkhaliqi
force-pushed
the
jk_cve_allocate
branch
from
2b5814b to
8ba547b
Compare
| } | ||
| memcpy(reallocatedP, p, std::min(size, newSize)); | ||
| std::free(p); | ||
| if (p != nullptr) { |
There was a problem hiding this comment.
Let's add the check of p at beginning
if (newSize <= 0 && p==nullptr) {
There was a problem hiding this comment.
Thank you! Will switch the && to ||
|
|
||
| bool StdMemoryAllocator::free(void* p, int64_t size) { | ||
| std::free(p); | ||
| if (p != nullptr) { |
There was a problem hiding this comment.
change to
if (p==nullptr) return false;
jkhaliqi
force-pushed
the
jk_cve_allocate
branch
2 times, most recently
from
6a19100 to
d3c9231
Compare
|
|
||
| bool StdMemoryAllocator::reallocateAligned(void* p, uint64_t alignment, int64_t size, int64_t newSize, void** out) { | ||
| if (newSize <= 0) { | ||
| if (newSize <= 0 || p == nullptr) { |
There was a problem hiding this comment.
Can we throw exceptions (and other changes as well)? Since it's not usual to have a nullptr passed in for such functions.
There was a problem hiding this comment.
Could refer to the macros in https://github.com/apache/incubator-gluten/blob/6d71d27884dd6bda38848cb012a63fb37ee2d298/cpp/core/utils/Exception.h#L42 if needed
jkhaliqi
force-pushed
the
jk_cve_allocate
branch
from
d3c9231 to
a11df59
Compare
| bool StdMemoryAllocator::reallocateAligned(void* p, uint64_t alignment, int64_t size, int64_t newSize, void** out) { | ||
| if (p == nullptr) { | ||
| throw gluten::GlutenException("p is nullptr"); | ||
| } |
There was a problem hiding this comment.
Hongze means to use the Macro:
GLUTEN_CHECK(p!=nullptr,"reallocate with nullptr");
There was a problem hiding this comment.
updated, thank you!
| bool StdMemoryAllocator::free(void* p, int64_t size) { | ||
| if (p == nullptr) { | ||
| throw gluten::GlutenException("p is nullptr"); | ||
| } |
There was a problem hiding this comment.
GLUTEN_CHECK(p!=nullptr, "free with nullptr");
There was a problem hiding this comment.
updated, thank you!
jkhaliqi
force-pushed
the
jk_cve_allocate
branch
from
a11df59 to
ef83329
Compare
3 participants
What changes were proposed in this pull request?
Allocate and free memory in the same module
(Please fill in changes proposed in this fix)
(Fixes: #8476)