Skip to content

[ntuple] fix bit truncation on big endian #22358

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jblomer merged 1 commit into from
May 22, 2026
+22 −53
Merged

[ntuple] fix bit truncation on big endian #22358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion tree/ntuple/src/RColumnElement.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,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;

Expand All @@ -232,6 +237,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;
Expand All @@ -249,8 +255,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);
Expand Down Expand Up @@ -278,6 +286,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;
Expand All @@ -289,7 +298,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;
}
Expand All @@ -311,7 +325,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;
}
Expand Down
30 changes: 2 additions & 28 deletions tree/ntuple/src/RColumnElement.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}
};

Expand All @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down
24 changes: 0 additions & 24 deletions tree/ntuple/test/ntuple_endian.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Loading