You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@arrow.apache.org by we...@apache.org on 2018/12/19 15:10:25 UTC
[arrow] branch master updated: ARROW-3979 : [Gandiva] fix all
valgrind reported errors
This is an automated email from the ASF dual-hosted git repository.
wesm pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/master by this push:
new 6bfac93 ARROW-3979 : [Gandiva] fix all valgrind reported errors
6bfac93 is described below
commit 6bfac93ab1c133190d782683df4054f98b2007e5
Author: shyam <sh...@dremio.com>
AuthorDate: Wed Dec 19 09:10:16 2018 -0600
ARROW-3979 : [Gandiva] fix all valgrind reported errors
Fix all the issues reported by valgrind and also enable option ARROW_TRAVIS_VALGRIND.
Author: shyam <sh...@dremio.com>
Closes #3201 from shyambits2004/master and squashes the following commits:
81d5b7669 <shyam> ARROW-3979 : fix all valgrind reported errors
---
.travis.yml | 5 ++-
cpp/src/gandiva/bitmap_accumulator_test.cc | 7 ++--
cpp/src/gandiva/eval_batch.h | 2 +-
cpp/src/gandiva/exported_funcs_registry.h | 8 +++--
cpp/src/gandiva/local_bitmaps_holder.h | 6 ++--
cpp/src/gandiva/precompiled/CMakeLists.txt | 2 +-
cpp/src/gandiva/projector.cc | 6 ++++
cpp/src/gandiva/selection_vector_test.cc | 51 ++++++++++++++----------------
cpp/src/gandiva/tests/projector_test.cc | 9 +++---
cpp/valgrind.supp | 13 +++++++-
10 files changed, 62 insertions(+), 47 deletions(-)
diff --git a/.travis.yml b/.travis.yml
index bf0261b..6440812 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -114,8 +114,7 @@ matrix:
- ARROW_TRAVIS_OPTIONAL_INSTALL=1
- ARROW_CPP_BUILD_TARGETS="gandiva-all"
- ARROW_TRAVIS_USE_TOOLCHAIN=1
- # ARROW-3979 temporarily disabled.
- - ARROW_TRAVIS_VALGRIND=0
+ - ARROW_TRAVIS_VALGRIND=1
- ARROW_BUILD_WARNING_LEVEL=CHECKIN
- MATRIX_EVAL="CC=gcc-4.9 && CXX=g++-4.9"
before_script:
@@ -123,7 +122,7 @@ matrix:
- if [ $ARROW_CI_CPP_AFFECTED != "1" ] && [ $ARROW_CI_JAVA_AFFECTED != "1" ]; then exit; fi
- $TRAVIS_BUILD_DIR/ci/travis_install_linux.sh
- $TRAVIS_BUILD_DIR/ci/travis_install_clang_tools.sh
- - $TRAVIS_BUILD_DIR/ci/travis_before_script_cpp.sh --only-library
+ - $TRAVIS_BUILD_DIR/ci/travis_before_script_cpp.sh
script:
- $TRAVIS_BUILD_DIR/ci/travis_script_gandiva_cpp.sh
- $TRAVIS_BUILD_DIR/ci/travis_script_gandiva_java.sh
diff --git a/cpp/src/gandiva/bitmap_accumulator_test.cc b/cpp/src/gandiva/bitmap_accumulator_test.cc
index fc89421..53e8aac 100644
--- a/cpp/src/gandiva/bitmap_accumulator_test.cc
+++ b/cpp/src/gandiva/bitmap_accumulator_test.cc
@@ -32,9 +32,8 @@ class TestBitMapAccumulator : public ::testing::Test {
int nrecords);
};
-void TestBitMapAccumulator::FillBitMap(uint8_t* bmap, int nrecords) {
- int nbytes = nrecords / 8;
- unsigned int cur;
+void TestBitMapAccumulator::FillBitMap(uint8_t* bmap, int nbytes) {
+ unsigned int cur = 0;
for (int i = 0; i < nbytes; ++i) {
rand_r(&cur);
@@ -62,7 +61,7 @@ TEST_F(TestBitMapAccumulator, TestIntersectBitMaps) {
uint8_t expected_bitmap[length];
for (int i = 0; i < 4; i++) {
- FillBitMap(src_bitmaps[i], nrecords);
+ FillBitMap(src_bitmaps[i], length);
}
for (int i = 0; i < 4; i++) {
diff --git a/cpp/src/gandiva/eval_batch.h b/cpp/src/gandiva/eval_batch.h
index 608f420..093968f 100644
--- a/cpp/src/gandiva/eval_batch.h
+++ b/cpp/src/gandiva/eval_batch.h
@@ -85,7 +85,7 @@ class EvalBatch {
/// An array of 'num_buffers_', each containing a buffer. The buffer
/// sizes depends on the data type, but all of them have the same
/// number of slots (equal to num_records_).
- std::unique_ptr<uint8_t*> buffers_array_;
+ std::unique_ptr<uint8_t* []> buffers_array_;
std::unique_ptr<LocalBitMapsHolder> local_bitmaps_holder_;
diff --git a/cpp/src/gandiva/exported_funcs_registry.h b/cpp/src/gandiva/exported_funcs_registry.h
index 511ec9c..35ad5c0 100644
--- a/cpp/src/gandiva/exported_funcs_registry.h
+++ b/cpp/src/gandiva/exported_funcs_registry.h
@@ -18,6 +18,7 @@
#ifndef GANDIVA_EXPORTED_FUNCS_REGISTRY_H
#define GANDIVA_EXPORTED_FUNCS_REGISTRY_H
+#include <memory>
#include <vector>
#include <gandiva/engine.h>
@@ -30,12 +31,12 @@ class ExportedFuncsBase;
/// LLVM/IR code.
class ExportedFuncsRegistry {
public:
- using list_type = std::vector<ExportedFuncsBase*>;
+ using list_type = std::vector<std::shared_ptr<ExportedFuncsBase>>;
// Add functions from all the registered classes to the engine.
static void AddMappings(Engine* engine);
- static bool Register(ExportedFuncsBase* entry) {
+ static bool Register(std::shared_ptr<ExportedFuncsBase> entry) {
registered().push_back(entry);
return true;
}
@@ -48,7 +49,8 @@ class ExportedFuncsRegistry {
};
#define REGISTER_EXPORTED_FUNCS(classname) \
- static bool _registered_##classname = ExportedFuncsRegistry::Register(new classname)
+ static bool _registered_##classname = \
+ ExportedFuncsRegistry::Register(std::make_shared<classname>())
} // namespace gandiva
diff --git a/cpp/src/gandiva/local_bitmaps_holder.h b/cpp/src/gandiva/local_bitmaps_holder.h
index 1dc8256..ae0ba53 100644
--- a/cpp/src/gandiva/local_bitmaps_holder.h
+++ b/cpp/src/gandiva/local_bitmaps_holder.h
@@ -50,10 +50,10 @@ class LocalBitMapsHolder {
int64_t num_records_;
/// A container of 'local_bitmaps_', each sized to accomodate 'num_records'.
- std::vector<std::unique_ptr<uint8_t>> local_bitmaps_vec_;
+ std::vector<std::unique_ptr<uint8_t[]>> local_bitmaps_vec_;
/// An array of the local bitmaps.
- std::unique_ptr<uint8_t*> local_bitmaps_array_;
+ std::unique_ptr<uint8_t* []> local_bitmaps_array_;
int64_t local_bitmap_size_;
};
@@ -72,7 +72,7 @@ inline LocalBitMapsHolder::LocalBitMapsHolder(int64_t num_records, int num_local
// Alloc 'num_local_bitmaps_' number of bitmaps, each of capacity 'num_records_'.
for (int i = 0; i < num_local_bitmaps; ++i) {
// TODO : round-up to a slab friendly multiple.
- std::unique_ptr<uint8_t> bitmap(new uint8_t[local_bitmap_size_]);
+ std::unique_ptr<uint8_t[]> bitmap(new uint8_t[local_bitmap_size_]);
// keep pointer to the bitmap in the array.
(local_bitmaps_array_.get())[i] = bitmap.get();
diff --git a/cpp/src/gandiva/precompiled/CMakeLists.txt b/cpp/src/gandiva/precompiled/CMakeLists.txt
index 2af4908..21a74bd 100644
--- a/cpp/src/gandiva/precompiled/CMakeLists.txt
+++ b/cpp/src/gandiva/precompiled/CMakeLists.txt
@@ -65,7 +65,7 @@ function(add_precompiled_unit_test REL_TEST_NAME)
)
target_compile_definitions(${TEST_NAME} PRIVATE GANDIVA_UNIT_TEST=1)
add_test(NAME ${TEST_NAME} COMMAND ${TEST_NAME})
- set_property(TEST ${TEST_NAME} PROPERTY LABELS gandiva;unittest ${TEST_NAME})
+ set_property(TEST ${TEST_NAME} PROPERTY LABELS gandiva-tests {TEST_NAME})
endfunction(add_precompiled_unit_test REL_TEST_NAME)
# testing
diff --git a/cpp/src/gandiva/projector.cc b/cpp/src/gandiva/projector.cc
index 8020a45..40fdc20 100644
--- a/cpp/src/gandiva/projector.cc
+++ b/cpp/src/gandiva/projector.cc
@@ -175,6 +175,12 @@ Status Projector::AllocArrayData(const DataTypePtr& type, int64_t num_records,
astatus = arrow::AllocateBuffer(pool, data_len, &data);
ARROW_RETURN_NOT_OK(astatus);
+ // Valgrind detects unitialized memory at byte level. Boolean types use bits
+ // and can leave buffer memory uninitialized in the last byte.
+ if (type->id() == arrow::Type::BOOL) {
+ data->mutable_data()[data_len - 1] = 0;
+ }
+
*array_data = arrow::ArrayData::Make(type, num_records, {null_bitmap, data});
return Status::OK();
}
diff --git a/cpp/src/gandiva/selection_vector_test.cc b/cpp/src/gandiva/selection_vector_test.cc
index acb0f33..6738927 100644
--- a/cpp/src/gandiva/selection_vector_test.cc
+++ b/cpp/src/gandiva/selection_vector_test.cc
@@ -18,6 +18,7 @@
#include "gandiva/selection_vector.h"
#include <memory>
+#include <vector>
#include <gtest/gtest.h>
@@ -102,15 +103,14 @@ TEST_F(TestSelectionVector, TestInt16PopulateFromBitMap) {
EXPECT_EQ(status.ok(), true) << status.message();
int bitmap_size = RoundUpNumi64(max_slots) * 8;
- std::unique_ptr<uint8_t> bitmap(new uint8_t[bitmap_size]);
- memset(bitmap.get(), 0, bitmap_size);
+ std::vector<uint8_t> bitmap(bitmap_size);
- arrow::BitUtil::SetBit(bitmap.get(), 0);
- arrow::BitUtil::SetBit(bitmap.get(), 5);
- arrow::BitUtil::SetBit(bitmap.get(), 121);
- arrow::BitUtil::SetBit(bitmap.get(), 220);
+ arrow::BitUtil::SetBit(&bitmap[0], 0);
+ arrow::BitUtil::SetBit(&bitmap[0], 5);
+ arrow::BitUtil::SetBit(&bitmap[0], 121);
+ arrow::BitUtil::SetBit(&bitmap[0], 220);
- status = selection->PopulateFromBitMap(bitmap.get(), bitmap_size, max_slots - 1);
+ status = selection->PopulateFromBitMap(&bitmap[0], bitmap_size, max_slots - 1);
EXPECT_EQ(status.ok(), true) << status.message();
EXPECT_EQ(selection->GetNumSlots(), 3);
@@ -127,15 +127,14 @@ TEST_F(TestSelectionVector, TestInt16PopulateFromBitMapNegative) {
EXPECT_EQ(status.ok(), true) << status.message();
int bitmap_size = 16;
- std::unique_ptr<uint8_t> bitmap(new uint8_t[bitmap_size]);
- memset(bitmap.get(), 0, bitmap_size);
+ std::vector<uint8_t> bitmap(bitmap_size);
- arrow::BitUtil::SetBit(bitmap.get(), 0);
- arrow::BitUtil::SetBit(bitmap.get(), 1);
- arrow::BitUtil::SetBit(bitmap.get(), 2);
+ arrow::BitUtil::SetBit(&bitmap[0], 0);
+ arrow::BitUtil::SetBit(&bitmap[0], 1);
+ arrow::BitUtil::SetBit(&bitmap[0], 2);
// The bitmap has three set bits, whereas the selection vector has capacity for only 2.
- status = selection->PopulateFromBitMap(bitmap.get(), bitmap_size, 2);
+ status = selection->PopulateFromBitMap(&bitmap[0], bitmap_size, 2);
EXPECT_EQ(status.IsInvalid(), true);
}
@@ -175,15 +174,14 @@ TEST_F(TestSelectionVector, TestInt32PopulateFromBitMap) {
EXPECT_EQ(status.ok(), true) << status.message();
int bitmap_size = RoundUpNumi64(max_slots) * 8;
- std::unique_ptr<uint8_t> bitmap(new uint8_t[bitmap_size]);
- memset(bitmap.get(), 0, bitmap_size);
+ std::vector<uint8_t> bitmap(bitmap_size);
- arrow::BitUtil::SetBit(bitmap.get(), 0);
- arrow::BitUtil::SetBit(bitmap.get(), 5);
- arrow::BitUtil::SetBit(bitmap.get(), 121);
- arrow::BitUtil::SetBit(bitmap.get(), 220);
+ arrow::BitUtil::SetBit(&bitmap[0], 0);
+ arrow::BitUtil::SetBit(&bitmap[0], 5);
+ arrow::BitUtil::SetBit(&bitmap[0], 121);
+ arrow::BitUtil::SetBit(&bitmap[0], 220);
- status = selection->PopulateFromBitMap(bitmap.get(), bitmap_size, max_slots - 1);
+ status = selection->PopulateFromBitMap(&bitmap[0], bitmap_size, max_slots - 1);
EXPECT_EQ(status.ok(), true) << status.message();
EXPECT_EQ(selection->GetNumSlots(), 3);
@@ -243,15 +241,14 @@ TEST_F(TestSelectionVector, TestInt64PopulateFromBitMap) {
EXPECT_EQ(status.ok(), true) << status.message();
int bitmap_size = RoundUpNumi64(max_slots) * 8;
- std::unique_ptr<uint8_t> bitmap(new uint8_t[bitmap_size]);
- memset(bitmap.get(), 0, bitmap_size);
+ std::vector<uint8_t> bitmap(bitmap_size);
- arrow::BitUtil::SetBit(bitmap.get(), 0);
- arrow::BitUtil::SetBit(bitmap.get(), 5);
- arrow::BitUtil::SetBit(bitmap.get(), 121);
- arrow::BitUtil::SetBit(bitmap.get(), 220);
+ arrow::BitUtil::SetBit(&bitmap[0], 0);
+ arrow::BitUtil::SetBit(&bitmap[0], 5);
+ arrow::BitUtil::SetBit(&bitmap[0], 121);
+ arrow::BitUtil::SetBit(&bitmap[0], 220);
- status = selection->PopulateFromBitMap(bitmap.get(), bitmap_size, max_slots - 1);
+ status = selection->PopulateFromBitMap(&bitmap[0], bitmap_size, max_slots - 1);
EXPECT_EQ(status.ok(), true) << status.message();
EXPECT_EQ(selection->GetNumSlots(), 3);
diff --git a/cpp/src/gandiva/tests/projector_test.cc b/cpp/src/gandiva/tests/projector_test.cc
index becaf8f..61d9dc3 100644
--- a/cpp/src/gandiva/tests/projector_test.cc
+++ b/cpp/src/gandiva/tests/projector_test.cc
@@ -493,14 +493,15 @@ TEST_F(TestProjector, TestZeroCopy) {
// allocate output buffers
int64_t bitmap_sz = arrow::BitUtil::BytesForBits(num_records);
- std::unique_ptr<uint8_t[]> bitmap(new uint8_t[bitmap_sz]);
+ int64_t bitmap_capacity = arrow::BitUtil::RoundUpToMultipleOf64(bitmap_sz);
+ std::vector<uint8_t> bitmap(bitmap_capacity);
std::shared_ptr<arrow::MutableBuffer> bitmap_buf =
- std::make_shared<arrow::MutableBuffer>(bitmap.get(), bitmap_sz);
+ std::make_shared<arrow::MutableBuffer>(&bitmap[0], bitmap_capacity);
int64_t data_sz = sizeof(float) * num_records;
- std::unique_ptr<uint8_t[]> data(new uint8_t[data_sz]);
+ std::vector<uint8_t> data(bitmap_capacity);
std::shared_ptr<arrow::MutableBuffer> data_buf =
- std::make_shared<arrow::MutableBuffer>(data.get(), data_sz);
+ std::make_shared<arrow::MutableBuffer>(&data[0], data_sz);
auto array_data =
arrow::ArrayData::Make(float32(), num_records, {bitmap_buf, data_buf});
diff --git a/cpp/valgrind.supp b/cpp/valgrind.supp
index 8e707e3..d8bc8fb 100644
--- a/cpp/valgrind.supp
+++ b/cpp/valgrind.supp
@@ -21,4 +21,15 @@
Memcheck:Cond
fun:*CastFunctor*BooleanType*
}
-
+{
+ <re2>:Conditional jump or move depends on uninitialised value(s)
+ Memcheck:Cond
+ ...
+ fun:_ZN3re23RE2C1E*
+}
+{
+ <re2>:Use of uninitialised value of size 8
+ Memcheck:Value8
+ ...
+ fun:_ZN3re23RE2C1E*
+}