From 0133444433ac071793ebc282dad084e4dd99851f Mon Sep 17 00:00:00 2001 From: HarryLeeIBM Date: Mon, 14 Nov 2022 10:47:32 -0800 Subject: [PATCH 1/2] Fix byte order issue of wide integer for s390x --- base/base/wide_integer_impl.h | 72 +++++++++++-------- src/Common/SipHash.h | 5 ++ src/Common/hex.h | 8 ++- src/Common/tests/gtest_wide_integer.cpp | 17 +++-- .../FunctionsBinaryRepresentation.cpp | 55 ++++++++++---- src/IO/ReadHelpers.h | 2 +- src/IO/WriteHelpers.h | 2 +- 7 files changed, 108 insertions(+), 53 deletions(-) diff --git a/base/base/wide_integer_impl.h b/base/base/wide_integer_impl.h index 1b5f502722c..f5b30cbab55 100644 --- a/base/base/wide_integer_impl.h +++ b/base/base/wide_integer_impl.h @@ -187,8 +187,20 @@ struct integer::_impl static_assert(Bits % base_bits == 0); /// Simple iteration in both directions - static constexpr unsigned little(unsigned idx) { return idx; } - static constexpr unsigned big(unsigned idx) { return item_count - 1 - idx; } + static constexpr unsigned little(unsigned idx) + { + if constexpr (std::endian::native == std::endian::little) + return idx; + else + return item_count - 1 - idx; + } + static constexpr unsigned big(unsigned idx) + { + if constexpr (std::endian::native == std::endian::little) + return item_count - 1 - idx; + else + return idx; + } static constexpr unsigned any(unsigned idx) { return idx; } template @@ -240,20 +252,20 @@ struct integer::_impl { static_assert(sizeof(Integral) <= sizeof(base_type)); - self.items[0] = _impl::to_Integral(rhs); + self.items[little(0)] = _impl::to_Integral(rhs); if constexpr (std::is_signed_v) { if (rhs < 0) { - for (size_t i = 1; i < item_count; ++i) - self.items[i] = -1; + for (unsigned i = 1; i < item_count; ++i) + self.items[little(i)] = -1; return; } } - for (size_t i = 1; i < item_count; ++i) - self.items[i] = 0; + for (unsigned i = 1; i < item_count; ++i) + self.items[little(i)] = 0; } template @@ -348,7 +360,7 @@ struct integer::_impl constexpr const unsigned to_copy = min_bits / base_bits; for (unsigned i = 0; i < to_copy; ++i) - self.items[i] = rhs.items[i]; + self.items[little(i)] = rhs.items[little(i)]; if constexpr (Bits > Bits2) { @@ -357,13 +369,13 @@ struct integer::_impl if (rhs < 0) { for (unsigned i = to_copy; i < item_count; ++i) - self.items[i] = -1; + self.items[little(i)] = -1; return; } } for (unsigned i = to_copy; i < item_count; ++i) - self.items[i] = 0; + self.items[little(i)] = 0; } } @@ -454,7 +466,7 @@ private: { if constexpr (sizeof(T) <= sizeof(base_type)) { - if (0 == idx) + if (little(0) == idx) return static_cast(x); } else if (idx * sizeof(base_type) < sizeof(T)) @@ -475,7 +487,7 @@ private: for (unsigned i = 0; i < op_items; ++i) { - base_type rhs_item = get_item(rhs, i); + base_type rhs_item = get_item(rhs, little(i)); base_type & res_item = res.items[little(i)]; underflows[i] = res_item < rhs_item; @@ -508,7 +520,7 @@ private: for (unsigned i = 0; i < op_items; ++i) { - base_type rhs_item = get_item(rhs, i); + base_type rhs_item = get_item(rhs, little(i)); base_type & res_item = res.items[little(i)]; res_item += rhs_item; @@ -580,12 +592,12 @@ private: else if constexpr (Bits == 128 && sizeof(base_type) == 8) { using CompilerUInt128 = unsigned __int128; - CompilerUInt128 a = (CompilerUInt128(lhs.items[1]) << 64) + lhs.items[0]; // NOLINT(clang-analyzer-core.UndefinedBinaryOperatorResult) - CompilerUInt128 b = (CompilerUInt128(rhs.items[1]) << 64) + rhs.items[0]; // NOLINT(clang-analyzer-core.UndefinedBinaryOperatorResult) + CompilerUInt128 a = (CompilerUInt128(lhs.items[little(1)]) << 64) + lhs.items[little(0)]; // NOLINT(clang-analyzer-core.UndefinedBinaryOperatorResult) + CompilerUInt128 b = (CompilerUInt128(rhs.items[little(1)]) << 64) + rhs.items[little(0)]; // NOLINT(clang-analyzer-core.UndefinedBinaryOperatorResult) CompilerUInt128 c = a * b; integer res; - res.items[0] = c; - res.items[1] = c >> 64; + res.items[little(0)] = c; + res.items[little(1)] = c >> 64; return res; } else @@ -597,7 +609,7 @@ private: #endif for (unsigned i = 0; i < item_count; ++i) { - base_type rhs_item = get_item(rhs, i); + base_type rhs_item = get_item(rhs, little(i)); unsigned pos = i * base_bits; while (rhs_item) @@ -792,7 +804,7 @@ public: integer res; for (unsigned i = 0; i < item_count; ++i) - res.items[little(i)] = lhs.items[little(i)] | get_item(rhs, i); + res.items[little(i)] = lhs.items[little(i)] | get_item(rhs, little(i)); return res; } else @@ -810,7 +822,7 @@ public: integer res; for (unsigned i = 0; i < item_count; ++i) - res.items[little(i)] = lhs.items[little(i)] & get_item(rhs, i); + res.items[little(i)] = lhs.items[little(i)] & get_item(rhs, little(i)); return res; } else @@ -845,17 +857,17 @@ public: { using CompilerUInt128 = unsigned __int128; - CompilerUInt128 a = (CompilerUInt128(numerator.items[1]) << 64) + numerator.items[0]; // NOLINT(clang-analyzer-core.UndefinedBinaryOperatorResult) - CompilerUInt128 b = (CompilerUInt128(denominator.items[1]) << 64) + denominator.items[0]; // NOLINT(clang-analyzer-core.UndefinedBinaryOperatorResult) + CompilerUInt128 a = (CompilerUInt128(numerator.items[little(1)]) << 64) + numerator.items[little(0)]; // NOLINT(clang-analyzer-core.UndefinedBinaryOperatorResult) + CompilerUInt128 b = (CompilerUInt128(denominator.items[little(1)]) << 64) + denominator.items[little(0)]; // NOLINT(clang-analyzer-core.UndefinedBinaryOperatorResult) CompilerUInt128 c = a / b; // NOLINT integer res; - res.items[0] = c; - res.items[1] = c >> 64; + res.items[little(0)] = c; + res.items[little(1)] = c >> 64; CompilerUInt128 remainder = a - b * c; - numerator.items[0] = remainder; - numerator.items[1] = remainder >> 64; + numerator.items[little(0)] = remainder; + numerator.items[little(1)] = remainder >> 64; return res; } @@ -1039,15 +1051,15 @@ constexpr integer::integer(std::initializer_list il) noexcept else { auto it = il.begin(); - for (size_t i = 0; i < _impl::item_count; ++i) + for (unsigned i = 0; i < _impl::item_count; ++i) { if (it < il.end()) { - items[i] = *it; + items[_impl::little(i)] = *it; ++it; } else - items[i] = 0; + items[_impl::little(i)] = 0; } } } @@ -1208,7 +1220,7 @@ constexpr integer::operator T() const noexcept UnsignedT res{}; for (unsigned i = 0; i < _impl::item_count && i < (sizeof(T) + sizeof(base_type) - 1) / sizeof(base_type); ++i) - res += UnsignedT(items[i]) << (sizeof(base_type) * 8 * i); // NOLINT(clang-analyzer-core.UndefinedBinaryOperatorResult) + res += UnsignedT(items[_impl::little(i)]) << (sizeof(base_type) * 8 * i); // NOLINT(clang-analyzer-core.UndefinedBinaryOperatorResult) return res; } diff --git a/src/Common/SipHash.h b/src/Common/SipHash.h index 281a65ca36a..46680fc28df 100644 --- a/src/Common/SipHash.h +++ b/src/Common/SipHash.h @@ -164,8 +164,13 @@ public: void get128(char * out) { finalize(); +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ + unalignedStore(out + 8, v0 ^ v1); + unalignedStore(out, v2 ^ v3); +#else unalignedStore(out, v0 ^ v1); unalignedStore(out + 8, v2 ^ v3); +#endif } template diff --git a/src/Common/hex.h b/src/Common/hex.h index 424ed1f6c3e..062a6c27f76 100644 --- a/src/Common/hex.h +++ b/src/Common/hex.h @@ -58,9 +58,13 @@ inline void writeHexUIntImpl(TUInt uint_, char * out, const char * const table) value = uint_; - /// Use little endian for (size_t i = 0; i < sizeof(TUInt); ++i) - memcpy(out + i * 2, &table[static_cast(uint8[sizeof(TUInt) - 1 - i]) * 2], 2); + { + if constexpr (std::endian::native == std::endian::little) + memcpy(out + i * 2, &table[static_cast(uint8[sizeof(TUInt) - 1 - i]) * 2], 2); + else + memcpy(out + i * 2, &table[static_cast(uint8[i]) * 2], 2); + } } template diff --git a/src/Common/tests/gtest_wide_integer.cpp b/src/Common/tests/gtest_wide_integer.cpp index fa614e9390a..93c962b23cc 100644 --- a/src/Common/tests/gtest_wide_integer.cpp +++ b/src/Common/tests/gtest_wide_integer.cpp @@ -62,7 +62,7 @@ GTEST_TEST(WideInteger, Conversions) zero += minus_one; #if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ - ASSERT_EQ(0, memcmp(&zero, "\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFE\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF", sizeof(zero))); + ASSERT_EQ(0, memcmp(&zero, "\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFE", sizeof(zero))); #else ASSERT_EQ(0, memcmp(&zero, "\xFE\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF", sizeof(zero))); #endif @@ -160,7 +160,7 @@ GTEST_TEST(WideInteger, Arithmetic) zero += minus_one; #if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ - ASSERT_EQ(0, memcmp(&zero, "\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFE\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF", sizeof(zero))); + ASSERT_EQ(0, memcmp(&zero, "\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFE", sizeof(zero))); #else ASSERT_EQ(0, memcmp(&zero, "\xFE\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF", sizeof(zero))); #endif @@ -244,7 +244,7 @@ GTEST_TEST(WideInteger, Shift) auto y = x << 64; #if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ - ASSERT_EQ(0, memcmp(&y, "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01", sizeof(Int128))); + ASSERT_EQ(0, memcmp(&y, "\x00\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00\x00", sizeof(Int128))); #else ASSERT_EQ(0, memcmp(&y, "\x00\x00\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00", sizeof(Int128))); #endif @@ -261,7 +261,7 @@ GTEST_TEST(WideInteger, Shift) y = x << 16; #if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ - ASSERT_EQ(0, memcmp(&y, "\xFF\xFF\xFF\xFF\xFF\xFF\x00\x00\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF", sizeof(Int128))); + ASSERT_EQ(0, memcmp(&y, "\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\x00\x00", sizeof(Int128))); #else ASSERT_EQ(0, memcmp(&y, "\x00\x00\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF", sizeof(Int128))); #endif @@ -269,18 +269,21 @@ GTEST_TEST(WideInteger, Shift) ASSERT_EQ(0, memcmp(&y, "\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF", sizeof(Int128))); y <<= 64; +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ + ASSERT_EQ(0, memcmp(&y, "\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\x00\x00\x00\x00\x00\x00\x00\x00", sizeof(Int128))); +#else ASSERT_EQ(0, memcmp(&y, "\x00\x00\x00\x00\x00\x00\x00\x00\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF", sizeof(Int128))); - +#endif y >>= 32; #if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ - ASSERT_EQ(0, memcmp(&y, "\xFF\xFF\xFF\xFF\x00\x00\x00\x00\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF", sizeof(Int128))); + ASSERT_EQ(0, memcmp(&y, "\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\x00\x00\x00\x00", sizeof(Int128))); #else ASSERT_EQ(0, memcmp(&y, "\x00\x00\x00\x00\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF", sizeof(Int128))); #endif y <<= 64; #if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ - ASSERT_EQ(0, memcmp(&y, "\x00\x00\x00\x00\x00\x00\x00\x00\xFF\xFF\xFF\xFF\x00\x00\x00\x00", sizeof(Int128))); + ASSERT_EQ(0, memcmp(&y, "\xFF\xFF\xFF\xFF\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00", sizeof(Int128))); #else ASSERT_EQ(0, memcmp(&y, "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\xFF\xFF\xFF\xFF", sizeof(Int128))); #endif diff --git a/src/Functions/FunctionsBinaryRepresentation.cpp b/src/Functions/FunctionsBinaryRepresentation.cpp index 775696ded8a..5fac454502b 100644 --- a/src/Functions/FunctionsBinaryRepresentation.cpp +++ b/src/Functions/FunctionsBinaryRepresentation.cpp @@ -65,13 +65,27 @@ struct HexImpl } } - static void executeOneString(const UInt8 * pos, const UInt8 * end, char *& out) + static void executeOneString(const UInt8 * pos, const UInt8 * end, char *& out, bool reverse_order = false) { - while (pos < end) + if (!reverse_order) { - writeHexByteUppercase(*pos, out); - ++pos; - out += word_size; + while (pos < end) + { + writeHexByteUppercase(*pos, out); + ++pos; + out += word_size; + } + } + else + { + auto start_pos = pos; + pos = end - 1; + while (pos >= start_pos) + { + writeHexByteUppercase(*pos, out); + --pos; + out += word_size; + } } *out = '\0'; ++out; @@ -95,7 +109,8 @@ struct HexImpl for (size_t i = 0; i < size; ++i) { const UInt8 * in_pos = reinterpret_cast(&in_vec[i]); - executeOneString(in_pos, in_pos + type_size_in_bytes, out); + bool reverse_order = (std::endian::native == std::endian::big); + executeOneString(in_pos, in_pos + type_size_in_bytes, out, reverse_order); pos += hex_length; out_offsets[i] = pos; @@ -174,7 +189,9 @@ struct BinImpl for (size_t i = 0; i < size; ++i) { const UInt8 * in_pos = reinterpret_cast(&in_vec[i]); - executeOneString(in_pos, in_pos + type_size_in_bytes, out); + + bool reverse_order = (std::endian::native == std::endian::big); + executeOneString(in_pos, in_pos + type_size_in_bytes, out, reverse_order); pos += hex_length; out_offsets[i] = pos; @@ -182,13 +199,27 @@ struct BinImpl col_res = std::move(col_str); } - static void executeOneString(const UInt8 * pos, const UInt8 * end, char *& out) + static void executeOneString(const UInt8 * pos, const UInt8 * end, char *& out, bool reverse_order = false) { - while (pos < end) + if (!reverse_order) { - writeBinByte(*pos, out); - ++pos; - out += word_size; + while (pos < end) + { + writeBinByte(*pos, out); + ++pos; + out += word_size; + } + } + else + { + auto start_pos = pos; + pos = end - 1; + while (pos >= start_pos) + { + writeBinByte(*pos, out); + --pos; + out += word_size; + } } *out = '\0'; ++out; diff --git a/src/IO/ReadHelpers.h b/src/IO/ReadHelpers.h index 27a24eef804..8d7b46e511e 100644 --- a/src/IO/ReadHelpers.h +++ b/src/IO/ReadHelpers.h @@ -1068,7 +1068,7 @@ inline void readBinaryBigEndian(T & x, ReadBuffer & buf) /// Assuming little { for (size_t i = 0; i != std::size(x.items); ++i) { - auto & item = x.items[std::size(x.items) - i - 1]; + auto & item = x.items[(std::endian::native == std::endian::little) ? std::size(x.items) - i - 1 : i]; readBinaryBigEndian(item, buf); } } diff --git a/src/IO/WriteHelpers.h b/src/IO/WriteHelpers.h index 39024b33eb1..003e5a56958 100644 --- a/src/IO/WriteHelpers.h +++ b/src/IO/WriteHelpers.h @@ -1126,7 +1126,7 @@ inline void writeBinaryBigEndian(const T & x, WriteBuffer & buf) /// Assuming { for (size_t i = 0; i != std::size(x.items); ++i) { - const auto & item = x.items[std::size(x.items) - i - 1]; + const auto & item = x.items[(std::endian::native == std::endian::little) ? std::size(x.items) - i - 1 : i]; writeBinaryBigEndian(item, buf); } } From 8e07e37fe78b83a07fc5b8320b3dd9b2837a476b Mon Sep 17 00:00:00 2001 From: Robert Schulze Date: Wed, 7 Dec 2022 09:38:03 +0100 Subject: [PATCH 2/2] Update FunctionsBinaryRepresentation.cpp --- src/Functions/FunctionsBinaryRepresentation.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Functions/FunctionsBinaryRepresentation.cpp b/src/Functions/FunctionsBinaryRepresentation.cpp index 7542ce4bb03..f71f05bbf34 100644 --- a/src/Functions/FunctionsBinaryRepresentation.cpp +++ b/src/Functions/FunctionsBinaryRepresentation.cpp @@ -78,7 +78,7 @@ struct HexImpl } else { - auto start_pos = pos; + const auto * start_pos = pos; pos = end - 1; while (pos >= start_pos) { @@ -212,7 +212,7 @@ struct BinImpl } else { - auto start_pos = pos; + const auto * start_pos = pos; pos = end - 1; while (pos >= start_pos) {