KUDU-2378. Fix unaligned loads of int128 from rows Our row format doesn't ensure any alignment of data. This is usually fine on x86 where unaligned loads of ints don't need any special instruction and carry a relatively low performance penalty. However, loads of int128_t may generate a 'movdqa' (aligned load) instruction which crashes if the operand is not 16-byte aligned.
This fixes a bunch of spots where we are dereferencing a reinterpret_casted int and replaces them with new UnalignedLoad<> and UnalignedStore<> template functions. These use simple reinterpret_cast dereferences for <=64bit types. For int128_t we use memcpy. The compiler should already treat memcpy as an intrinsic and this will end up being a single instruction. Change-Id: Ic71149ed5c6881cb16dfe6cf8f7906c044a0b41a Reviewed-on: http://gerrit.cloudera.org:8080/9848 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin <aser...@cloudera.com> Reviewed-by: Grant Henke <granthe...@apache.org> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/35940a12 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/35940a12 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/35940a12 Branch: refs/heads/master Commit: 35940a12aa38dedeff7e932f91448d2562163302 Parents: f391365 Author: Todd Lipcon <t...@apache.org> Authored: Mon Mar 26 16:45:02 2018 -0700 Committer: Todd Lipcon <t...@apache.org> Committed: Tue Apr 17 19:29:10 2018 +0000 ---------------------------------------------------------------------- src/kudu/cfile/bshuf_block.cc | 6 +- src/kudu/cfile/bshuf_block.h | 16 +++--- src/kudu/cfile/plain_block.h | 9 +-- src/kudu/cfile/rle_block.h | 13 +++-- src/kudu/common/partial_row.cc | 6 +- src/kudu/common/types.h | 15 ++--- src/kudu/gutil/port.h | 67 ++++++++++++++++++---- src/kudu/integration-tests/all_types-itest.cc | 4 +- 8 files changed, 92 insertions(+), 44 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/35940a12/src/kudu/cfile/bshuf_block.cc ---------------------------------------------------------------------- diff --git a/src/kudu/cfile/bshuf_block.cc b/src/kudu/cfile/bshuf_block.cc index db68d27..bdc12c1 100644 --- a/src/kudu/cfile/bshuf_block.cc +++ b/src/kudu/cfile/bshuf_block.cc @@ -57,7 +57,7 @@ Slice BShufBlockBuilder<UINT32>::Finish(rowid_t ordinal_pos) { RememberFirstAndLastKey(); uint32_t max_value = 0; for (int i = 0; i < count_; i++) { - max_value = std::max(max_value, *cell_ptr(i)); + max_value = std::max(max_value, cell(i)); } // Shrink the block of UINT32 to block of UINT8 or UINT16 whenever possible and @@ -66,7 +66,7 @@ Slice BShufBlockBuilder<UINT32>::Finish(rowid_t ordinal_pos) { Slice ret; if (max_value <= std::numeric_limits<uint8_t>::max()) { for (int i = 0; i < count_; i++) { - uint32_t value = *cell_ptr(i); + uint32_t value = cell(i); uint8_t converted_value = static_cast<uint8_t>(value); memcpy(&data_[i * sizeof(converted_value)], &converted_value, sizeof(converted_value)); } @@ -74,7 +74,7 @@ Slice BShufBlockBuilder<UINT32>::Finish(rowid_t ordinal_pos) { InlineEncodeFixed32(ret.mutable_data() + 16, sizeof(uint8_t)); } else if (max_value <= std::numeric_limits<uint16_t>::max()) { for (int i = 0; i < count_; i++) { - uint32_t value = *cell_ptr(i); + uint32_t value = cell(i); uint16_t converted_value = static_cast<uint16_t>(value); memcpy(&data_[i * sizeof(converted_value)], &converted_value, sizeof(converted_value)); } http://git-wip-us.apache.org/repos/asf/kudu/blob/35940a12/src/kudu/cfile/bshuf_block.h ---------------------------------------------------------------------- diff --git a/src/kudu/cfile/bshuf_block.h b/src/kudu/cfile/bshuf_block.h index 11442eb..564db76 100644 --- a/src/kudu/cfile/bshuf_block.h +++ b/src/kudu/cfile/bshuf_block.h @@ -108,6 +108,8 @@ class BShufBlockBuilder final : public BlockBuilder { count_ = 0; data_.clear(); data_.reserve(block_size); + DCHECK_EQ(reinterpret_cast<uintptr_t>(data_.data()) & (alignof(CppType) - 1), 0) + << "buffer must be naturally-aligned"; buffer_.clear(); buffer_.resize(kHeaderSize); finished_ = false; @@ -157,13 +159,9 @@ class BShufBlockBuilder final : public BlockBuilder { private: typedef typename TypeTraits<Type>::cpp_type CppType; - const CppType* cell_ptr(int idx) const { + CppType cell(int idx) const { DCHECK_GE(idx, 0); - return reinterpret_cast<const CppType*>(&data_[idx * size_of_type]); - } - CppType* cell_ptr(int idx) { - DCHECK_GE(idx, 0); - return reinterpret_cast<CppType*>(&data_[idx * size_of_type]); + return UnalignedLoad<CppType>(&data_[idx * size_of_type]); } // Remember the last added key in 'last_key_'. This is done during @@ -171,8 +169,8 @@ class BShufBlockBuilder final : public BlockBuilder { // data_ buffer during the encoding process. void RememberFirstAndLastKey() { if (count_ == 0) return; - memcpy(&first_key_, cell_ptr(0), size_of_type); - memcpy(&last_key_, cell_ptr(count_ - 1), size_of_type); + first_key_ = cell(0); + last_key_ = cell(count_ - 1); } Slice Finish(rowid_t ordinal_pos, int final_size_of_type) { @@ -299,7 +297,7 @@ class BShufBlockDecoder final : public BlockDecoder { } Status SeekAtOrAfterValue(const void* value_void, bool* exact) OVERRIDE { - CppType target = *reinterpret_cast<const CppType*>(value_void); + CppType target = UnalignedLoad<CppType>(value_void); int32_t left = 0; int32_t right = num_elems_; while (left != right) { http://git-wip-us.apache.org/repos/asf/kudu/blob/35940a12/src/kudu/cfile/plain_block.h ---------------------------------------------------------------------- diff --git a/src/kudu/cfile/plain_block.h b/src/kudu/cfile/plain_block.h index a878e58..b262478 100644 --- a/src/kudu/cfile/plain_block.h +++ b/src/kudu/cfile/plain_block.h @@ -23,8 +23,9 @@ #include "kudu/cfile/block_encodings.h" #include "kudu/cfile/cfile_util.h" #include "kudu/common/columnblock.h" -#include "kudu/util/coding.h" +#include "kudu/gutil/port.h" #include "kudu/util/coding-inl.h" +#include "kudu/util/coding.h" #include "kudu/util/hexdump.h" namespace kudu { @@ -83,14 +84,14 @@ class PlainBlockBuilder final : public BlockBuilder { virtual Status GetFirstKey(void *key) const OVERRIDE { DCHECK_GT(count_, 0); - *reinterpret_cast<CppType *>(key) = Decode<CppType>(&buffer_[kPlainBlockHeaderSize]); + UnalignedStore(key, Decode<CppType>(&buffer_[kPlainBlockHeaderSize])); return Status::OK(); } virtual Status GetLastKey(void *key) const OVERRIDE { DCHECK_GT(count_, 0); size_t idx = kPlainBlockHeaderSize + (count_ - 1) * kCppTypeSize; - *reinterpret_cast<CppType *>(key) = Decode<CppType>(&buffer_[idx]); + UnalignedStore(key, Decode<CppType>(&buffer_[idx])); return Status::OK(); } @@ -160,7 +161,7 @@ class PlainBlockDecoder final : public BlockDecoder { virtual Status SeekAtOrAfterValue(const void *value, bool *exact_match) OVERRIDE { DCHECK(value != NULL); - const CppType &target = *reinterpret_cast<const CppType *>(value); + CppType target = UnalignedLoad<CppType>(value); uint32_t left = 0; uint32_t right = num_elems_; http://git-wip-us.apache.org/repos/asf/kudu/blob/35940a12/src/kudu/cfile/rle_block.h ---------------------------------------------------------------------- diff --git a/src/kudu/cfile/rle_block.h b/src/kudu/cfile/rle_block.h index 4cc6e02..86f296b 100644 --- a/src/kudu/cfile/rle_block.h +++ b/src/kudu/cfile/rle_block.h @@ -233,10 +233,13 @@ class RleIntBlockBuilder final : public BlockBuilder { } virtual int Add(const uint8_t* vals_void, size_t count) OVERRIDE { + DCHECK_EQ(reinterpret_cast<uintptr_t>(vals_void) & (alignof(CppType) - 1), 0) + << "Pointer passed to Add() must be naturally-aligned"; + + const CppType* vals = reinterpret_cast<const CppType*>(vals_void); if (PREDICT_FALSE(count_ == 0)) { - first_key_ = *reinterpret_cast<const CppType*>(vals_void); + first_key_ = vals[0]; } - const CppType* vals = reinterpret_cast<const CppType*>(vals_void); for (size_t i = 0; i < count; ++i) { rle_encoder_.Put(vals[i], 1); } @@ -268,7 +271,7 @@ class RleIntBlockBuilder final : public BlockBuilder { if (PREDICT_FALSE(count_ == 0)) { return Status::NotFound("No keys in the block"); } - *reinterpret_cast<CppType*>(key) = first_key_; + UnalignedStore<CppType>(key, first_key_); return Status::OK(); } @@ -276,7 +279,7 @@ class RleIntBlockBuilder final : public BlockBuilder { if (PREDICT_FALSE(count_ == 0)) { return Status::NotFound("No keys in the block"); } - *reinterpret_cast<CppType*>(key) = last_key_; + UnalignedStore<CppType>(key, last_key_); return Status::OK(); } @@ -371,7 +374,7 @@ class RleIntBlockDecoder final : public BlockDecoder { SeekToPositionInBlock(0); - CppType target = *reinterpret_cast<const CppType *>(value_void); + CppType target = UnalignedLoad<CppType>(value_void); while (cur_idx_ < num_elems_) { CppType cur_elem; http://git-wip-us.apache.org/repos/asf/kudu/blob/35940a12/src/kudu/common/partial_row.cc ---------------------------------------------------------------------- diff --git a/src/kudu/common/partial_row.cc b/src/kudu/common/partial_row.cc index 9ca3bcb..de7d52a 100644 --- a/src/kudu/common/partial_row.cc +++ b/src/kudu/common/partial_row.cc @@ -28,6 +28,7 @@ #include "kudu/common/row.h" #include "kudu/common/schema.h" #include "kudu/common/types.h" +#include "kudu/gutil/port.h" #include "kudu/gutil/strings/stringpiece.h" #include "kudu/gutil/strings/substitute.h" #include "kudu/util/bitmap.h" @@ -187,10 +188,12 @@ Status KuduPartialRow::Set(int32_t column_idx, const uint8_t* val) { break; }; case STRING: { + // TODO(todd) Is reinterpret_cast unsafe here? RETURN_NOT_OK(SetStringCopy(column_idx, *reinterpret_cast<const Slice*>(val))); break; }; case BINARY: { + // TODO(todd) Is reinterpret_cast unsafe here? RETURN_NOT_OK(SetBinaryCopy(column_idx, *reinterpret_cast<const Slice*>(val))); break; }; @@ -209,8 +212,7 @@ Status KuduPartialRow::Set(int32_t column_idx, const uint8_t* val) { break; }; case DECIMAL128: { - RETURN_NOT_OK(Set<TypeTraits<DECIMAL128> >(column_idx, - *reinterpret_cast<const int128_t*>(val))); + RETURN_NOT_OK(Set<TypeTraits<DECIMAL128>>(column_idx, UnalignedLoad<int128_t>(val))); break; }; default: { http://git-wip-us.apache.org/repos/asf/kudu/blob/35940a12/src/kudu/common/types.h ---------------------------------------------------------------------- diff --git a/src/kudu/common/types.h b/src/kudu/common/types.h index 145fb25..7b7aca3 100644 --- a/src/kudu/common/types.h +++ b/src/kudu/common/types.h @@ -33,6 +33,7 @@ #include "kudu/common/common.pb.h" #include "kudu/gutil/macros.h" #include "kudu/gutil/mathlimits.h" +#include "kudu/gutil/port.h" #include "kudu/gutil/strings/escaping.h" #include "kudu/gutil/strings/numbers.h" #include "kudu/util/int128.h" @@ -102,8 +103,8 @@ template<DataType Type> struct DataTypeTraits {}; template<DataType Type> static int GenericCompare(const void *lhs, const void *rhs) { typedef typename DataTypeTraits<Type>::cpp_type CppType; - CppType lhs_int = *reinterpret_cast<const CppType *>(lhs); - CppType rhs_int = *reinterpret_cast<const CppType *>(rhs); + CppType lhs_int = UnalignedLoad<CppType>(lhs); + CppType rhs_int = UnalignedLoad<CppType>(rhs); if (lhs_int < rhs_int) { return -1; } else if (lhs_int > rhs_int) { @@ -116,8 +117,8 @@ static int GenericCompare(const void *lhs, const void *rhs) { template<DataType Type> static int AreIntegersConsecutive(const void* a, const void* b) { typedef typename DataTypeTraits<Type>::cpp_type CppType; - CppType a_int = *reinterpret_cast<const CppType*>(a); - CppType b_int = *reinterpret_cast<const CppType*>(b); + CppType a_int = UnalignedLoad<CppType>(a); + CppType b_int = UnalignedLoad<CppType>(b); // Avoid overflow by checking relative position first. return a_int < b_int && a_int + 1 == b_int; } @@ -125,8 +126,8 @@ static int AreIntegersConsecutive(const void* a, const void* b) { template<DataType Type> static int AreFloatsConsecutive(const void* a, const void* b) { typedef typename DataTypeTraits<Type>::cpp_type CppType; - CppType a_float = *reinterpret_cast<const CppType*>(a); - CppType b_float = *reinterpret_cast<const CppType*>(b); + CppType a_float = UnalignedLoad<CppType>(a); + CppType b_float = UnalignedLoad<CppType>(b); return a_float < b_float && std::nextafter(a_float, b_float) == b_float; } @@ -330,7 +331,7 @@ struct DataTypeTraits<INT128> { return "int128"; } static void AppendDebugStringForValue(const void *val, std::string *str) { - str->append(SimpleItoa(*reinterpret_cast<const int128_t *>(val))); + str->append(SimpleItoa(UnalignedLoad<int128_t>(val))); } static int Compare(const void *lhs, const void *rhs) { return GenericCompare<INT128>(lhs, rhs); http://git-wip-us.apache.org/repos/asf/kudu/blob/35940a12/src/kudu/gutil/port.h ---------------------------------------------------------------------- diff --git a/src/kudu/gutil/port.h b/src/kudu/gutil/port.h index 9c66721..49881f1 100644 --- a/src/kudu/gutil/port.h +++ b/src/kudu/gutil/port.h @@ -18,6 +18,8 @@ #include <malloc.h> // for memalign() #endif +#include <type_traits> + #include "kudu/gutil/integral_types.h" // Must happens before inttypes.h inclusion */ @@ -1160,23 +1162,64 @@ inline void UNALIGNED_STORE64(void *p, uint64 v) { #if defined(__cplusplus) -inline void UnalignedCopy16(const void *src, void *dst) { - UNALIGNED_STORE16(dst, UNALIGNED_LOAD16(src)); +namespace port_internal { + +template<class T> +constexpr bool LoadByReinterpretCast() { +#ifndef NEED_ALIGNED_LOADS + // Per above, it's safe to use reinterpret_cast on x86 for types int64 and smaller. + return sizeof(T) <= 8; +#else + return false; +#endif } -inline void UnalignedCopy32(const void *src, void *dst) { - UNALIGNED_STORE32(dst, UNALIGNED_LOAD32(src)); +// Enable UnalignedLoad and UnalignedStore for numeric types (floats and ints) including int128. +// We don't allow these functions for other types, even if they are POD and <= 16 bits. +template<class T> +using enable_if_numeric = std::enable_if< + std::is_arithmetic<T>::value || std::is_same<T, __int128>::value, T>; + +} // namespace port_internal + + +// Load an integer from pointer 'src'. +// +// This is a safer equivalent of *reinterpret_cast<const T*>(src) that properly handles +// the case of larger types such as int128 which require alignment. +// +// Usage: +// int32_t x = UnalignedLoad<int32_t>(void_ptr); +// +template<typename T, + typename port_internal::enable_if_numeric<T>::type* = nullptr, + bool USE_REINTERPRET = port_internal::LoadByReinterpretCast<T>()> +inline T UnalignedLoad(const void* src) { + if (USE_REINTERPRET) { + return *reinterpret_cast<const T*>(src); + } + T ret; + memcpy(&ret, src, sizeof(T)); + return ret; } -inline void UnalignedCopy64(const void *src, void *dst) { - if (sizeof(void *) == 8) { - UNALIGNED_STORE64(dst, UNALIGNED_LOAD64(src)); - } else { - const char *src_char = reinterpret_cast<const char *>(src); - char *dst_char = reinterpret_cast<char *>(dst); - UNALIGNED_STORE32(dst_char, UNALIGNED_LOAD32(src_char)); - UNALIGNED_STORE32(dst_char + 4, UNALIGNED_LOAD32(src_char + 4)); +// Store the integer 'src' in the pointer 'dst'. +// +// Usage: +// int32_t foo = 123; +// UnalignedStore(my_void_ptr, foo); +// +// NOTE: this reverses the usual style-guide-suggested order of arguments +// to match the more natural "*p = v;" ordering of a normal store. +template<typename T, + typename port_internal::enable_if_numeric<T>::type* = nullptr, + bool USE_REINTERPRET = port_internal::LoadByReinterpretCast<T>()> +inline void UnalignedStore(void* dst, const T& src) { + if (USE_REINTERPRET) { + *reinterpret_cast<T*>(dst) = src; + } else { + memcpy(dst, &src, sizeof(T)); } } http://git-wip-us.apache.org/repos/asf/kudu/blob/35940a12/src/kudu/integration-tests/all_types-itest.cc ---------------------------------------------------------------------- diff --git a/src/kudu/integration-tests/all_types-itest.cc b/src/kudu/integration-tests/all_types-itest.cc index a5c635e..840b32f 100644 --- a/src/kudu/integration-tests/all_types-itest.cc +++ b/src/kudu/integration-tests/all_types-itest.cc @@ -223,7 +223,7 @@ struct IntKeysTestSetup { } Status VerifyRowKeyRaw(const uint8_t* raw_key, int split_idx, int row_idx) const { - CppType val = *reinterpret_cast<const CppType*>(raw_key); + CppType val = UnalignedLoad<CppType>(raw_key); return VerifyIntRowKey(val, split_idx, row_idx); } @@ -677,7 +677,7 @@ TYPED_TEST(AllTypesItest, TestTimestampPadding) { vals.expected_decimal_val); break; case DECIMAL128: - ASSERT_EQ(*reinterpret_cast<const int128_t*>(row_data), + ASSERT_EQ(UnalignedLoad<int128_t>(row_data), vals.expected_decimal_val); break; default:
