[ntuple] fix bit truncation on big endian

Moves the endianess handling inside BitPacking::[Pack|Unpack]Bits.
Fixes truncated and quantized real values on big endian architectures.

Author: jblomer

tree/ntuple/src/RColumnElement.cxx

@@ -212,7 +212,12 @@ void ROOT::Internal::BitPacking::PackBits(void *dst, const void *src, std::size_
    std::size_t dstIdx = 0;
    for (std::size_t i = 0; i < count; ++i) {
       Word_t packedWord = 0;
+#if R__LITTLE_ENDIAN == 0
+      memcpy(reinterpret_cast<unsigned char *>(&packedWord) + (sizeof(Word_t) - sizeofSrc), srcArray + i * sizeofSrc,
+             sizeofSrc);
+#else
       memcpy(&packedWord, srcArray + i * sizeofSrc, sizeofSrc);
+#endif
       // truncate the LSB of the item
       packedWord >>= sizeofSrc * 8 - nDstBits;
 
@@ -231,6 +236,7 @@ void ROOT::Internal::BitPacking::PackBits(void *dst, const void *src, std::size_
             accum |= (packedWordLsb << bitsUsed);
          }
 
+         ByteSwapIfNecessary(accum);
          memcpy(&dstArray[dstIdx++], &accum, sizeof(accum));
          accum = 0;
          bitsUsed = 0;
@@ -248,8 +254,10 @@ void ROOT::Internal::BitPacking::PackBits(void *dst, const void *src, std::size_
       }
    }
 
-   if (bitsUsed)
+   if (bitsUsed) {
+      ByteSwapIfNecessary(accum);
       memcpy(&dstArray[dstIdx++], &accum, (bitsUsed + 7) / 8);
+   }
 
    [[maybe_unused]] auto expDstCount = (count * nDstBits + kBitsPerWord - 1) / kBitsPerWord;
    assert(dstIdx == expDstCount);
@@ -277,6 +285,7 @@ void ROOT::Internal::BitPacking::UnpackBits(void *dst, const void *src, std::siz
       Word_t packedBytes = 0;
       std::size_t bytesLoaded = std::min(remBytesToLoad, sizeof(Word_t));
       memcpy(&packedBytes, &srcArray[i], bytesLoaded);
+      ByteSwapIfNecessary(packedBytes);
 
       assert(remBytesToLoad >= bytesLoaded);
       remBytesToLoad -= bytesLoaded;
@@ -288,7 +297,12 @@ void ROOT::Internal::BitPacking::UnpackBits(void *dst, const void *src, std::siz
          std::uint32_t msb = packedBytes << (8 * sizeofDst - nMsb);
          Word_t packedWord = msb | prevWordLsb;
          prevWordLsb = 0;
+#if R__LITTLE_ENDIAN == 0
+         memcpy(dstArray + dstIdx * sizeofDst,
+                reinterpret_cast<unsigned char *>(&packedWord) + sizeof(Word_t) - sizeofDst, sizeofDst);
+#else
          memcpy(dstArray + dstIdx * sizeofDst, &packedWord, sizeofDst);
+#endif
          ++dstIdx;
          offInWord = nMsb;
       }
@@ -310,7 +324,12 @@ void ROOT::Internal::BitPacking::UnpackBits(void *dst, const void *src, std::siz
          assert(nSrcBits + offInWord <= kBitsPerWord);
          packedWord >>= offInWord;
          packedWord <<= 8 * sizeofDst - nSrcBits;
+#if R__LITTLE_ENDIAN == 0
+         memcpy(dstArray + dstIdx * sizeofDst,
+                reinterpret_cast<unsigned char *>(&packedWord) + sizeof(Word_t) - sizeofDst, sizeofDst);
+#else
          memcpy(dstArray + dstIdx * sizeofDst, &packedWord, sizeofDst);
+#endif
          ++dstIdx;
          offInWord += nSrcBits;
       }

tree/ntuple/src/RColumnElement.hxx

@@ -925,16 +925,7 @@ public:
 
       R__ASSERT(GetPackedSize(count) == MinBufSize(count, fBitsOnStorage));
 
-#if R__LITTLE_ENDIAN == 0
-      // TODO(gparolini): to avoid this extra allocation we might want to perform byte swapping
-      // directly in the Pack/UnpackBits functions.
-      auto bswapped = MakeUninitArray<float>(count);
-      CopyBswap<sizeof(float)>(bswapped.get(), src, count);
-      const auto *srcLe = bswapped.get();
-#else
-      const auto *srcLe = reinterpret_cast<const float *>(src);
-#endif
-      PackBits(dst, srcLe, count, sizeof(float), fBitsOnStorage);
+      PackBits(dst, src, count, sizeof(float), fBitsOnStorage);
    }
 
    void Unpack(void *dst, const void *src, std::size_t count) const final
@@ -944,9 +935,6 @@ public:
       R__ASSERT(GetPackedSize(count) == MinBufSize(count, fBitsOnStorage));
 
       UnpackBits(dst, src, count, sizeof(float), fBitsOnStorage);
-#if R__LITTLE_ENDIAN == 0
-      InPlaceBswap<sizeof(float)>(dst, count);
-#endif
    }
 };
 
