[GitHub] [arrow] wesm commented on a change in pull request #7679: ARROW-9350: [C++] Fix Valgrind failures

[GitBox <gi...@apache.org>]

wesm commented on a change in pull request #7679:
URL: https://github.com/apache/arrow/pull/7679#discussion_r451603218



##########
File path: cpp/src/arrow/compute/kernel.cc
##########
@@ -46,23 +46,16 @@ namespace compute {
 // KernelContext
 
 Result<std::shared_ptr<ResizableBuffer>> KernelContext::Allocate(int64_t nbytes) {
-  ARROW_ASSIGN_OR_RAISE(std::shared_ptr<ResizableBuffer> result,
-                        AllocateResizableBuffer(nbytes, exec_ctx_->memory_pool()));
-  result->ZeroPadding();
-  return result;
+  return AllocateResizableBuffer(nbytes, exec_ctx_->memory_pool());
 }
 
 Result<std::shared_ptr<ResizableBuffer>> KernelContext::AllocateBitmap(int64_t num_bits) {
   const int64_t nbytes = BitUtil::BytesForBits(num_bits);
   ARROW_ASSIGN_OR_RAISE(std::shared_ptr<ResizableBuffer> result,
                         AllocateResizableBuffer(nbytes, exec_ctx_->memory_pool()));
-  // Some utility methods access the last byte before it might be
-  // initialized this makes valgrind/asan unhappy, so we proactively
-  // zero it.
-  if (nbytes > 0) {
-    internal::ZeroByte(result.get(), result->size() - 1);
-    result->ZeroPadding();
-  }
+  // Since bitmaps are typically written bit by bit, we could leak uninitialized bits.
+  // Make sure all memory is initialized (this also appeases Valgrind).
+  internal::ZeroMemory(result.get());

Review comment:
       This is definitely a "nuke it from orbit" approach, but I think it's OK for now (until it can be demonstrated that there is some performance benefit to not doing this)




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org