From ca5ac4e5ce9d695f6a7bd2451a54cfb3cc84e1e6 Mon Sep 17 00:00:00 2001 From: Tamas Gal Date: Thu, 23 Jul 2026 11:49:24 +0200 Subject: [PATCH 1/9] fix TStreamerLoop counter offset in member-wise streaming --- io/io/src/TStreamerInfoActions.cxx | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/io/io/src/TStreamerInfoActions.cxx b/io/io/src/TStreamerInfoActions.cxx index bebd784eacd7d..7ea45f3a1d6e6 100644 --- a/io/io/src/TStreamerInfoActions.cxx +++ b/io/io/src/TStreamerInfoActions.cxx @@ -238,12 +238,29 @@ namespace TStreamerInfoActions struct TConfStreamerLoop : public TConfiguration { bool fIsPtrPtr = false; // Which are we, an array of objects or an array of pointers to objects? + Longptr_t fCounterOffset = 0; // Offset of the '//[n]' counter, relative to the streamed object. TConfStreamerLoop(TVirtualStreamerInfo *info, UInt_t id, TCompInfo_t *compinfo, Int_t offset, bool isPtrPtr) - : TConfiguration(info, id, compinfo, offset), fIsPtrPtr(isPtrPtr) + : TConfiguration(info, id, compinfo, offset), fIsPtrPtr(isPtrPtr), + fCounterOffset((Longptr_t)compinfo->fMethod) { } + void AddToOffset(Int_t delta) override + { + // Add the (potentially negative) delta to the configuration's offsets. This is used by + // TBranchElement in the case of a split sub-object and by the member-wise streaming of a + // base class, where the whole action sequence of the base is shifted by the base-class + // offset. The counter of the variable-size array lives in the same class as the array, + // so its offset must be shifted by the same delta as fOffset; it cannot be taken from + // the shared fCompInfo->fMethod, which stays relative to the declaring (base) class. + + if (fOffset != TVirtualStreamerInfo::kMissing) { + fOffset += delta; + fCounterOffset += delta; + } + } + TConfiguration *Copy() override { return new TConfStreamerLoop(*this); }; }; @@ -1572,7 +1589,7 @@ namespace TStreamerInfoActions UInt_t ioffset = actionConfig->fOffset; // Get any private streamer which was set for the data member. TMemberStreamer* pstreamer = actionConfig->fCompInfo->fStreamer; - Int_t* counter = (Int_t*) ((char *) addr /*entry pointer*/ + actionConfig->fCompInfo->fMethod /*counter offset*/); + Int_t* counter = (Int_t*) ((char *) addr /*entry pointer*/ + ((const TConfStreamerLoop*)actionConfig)->fCounterOffset /*counter offset*/); // And call the private streamer, passing it the buffer, the object, and the counter. (*pstreamer)(buf, (char *) addr /*entry pointer*/ + ioffset /*object offset*/, *counter); return 0; @@ -1586,7 +1603,7 @@ namespace TStreamerInfoActions bool isPtrPtr = ((TConfStreamerLoop*)config)->fIsPtrPtr; // Get the counter for the varying length array. - Int_t vlen = *((Int_t*) ((char *) addr /*entry pointer*/ + config->fCompInfo->fMethod /*counter offset*/)); + Int_t vlen = *((Int_t*) ((char *) addr /*entry pointer*/ + ((const TConfStreamerLoop*)config)->fCounterOffset /*counter offset*/)); //b << vlen; if (vlen) { @@ -1629,7 +1646,7 @@ namespace TStreamerInfoActions bool isPtrPtr = ((TConfStreamerLoop*)config)->fIsPtrPtr; // Get the counter for the varying length array. - Int_t vlen = *((Int_t*) ((char *) addr /*entry pointer*/ + config->fCompInfo->fMethod /*counter offset*/)); + Int_t vlen = *((Int_t*) ((char *) addr /*entry pointer*/ + ((const TConfStreamerLoop*)config)->fCounterOffset /*counter offset*/)); //b << vlen; if (vlen) { // Get a pointer to the array of pointers. @@ -1723,7 +1740,7 @@ namespace TStreamerInfoActions // Get the counter for the varying length array. Int_t vlen = *((Int_t *)((char *)addr /*entry pointer*/ + - config->fCompInfo->fMethod /*counter offset*/)); + ((const TConfStreamerLoop*)config)->fCounterOffset /*counter offset*/)); // Int_t realLen; // b >> realLen; // if (realLen != vlen) { @@ -1821,7 +1838,7 @@ namespace TStreamerInfoActions // Get the counter for the varying length array. Int_t vlen = *((Int_t *)((char *)addr /*entry pointer*/ + - config->fCompInfo->fMethod /*counter offset*/)); + ((const TConfStreamerLoop*)config)->fCounterOffset /*counter offset*/)); // Int_t realLen; // b >> realLen; // if (realLen != vlen) { From 0bb5c01b583c7f264addd86e78d1a6e59b7b8d26 Mon Sep 17 00:00:00 2001 From: Tamas Gal Date: Thu, 23 Jul 2026 12:00:44 +0200 Subject: [PATCH 2/9] add tests for TStreamerLoop member-wise streaming --- io/io/test/CMakeLists.txt | 8 +++ io/io/test/StreamerLoopMemberwise.cxx | 1 + io/io/test/StreamerLoopMemberwise.hxx | 72 ++++++++++++++++++++++ io/io/test/StreamerLoopMemberwiseLinkDef.h | 13 ++++ io/io/test/TStreamerLoopMemberwise.cxx | 72 ++++++++++++++++++++++ 5 files changed, 166 insertions(+) create mode 100644 io/io/test/StreamerLoopMemberwise.cxx create mode 100644 io/io/test/StreamerLoopMemberwise.hxx create mode 100644 io/io/test/StreamerLoopMemberwiseLinkDef.h create mode 100644 io/io/test/TStreamerLoopMemberwise.cxx diff --git a/io/io/test/CMakeLists.txt b/io/io/test/CMakeLists.txt index 78fa44aa014d9..3f947bddfdac2 100644 --- a/io/io/test/CMakeLists.txt +++ b/io/io/test/CMakeLists.txt @@ -37,6 +37,14 @@ endif() ROOT_ADD_GTEST(ZipHeader ZipHeader.cxx LIBRARIES RIO Tree ROOTNTuple) +ROOT_STANDARD_LIBRARY_PACKAGE(StreamerLoopMemberwise + NO_INSTALL_HEADERS + HEADERS ${CMAKE_CURRENT_SOURCE_DIR}/StreamerLoopMemberwise.hxx + SOURCES StreamerLoopMemberwise.cxx + LINKDEF StreamerLoopMemberwiseLinkDef.h + DEPENDENCIES RIO Tree) +ROOT_ADD_GTEST(TStreamerLoopMemberwise TStreamerLoopMemberwise.cxx LIBRARIES RIO Tree StreamerLoopMemberwise) + # Temporarily disabled. Test routinely fails on MacOS and some Linuxes. #if(NOT WIN32 AND (NOT MACOS_VERSION OR NOT MACOSX_VERSION VERSION_LESS 13.00)) # ROOT_EXECUTABLE(TMapFileTest TMapFileTest.cxx LIBRARIES RIO Hist New) diff --git a/io/io/test/StreamerLoopMemberwise.cxx b/io/io/test/StreamerLoopMemberwise.cxx new file mode 100644 index 0000000000000..49968380467a2 --- /dev/null +++ b/io/io/test/StreamerLoopMemberwise.cxx @@ -0,0 +1 @@ +#include "StreamerLoopMemberwise.hxx" diff --git a/io/io/test/StreamerLoopMemberwise.hxx b/io/io/test/StreamerLoopMemberwise.hxx new file mode 100644 index 0000000000000..f1c4378308aea --- /dev/null +++ b/io/io/test/StreamerLoopMemberwise.hxx @@ -0,0 +1,72 @@ +#ifndef ROOT_TEST_STREAMERLOOP_MEMBERWISE +#define ROOT_TEST_STREAMERLOOP_MEMBERWISE + +#include + +// Classes for the regression test of member-wise streaming of a variable-size +// array (`//[n]`, a TStreamerLoop element) that lives in a base class at a +// non-zero offset. See io/io/src/TStreamerInfoActions.cxx +// (TConfStreamerLoop::fCounterOffset) and TStreamerLoopMemberwise.cxx. + +namespace ROOTTest { +namespace StreamerLoopMemberwise { + +// Element stored in the variable-size array (a class, so the array is a +// TStreamerLoop and not a TStreamerBasicPointer). +class Hit { +public: + Hit() = default; + Hit(int a, int b) : fA(a), fB(b) {} + bool operator==(const Hit &o) const { return fA == o.fA && fB == o.fB; } + int fA = 0; + int fB = 0; + ClassDefNV(Hit, 1) +}; + +class Frame { +public: + Frame() = default; + ~Frame() { delete[] fHits; } + Frame(const Frame &o) { Set(o.fN, o.fHits); } + Frame &operator=(const Frame &o) + { + if (this != &o) + Set(o.fN, o.fHits); + return *this; + } + void Set(int n, const Hit *hits) + { + delete[] fHits; + fHits = nullptr; + fN = 0; + if (n > 0) { + fHits = new Hit[n]; + for (int i = 0; i < n; ++i) + fHits[i] = hits[i]; + fN = n; + } + } + int fN = 0; + Hit *fHits = nullptr; //[fN] + ClassDef(Frame, 1) +}; + +// A base preceding Frame, so that the Frame base does not sit at offset 0. +class Pad { +public: + Pad() = default; + int fX = 0; + ClassDef(Pad, 1) +}; + +// The collection element: Frame is a base at a non-zero offset. +class Super : public Pad, public Frame { +public: + Super() = default; + ClassDef(Super, 1) +}; + +} // namespace StreamerLoopMemberwise +} // namespace ROOTTest + +#endif diff --git a/io/io/test/StreamerLoopMemberwiseLinkDef.h b/io/io/test/StreamerLoopMemberwiseLinkDef.h new file mode 100644 index 0000000000000..57fd4796b4e10 --- /dev/null +++ b/io/io/test/StreamerLoopMemberwiseLinkDef.h @@ -0,0 +1,13 @@ +#ifdef __CLING__ + +#pragma link off all globals; +#pragma link off all classes; +#pragma link off all functions; + +#pragma link C++ class ROOTTest::StreamerLoopMemberwise::Hit + ; +#pragma link C++ class ROOTTest::StreamerLoopMemberwise::Frame + ; +#pragma link C++ class ROOTTest::StreamerLoopMemberwise::Pad + ; +#pragma link C++ class ROOTTest::StreamerLoopMemberwise::Super + ; +#pragma link C++ class std::vector < ROOTTest::StreamerLoopMemberwise::Super> + ; + +#endif diff --git a/io/io/test/TStreamerLoopMemberwise.cxx b/io/io/test/TStreamerLoopMemberwise.cxx new file mode 100644 index 0000000000000..a85d2cd773ab5 --- /dev/null +++ b/io/io/test/TStreamerLoopMemberwise.cxx @@ -0,0 +1,72 @@ +#include "StreamerLoopMemberwise.hxx" + +#include "gtest/gtest.h" + +#include +#include +#include + +#include + +using namespace ROOTTest::StreamerLoopMemberwise; + +// Regression test for the member-wise streaming of a variable-size array +// (`Hit* fHits; //[fN]`, i.e. a TStreamerLoop element) that lives in a base +// class (Frame) at a non-zero offset inside the collection element (Super). +// +// See https://github.com/root-project/root/issues/22895 for more information. +TEST(TStreamerLoopMemberwise, VariableArrayInBaseClass) +{ + const char *fname = "streamerloop_memberwise.root"; + const int kFrames = 4; + + int expectedHits = 0; + for (int f = 0; f < kFrames; ++f) + expectedHits += f + 1; // every frame non-empty; n > 0 is what triggered the original bug + + { + std::vector slice(kFrames); + for (int f = 0; f < kFrames; ++f) { + std::vector hits; + for (int i = 0; i <= f; ++i) + hits.emplace_back(100 * f + i, 7); + slice[f].Set(static_cast(hits.size()), hits.data()); + } + + TFile file(fname, "RECREATE"); + TTree tree("T", "T"); + std::vector *ptr = &slice; + tree.Branch("slice", &ptr); // default split level -> member-wise collection + tree.Fill(); + tree.Write(); + } + + int readHits = 0; + int nullBuffers = 0; + { + TFile file(fname); + auto *tree = file.Get("T"); + ASSERT_NE(tree, nullptr); + std::vector *slice = nullptr; + tree->SetBranchAddress("slice", &slice); + ASSERT_GT(tree->GetEntry(0), 0); + ASSERT_NE(slice, nullptr); + ASSERT_EQ(static_cast(slice->size()), kFrames); + + for (int f = 0; f < kFrames; ++f) { + const Frame &frame = (*slice)[f]; + EXPECT_EQ(frame.fN, f + 1); + if (frame.fN > 0 && frame.fHits == nullptr) + ++nullBuffers; + for (int i = 0; frame.fHits && i < frame.fN; ++i) { + EXPECT_EQ(frame.fHits[i], Hit(100 * f + i, 7)); + ++readHits; + } + } + } + + EXPECT_EQ(nullBuffers, 0); + EXPECT_EQ(readHits, expectedHits); + + gSystem->Unlink(fname); +} From c4493be5cdba5c41fe2401821b38638fb317452d Mon Sep 17 00:00:00 2001 From: Tamas Gal Date: Fri, 24 Jul 2026 19:02:27 +0200 Subject: [PATCH 3/9] Update io/io/test/CMakeLists.txt Co-authored-by: Jakob Blomer --- io/io/test/CMakeLists.txt | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/io/io/test/CMakeLists.txt b/io/io/test/CMakeLists.txt index 3f947bddfdac2..983016d2592a8 100644 --- a/io/io/test/CMakeLists.txt +++ b/io/io/test/CMakeLists.txt @@ -37,13 +37,11 @@ endif() ROOT_ADD_GTEST(ZipHeader ZipHeader.cxx LIBRARIES RIO Tree ROOTNTuple) -ROOT_STANDARD_LIBRARY_PACKAGE(StreamerLoopMemberwise - NO_INSTALL_HEADERS - HEADERS ${CMAKE_CURRENT_SOURCE_DIR}/StreamerLoopMemberwise.hxx - SOURCES StreamerLoopMemberwise.cxx - LINKDEF StreamerLoopMemberwiseLinkDef.h - DEPENDENCIES RIO Tree) -ROOT_ADD_GTEST(TStreamerLoopMemberwise TStreamerLoopMemberwise.cxx LIBRARIES RIO Tree StreamerLoopMemberwise) +ROOT_GENERATE_DICTIONARY(StreamerLoopMemberwiseDict ${CMAKE_CURRENT_SOURCE_DIR}/StreamerLoopMemberwise.hxx + MODULE TStreamerLoopMemberwise + LINKDEF StreamerLoopMemberwiseLinkDef.h + DEPENDENCIES RIO Tree) +ROOT_ADD_GTEST(TStreamerLoopMemberwise TStreamerLoopMemberwise.cxx LIBRARIES RIO Tree) # Temporarily disabled. Test routinely fails on MacOS and some Linuxes. #if(NOT WIN32 AND (NOT MACOS_VERSION OR NOT MACOSX_VERSION VERSION_LESS 13.00)) From cbfe71e2b2f25a19d0beefc467c634460fd649bb Mon Sep 17 00:00:00 2001 From: Tamas Gal Date: Fri, 24 Jul 2026 19:02:44 +0200 Subject: [PATCH 4/9] Update io/io/src/TStreamerInfoActions.cxx Co-authored-by: Jakob Blomer --- io/io/src/TStreamerInfoActions.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/io/io/src/TStreamerInfoActions.cxx b/io/io/src/TStreamerInfoActions.cxx index 7ea45f3a1d6e6..76d034ba3aa53 100644 --- a/io/io/src/TStreamerInfoActions.cxx +++ b/io/io/src/TStreamerInfoActions.cxx @@ -238,7 +238,7 @@ namespace TStreamerInfoActions struct TConfStreamerLoop : public TConfiguration { bool fIsPtrPtr = false; // Which are we, an array of objects or an array of pointers to objects? - Longptr_t fCounterOffset = 0; // Offset of the '//[n]' counter, relative to the streamed object. + Int_t fCounterOffset = 0; // Offset of the '//[n]' counter, relative to the streamed object. TConfStreamerLoop(TVirtualStreamerInfo *info, UInt_t id, TCompInfo_t *compinfo, Int_t offset, bool isPtrPtr) : TConfiguration(info, id, compinfo, offset), fIsPtrPtr(isPtrPtr), From 7b2dd5ceb62052a0c15ea6ae115f8e6642e60be5 Mon Sep 17 00:00:00 2001 From: Tamas Gal Date: Fri, 24 Jul 2026 20:51:04 +0200 Subject: [PATCH 5/9] use FileRaii --- io/io/test/TStreamerLoopMemberwise.cxx | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/io/io/test/TStreamerLoopMemberwise.cxx b/io/io/test/TStreamerLoopMemberwise.cxx index a85d2cd773ab5..16e04dc97fc01 100644 --- a/io/io/test/TStreamerLoopMemberwise.cxx +++ b/io/io/test/TStreamerLoopMemberwise.cxx @@ -2,13 +2,14 @@ #include "gtest/gtest.h" +#include #include #include -#include #include using namespace ROOTTest::StreamerLoopMemberwise; +using ROOT::TestSupport::FileRaii; // Regression test for the member-wise streaming of a variable-size array // (`Hit* fHits; //[fN]`, i.e. a TStreamerLoop element) that lives in a base @@ -17,7 +18,7 @@ using namespace ROOTTest::StreamerLoopMemberwise; // See https://github.com/root-project/root/issues/22895 for more information. TEST(TStreamerLoopMemberwise, VariableArrayInBaseClass) { - const char *fname = "streamerloop_memberwise.root"; + FileRaii fileGuard("streamerloop_memberwise.root"); const int kFrames = 4; int expectedHits = 0; @@ -33,7 +34,7 @@ TEST(TStreamerLoopMemberwise, VariableArrayInBaseClass) slice[f].Set(static_cast(hits.size()), hits.data()); } - TFile file(fname, "RECREATE"); + TFile file(fileGuard.GetPath().c_str(), "RECREATE"); TTree tree("T", "T"); std::vector *ptr = &slice; tree.Branch("slice", &ptr); // default split level -> member-wise collection @@ -44,7 +45,7 @@ TEST(TStreamerLoopMemberwise, VariableArrayInBaseClass) int readHits = 0; int nullBuffers = 0; { - TFile file(fname); + TFile file(fileGuard.GetPath().c_str()); auto *tree = file.Get("T"); ASSERT_NE(tree, nullptr); std::vector *slice = nullptr; @@ -67,6 +68,4 @@ TEST(TStreamerLoopMemberwise, VariableArrayInBaseClass) EXPECT_EQ(nullBuffers, 0); EXPECT_EQ(readHits, expectedHits); - - gSystem->Unlink(fname); } From 57c8673d3539babdffafef7bfcd96ce7e9f14a55 Mon Sep 17 00:00:00 2001 From: Tamas Gal Date: Fri, 24 Jul 2026 21:26:01 +0200 Subject: [PATCH 6/9] rewording and fixing the note on offsets --- io/io/src/TStreamerInfoActions.cxx | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/io/io/src/TStreamerInfoActions.cxx b/io/io/src/TStreamerInfoActions.cxx index 76d034ba3aa53..8dbd573fc5541 100644 --- a/io/io/src/TStreamerInfoActions.cxx +++ b/io/io/src/TStreamerInfoActions.cxx @@ -248,12 +248,13 @@ namespace TStreamerInfoActions void AddToOffset(Int_t delta) override { - // Add the (potentially negative) delta to the configuration's offsets. This is used by + // Add the (potentially negative) delta to the configuration's offsets. This is used by // TBranchElement in the case of a split sub-object and by the member-wise streaming of a // base class, where the whole action sequence of the base is shifted by the base-class - // offset. The counter of the variable-size array lives in the same class as the array, - // so its offset must be shifted by the same delta as fOffset; it cannot be taken from - // the shared fCompInfo->fMethod, which stays relative to the declaring (base) class. + // offset. fCounterOffset locates the '//[n]' counter inside the same object as fOffset, + // no matter if it's declared in that class or intherited from a base, so it takes the + // same shift. It is kept per configuration because fCompInfo->fMethod is shared by + // every sequence built from this streamerinfo and has to stay unshifted. if (fOffset != TVirtualStreamerInfo::kMissing) { fOffset += delta; From 41d95f703d60e2463c25f0b601c3d66340e56a08 Mon Sep 17 00:00:00 2001 From: Tamas Gal Date: Fri, 24 Jul 2026 21:28:53 +0200 Subject: [PATCH 7/9] followup fix of the fCounterOffset type change --- io/io/src/TStreamerInfoActions.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/io/io/src/TStreamerInfoActions.cxx b/io/io/src/TStreamerInfoActions.cxx index 8dbd573fc5541..deefa7a798183 100644 --- a/io/io/src/TStreamerInfoActions.cxx +++ b/io/io/src/TStreamerInfoActions.cxx @@ -242,7 +242,7 @@ namespace TStreamerInfoActions TConfStreamerLoop(TVirtualStreamerInfo *info, UInt_t id, TCompInfo_t *compinfo, Int_t offset, bool isPtrPtr) : TConfiguration(info, id, compinfo, offset), fIsPtrPtr(isPtrPtr), - fCounterOffset((Longptr_t)compinfo->fMethod) + fCounterOffset(static_cast(compinfo->fMethod)) { } From dca4eee49d4d3eb678bdbb1b243d0676b9de9f56 Mon Sep 17 00:00:00 2001 From: Tamas Gal Date: Sun, 26 Jul 2026 23:10:48 +0200 Subject: [PATCH 8/9] remove unnecessary cxx --- io/io/test/StreamerLoopMemberwise.cxx | 1 - 1 file changed, 1 deletion(-) delete mode 100644 io/io/test/StreamerLoopMemberwise.cxx diff --git a/io/io/test/StreamerLoopMemberwise.cxx b/io/io/test/StreamerLoopMemberwise.cxx deleted file mode 100644 index 49968380467a2..0000000000000 --- a/io/io/test/StreamerLoopMemberwise.cxx +++ /dev/null @@ -1 +0,0 @@ -#include "StreamerLoopMemberwise.hxx" From 600799a7b771d1bf6c490cb321b72ca2b749fa6f Mon Sep 17 00:00:00 2001 From: Tamas Gal Date: Sun, 26 Jul 2026 23:15:21 +0200 Subject: [PATCH 9/9] correct the order in tests... --- io/io/test/CMakeLists.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/io/io/test/CMakeLists.txt b/io/io/test/CMakeLists.txt index 983016d2592a8..797f2da89fde7 100644 --- a/io/io/test/CMakeLists.txt +++ b/io/io/test/CMakeLists.txt @@ -37,11 +37,12 @@ endif() ROOT_ADD_GTEST(ZipHeader ZipHeader.cxx LIBRARIES RIO Tree ROOTNTuple) +ROOT_ADD_GTEST(TStreamerLoopMemberwise TStreamerLoopMemberwise.cxx LIBRARIES RIO Tree) + ROOT_GENERATE_DICTIONARY(StreamerLoopMemberwiseDict ${CMAKE_CURRENT_SOURCE_DIR}/StreamerLoopMemberwise.hxx MODULE TStreamerLoopMemberwise LINKDEF StreamerLoopMemberwiseLinkDef.h DEPENDENCIES RIO Tree) -ROOT_ADD_GTEST(TStreamerLoopMemberwise TStreamerLoopMemberwise.cxx LIBRARIES RIO Tree) # Temporarily disabled. Test routinely fails on MacOS and some Linuxes. #if(NOT WIN32 AND (NOT MACOS_VERSION OR NOT MACOSX_VERSION VERSION_LESS 13.00))