@@ -966,16 +954,7 @@ public:
       for (std::size_t i = 0; i < count; ++i)
          srcFloat[i] = static_cast<float>(srcDouble[i]);
 
-#if R__LITTLE_ENDIAN == 0
-      // TODO(gparolini): to avoid this extra allocation we might want to perform byte swapping
-      // directly in the Pack/UnpackBits functions.
-      auto bswapped = MakeUninitArray<float>(count);
-      CopyBswap<sizeof(float)>(bswapped.get(), srcFloat.get(), count);
-      const float *srcLe = bswapped.get();
-#else
-      const float *srcLe = reinterpret_cast<const float *>(srcFloat.get());
-#endif
-      PackBits(dst, srcLe, count, sizeof(float), fBitsOnStorage);
+      PackBits(dst, reinterpret_cast<const float *>(srcFloat.get()), count, sizeof(float), fBitsOnStorage);
    }
 
    void Unpack(void *dst, const void *src, std::size_t count) const final
@@ -987,9 +966,6 @@ public:
       // TODO(gparolini): avoid this allocation
       auto dstFloat = MakeUninitArray<float>(count);
       UnpackBits(dstFloat.get(), src, count, sizeof(float), fBitsOnStorage);
-#if R__LITTLE_ENDIAN == 0
-      InPlaceBswap<sizeof(float)>(dstFloat.get(), count);
-#endif
 
       double *dstDouble = reinterpret_cast<double *>(dst);
       for (std::size_t i = 0; i < count; ++i)
@@ -1033,7 +1009,6 @@ int QuantizeReals(Quantized_t *dst, const T *src, std::size_t count, double min,
 
       const double e = 0.5 + (elem - min) * scale;
       Quantized_t q = static_cast<Quantized_t>(e);
-      ByteSwapIfNecessary(q);
 
       // double-check we actually used at most `nQuantBits`
       assert(outOfRange || ROOT::Internal::LeadingZeroes(q) >= unusedBits);
@@ -1068,7 +1043,6 @@ int UnquantizeReals(T *dst, const Quantized_t *src, std::size_t count, double mi
       // Undo the LSB-preserving shift performed by QuantizeReals
       assert(ROOT::Internal::TrailingZeroes(elem) >= unusedBits);
       elem >>= unusedBits;
-      ByteSwapIfNecessary(elem);
 
       const double fq = static_cast<double>(elem);
       double e = fq * scale + min;

tree/ntuple/test/ntuple_endian.cxx

@@ -224,27 +224,3 @@ TEST(RColumnElementEndian, DeltaSplit)
                        32));
 #endif
 }
-
-TEST(RColumnElementEndian, Real32Trunc)
-{
-#ifndef R__BYTESWAP
-   GTEST_SKIP() << "Skipping test on big endian node";
-#else
-   RColumnElement<float, ENTupleColumnType::kReal32Trunc> element;
-   element.SetBitsOnStorage(12);
-
-   RPageSinkMock sink1(element);
-   unsigned char buf1[] = {0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
-                           0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f};
-   RPage page1(buf1, nullptr, sizeof(float), 4);
-   page1.GrowUnchecked(4);
-   sink1.CommitPage(RPageStorage::ColumnHandle_t{}, page1);
-
-   EXPECT_EQ(0, memcmp(sink1.GetPages()[0].GetBuffer(), "\x00\x00\x04\x80\x00\x0c", 6));
-
-   RPageSourceMock source1(sink1.GetPages(), element);
-   auto page2Ref = source1.LoadPage(RPageStorage::ColumnHandle_t{}, NTupleSize_t{0});
-   EXPECT_EQ(
-      0, memcmp(page2Ref.Get().GetBuffer(), "\x00\x00\x00\x00\x04\x00\x00\x00\x08\x00\x00\x00\x0c\x00\x00\x00", 16));
-#endif
-}