From 17c0782fdd413a8fa5a1e54cfc4c760ce991d390 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Mon, 11 May 2026 14:17:26 +0200 Subject: [PATCH 1/3] [io] move (R)AlignedStorage to bit utils --- core/foundation/inc/ROOT/BitUtils.hxx | 7 ++++++ io/io/inc/TEmulatedCollectionProxy.h | 35 ++++++++++++--------------- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/core/foundation/inc/ROOT/BitUtils.hxx b/core/foundation/inc/ROOT/BitUtils.hxx index 738e22b4b3519..c9984e9782340 100644 --- a/core/foundation/inc/ROOT/BitUtils.hxx +++ b/core/foundation/inc/ROOT/BitUtils.hxx @@ -41,6 +41,13 @@ inline constexpr T AlignUp(T value, T align) noexcept return (value + align - 1) & ~(align - 1); } +/// Storage type whose alignment matches \a Align bytes. +/// Used to instantiate std::vector specializations with guaranteed buffer alignment. +template +struct alignas(AlignT) RAlignedStorage { + char data[AlignT] = {}; +}; + /// Given an integer `x`, returns the number of leading 0-bits starting at the most significant bit position. /// If `x` is 0, it returns the size of `x` in bits. /// diff --git a/io/io/inc/TEmulatedCollectionProxy.h b/io/io/inc/TEmulatedCollectionProxy.h index 97ee1ce90685a..1c4e7863fd5e2 100644 --- a/io/io/inc/TEmulatedCollectionProxy.h +++ b/io/io/inc/TEmulatedCollectionProxy.h @@ -23,27 +23,22 @@ class TEmulatedCollectionProxy : public TGenCollectionProxy { friend class TCollectionProxy; public: - /// Storage type whose alignment matches \a Align bytes. - /// Used to instantiate std::vector specializations with guaranteed buffer alignment. - template - struct alignas(Align) AlignedStorage { - char data[Align] = {}; - }; - // Convenience vector aliases for each supported alignment. - using Cont1_t = std::vector>; - using Cont2_t = std::vector>; - using Cont4_t = std::vector>; - using Cont8_t = std::vector>; - using Cont16_t = std::vector>; - using Cont32_t = std::vector>; - using Cont64_t = std::vector>; - using Cont128_t = std::vector>; - using Cont256_t = std::vector>; - using Cont512_t = std::vector>; - using Cont1024_t = std::vector>; - using Cont2048_t = std::vector>; - using Cont4096_t = std::vector>; + // clang-format off + using Cont1_t = std::vector>; + using Cont2_t = std::vector>; + using Cont4_t = std::vector>; + using Cont8_t = std::vector>; + using Cont16_t = std::vector>; + using Cont32_t = std::vector>; + using Cont64_t = std::vector>; + using Cont128_t = std::vector>; + using Cont256_t = std::vector>; + using Cont512_t = std::vector>; + using Cont1024_t = std::vector>; + using Cont2048_t = std::vector>; + using Cont4096_t = std::vector>; + // clang-format on // Canonical container type (used for sizeof/typeid; actual alignment is // selected at runtime via the alignment switch in each method). From 524aab6718176f5d80d92b7e715be06f56cc0a76 Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Mon, 11 May 2026 14:17:32 +0200 Subject: [PATCH 2/3] [ntuple] use TClass alignment info where possible Fixes RNTuple alignment of classes whose alignment is determined by a transient member. --- core/foundation/inc/ROOT/BitUtils.hxx | 6 +++ tree/ntuple/inc/ROOT/RField.hxx | 5 +- .../ROOT/RField/RFieldProxiedCollection.hxx | 4 +- tree/ntuple/inc/ROOT/RField/RFieldSTLMisc.hxx | 16 +++--- .../ROOT/RField/RFieldSequenceContainer.hxx | 12 ++--- tree/ntuple/inc/ROOT/RField/RFieldSoA.hxx | 3 +- tree/ntuple/inc/ROOT/RFieldBase.hxx | 8 +-- tree/ntuple/src/RFieldMeta.cxx | 50 +++++++++++++++---- tree/ntuple/src/RFieldSequenceContainer.cxx | 35 +++++++++++++ tree/ntuple/test/CMakeLists.txt | 2 +- tree/ntuple/test/CustomStruct.hxx | 18 +++++++ tree/ntuple/test/CustomStructLinkDef.h | 5 ++ tree/ntuple/test/rfield_class.cxx | 19 +++++++ 13 files changed, 148 insertions(+), 35 deletions(-) diff --git a/core/foundation/inc/ROOT/BitUtils.hxx b/core/foundation/inc/ROOT/BitUtils.hxx index c9984e9782340..f6946809c3544 100644 --- a/core/foundation/inc/ROOT/BitUtils.hxx +++ b/core/foundation/inc/ROOT/BitUtils.hxx @@ -15,6 +15,7 @@ #include #include #include +#include #include #ifdef _MSC_VER @@ -155,6 +156,11 @@ inline std::size_t TrailingZeroes(T x) #endif // _MSC_VER } + inline bool IsPowerOfTwo(std::uint64_t v) + { + return (v & (v - 1)) == 0; + } + } // namespace Internal } // namespace ROOT diff --git a/tree/ntuple/inc/ROOT/RField.hxx b/tree/ntuple/inc/ROOT/RField.hxx index 0329c436f0654..5cb2e808ad198 100644 --- a/tree/ntuple/inc/ROOT/RField.hxx +++ b/tree/ntuple/inc/ROOT/RField.hxx @@ -165,7 +165,6 @@ private: TClass *fClass; /// Additional information kept for each entry in `fSubfields` std::vector fSubfieldsInfo; - std::size_t fMaxAlignment = 1; /// The staging area stores inputs to I/O rules according to the offsets given by the streamer info of /// "TypeName@@Version". The area is allocated depending on I/O rules resp. the source members of the I/O rules. @@ -220,8 +219,8 @@ public: ~RClassField() override; std::vector SplitValue(const RValue &value) const final; - size_t GetValueSize() const final; - size_t GetAlignment() const final { return fMaxAlignment; } + std::size_t GetValueSize() const final; + std::size_t GetAlignment() const final; std::uint32_t GetTypeVersion() const final; std::uint32_t GetTypeChecksum() const final; /// For polymorphic classes (that declare or inherit at least one virtual method), return the expected dynamic type diff --git a/tree/ntuple/inc/ROOT/RField/RFieldProxiedCollection.hxx b/tree/ntuple/inc/ROOT/RField/RFieldProxiedCollection.hxx index 9ee3724b58b23..4b2637471a474 100644 --- a/tree/ntuple/inc/ROOT/RField/RFieldProxiedCollection.hxx +++ b/tree/ntuple/inc/ROOT/RField/RFieldProxiedCollection.hxx @@ -176,8 +176,8 @@ public: ~RProxiedCollectionField() override = default; std::vector SplitValue(const RValue &value) const final; - size_t GetValueSize() const final { return fProxy->Sizeof(); } - size_t GetAlignment() const final { return alignof(std::max_align_t); } + std::size_t GetValueSize() const final; + std::size_t GetAlignment() const final; void AcceptVisitor(ROOT::Detail::RFieldVisitor &visitor) const final; }; diff --git a/tree/ntuple/inc/ROOT/RField/RFieldSTLMisc.hxx b/tree/ntuple/inc/ROOT/RField/RFieldSTLMisc.hxx index eeeca91d20501..6a59b3a4d5e7a 100644 --- a/tree/ntuple/inc/ROOT/RField/RFieldSTLMisc.hxx +++ b/tree/ntuple/inc/ROOT/RField/RFieldSTLMisc.hxx @@ -264,8 +264,8 @@ public: ~ROptionalField() override = default; std::vector SplitValue(const RValue &value) const final; - size_t GetValueSize() const final; - size_t GetAlignment() const final; + std::size_t GetValueSize() const final; + std::size_t GetAlignment() const final; }; template @@ -313,8 +313,8 @@ public: ~RUniquePtrField() override = default; std::vector SplitValue(const RValue &value) const final; - size_t GetValueSize() const final { return sizeof(std::unique_ptr); } - size_t GetAlignment() const final { return alignof(std::unique_ptr); } + std::size_t GetValueSize() const final { return sizeof(std::unique_ptr); } + std::size_t GetAlignment() const final { return alignof(std::unique_ptr); } }; template @@ -363,8 +363,8 @@ public: RField &operator=(RField &&other) = default; ~RField() final = default; - size_t GetValueSize() const final { return sizeof(std::string); } - size_t GetAlignment() const final { return std::alignment_of(); } + std::size_t GetValueSize() const final { return sizeof(std::string); } + std::size_t GetAlignment() const final { return alignof(std::string); } void AcceptVisitor(ROOT::Detail::RFieldVisitor &visitor) const final; }; @@ -436,8 +436,8 @@ public: RVariantField &operator=(RVariantField &&other) = default; ~RVariantField() override = default; - size_t GetValueSize() const final; - size_t GetAlignment() const final; + std::size_t GetValueSize() const final; + std::size_t GetAlignment() const final; }; template diff --git a/tree/ntuple/inc/ROOT/RField/RFieldSequenceContainer.hxx b/tree/ntuple/inc/ROOT/RField/RFieldSequenceContainer.hxx index e4b152f0ccb49..4bf35f3c6f657 100644 --- a/tree/ntuple/inc/ROOT/RField/RFieldSequenceContainer.hxx +++ b/tree/ntuple/inc/ROOT/RField/RFieldSequenceContainer.hxx @@ -261,8 +261,8 @@ public: CreateUntyped(std::string_view fieldName, std::unique_ptr itemField); std::vector SplitValue(const RValue &value) const final; - size_t GetValueSize() const final { return sizeof(std::vector); } - size_t GetAlignment() const final { return std::alignment_of>(); } + std::size_t GetValueSize() const final; + std::size_t GetAlignment() const final; void AcceptVisitor(ROOT::Detail::RFieldVisitor &visitor) const final; }; @@ -315,8 +315,8 @@ public: std::vector SplitValue(const RValue &value) const final; - size_t GetValueSize() const final { return sizeof(std::vector); } - size_t GetAlignment() const final { return std::alignment_of>(); } + std::size_t GetValueSize() const final { return sizeof(std::vector); } + std::size_t GetAlignment() const final { return alignof(std::vector); } void AcceptVisitor(ROOT::Detail::RFieldVisitor &visitor) const final; }; @@ -410,8 +410,8 @@ public: RArrayAsVectorField &operator=(RArrayAsVectorField &&other) = default; ~RArrayAsVectorField() final = default; - size_t GetValueSize() const final { return sizeof(std::vector); } - size_t GetAlignment() const final { return std::alignment_of>(); } + std::size_t GetValueSize() const final; + std::size_t GetAlignment() const final; std::vector SplitValue(const RFieldBase::RValue &value) const final; void AcceptVisitor(ROOT::Detail::RFieldVisitor &visitor) const final; diff --git a/tree/ntuple/inc/ROOT/RField/RFieldSoA.hxx b/tree/ntuple/inc/ROOT/RField/RFieldSoA.hxx index 5fdc3fc9a4583..97a625eeb3c83 100644 --- a/tree/ntuple/inc/ROOT/RField/RFieldSoA.hxx +++ b/tree/ntuple/inc/ROOT/RField/RFieldSoA.hxx @@ -67,7 +67,6 @@ class RSoAField : public RFieldBase { /// The offset of the RVec members in the SoA type in the order of subfields of the underlying record type. /// In particular, the order is not necessarily the same then the order of RVec members in the SoA class. std::vector fSoAMemberOffsets; - std::size_t fMaxAlignment = 1; ROOT::Internal::RColumnIndex fNWritten; RSoAField(std::string_view fieldName, const RSoAField &source); ///< Used by CloneImpl @@ -98,7 +97,7 @@ public: std::vector SplitValue(const RValue &value) const final; size_t GetValueSize() const final; - size_t GetAlignment() const final { return fMaxAlignment; } + size_t GetAlignment() const final; std::uint32_t GetTypeVersion() const final; std::uint32_t GetTypeChecksum() const final; /// For polymorphic classes (that declare or inherit at least one virtual method), return the expected dynamic type diff --git a/tree/ntuple/inc/ROOT/RFieldBase.hxx b/tree/ntuple/inc/ROOT/RFieldBase.hxx index 2761e391116ae..b62f62c78f13d 100644 --- a/tree/ntuple/inc/ROOT/RFieldBase.hxx +++ b/tree/ntuple/inc/ROOT/RFieldBase.hxx @@ -556,6 +556,9 @@ protected: const ROOT::RNTupleDescriptor *desc, ROOT::DescriptorId_t fieldId); public: + /// Maximum supported alignment for field types, i.e. maximum returned by GetAlignment() + static constexpr std::size_t kMaxAlignment = 4096; + template class RSchemaIteratorTemplate; using RSchemaIterator = RSchemaIteratorTemplate; @@ -624,10 +627,9 @@ public: /// correct `std::variant` or all the elements of a collection. The default implementation assumes no subvalues /// and returns an empty vector. virtual std::vector SplitValue(const RValue &value) const; - /// The number of bytes taken by a value of the appropriate type + /// What sizeof(T) for this type returns virtual size_t GetValueSize() const = 0; - /// As a rule of thumb, the alignment is equal to the size of the type. There are, however, various exceptions - /// to this rule depending on OS and CPU architecture. So enforce the alignment to be explicitly spelled out. + /// What alignof(T) for this type returns virtual size_t GetAlignment() const = 0; std::uint32_t GetTraits() const { return fTraits; } bool HasReadCallbacks() const { return !fReadCallbacks.empty(); } diff --git a/tree/ntuple/src/RFieldMeta.cxx b/tree/ntuple/src/RFieldMeta.cxx index 78d3d1aba7da7..0289bf6a8a741 100644 --- a/tree/ntuple/src/RFieldMeta.cxx +++ b/tree/ntuple/src/RFieldMeta.cxx @@ -15,6 +15,7 @@ // - RField // - RVariantField +#include #include #include #include @@ -117,6 +118,12 @@ TEnum *EnsureValidEnum(std::string_view enumName) return e; } +void EnsureValidAlignment(std::size_t align) +{ + if (align == 0 || align > ROOT::RFieldBase::kMaxAlignment || !ROOT::Internal::IsPowerOfTwo(align)) + throw ROOT::RException(R__FAIL(std::string("invalid alignment: ") + std::to_string(align))); +} + /// Create a comma-separated list of type names from the given fields. Uses either the real type names or the /// type aliases (if there are any, otherwise the actual type name). Used to construct template argument lists /// for templated types such as std::pair<...>, std::tuple<...>, std::variant<...>. @@ -187,8 +194,7 @@ std::string BuildMapTypeName(ROOT::RMapField::EMapType mapType, const ROOT::RFie ROOT::RClassField::RClassField(std::string_view fieldName, const RClassField &source) : ROOT::RFieldBase(fieldName, source.GetTypeName(), ROOT::ENTupleStructure::kRecord, false /* isSimple */), fClass(source.fClass), - fSubfieldsInfo(source.fSubfieldsInfo), - fMaxAlignment(source.fMaxAlignment) + fSubfieldsInfo(source.fSubfieldsInfo) { for (const auto &f : source.GetConstSubfields()) { RFieldBase::Attach(f->Clone(f->GetFieldName())); @@ -281,7 +287,6 @@ ROOT::RClassField::~RClassField() void ROOT::RClassField::Attach(std::unique_ptr child, RSubfieldInfo info) { - fMaxAlignment = std::max(fMaxAlignment, child->GetAlignment()); fSubfieldsInfo.push_back(info); RFieldBase::Attach(std::move(child)); } @@ -623,11 +628,18 @@ std::vector ROOT::RClassField::SplitValue(const RValue return result; } -size_t ROOT::RClassField::GetValueSize() const +std::size_t ROOT::RClassField::GetValueSize() const { return fClass->GetClassSize(); } +std::size_t ROOT::RClassField::GetAlignment() const +{ + const auto align = fClass->GetClassAlignment(); + EnsureValidAlignment(align); + return align; +} + std::uint32_t ROOT::RClassField::GetTypeVersion() const { return fClass->GetClassVersion(); @@ -657,8 +669,7 @@ void ROOT::RClassField::AcceptVisitor(ROOT::Detail::RFieldVisitor &visitor) cons ROOT::Experimental::RSoAField::RSoAField(std::string_view fieldName, const RSoAField &source) : ROOT::RFieldBase(fieldName, source.GetTypeName(), ROOT::ENTupleStructure::kCollection, false /* isSimple */), fSoAClass(source.fSoAClass), - fSoAMemberOffsets(source.fSoAMemberOffsets), - fMaxAlignment(source.fMaxAlignment) + fSoAMemberOffsets(source.fSoAMemberOffsets) { fTraits = source.GetTraits(); Attach(source.fSubfields[0]->Clone(source.fSubfields[0]->GetFieldName())); @@ -757,8 +768,6 @@ ROOT::Experimental::RSoAField::RSoAField(std::string_view fieldName, TClass *clS leftType + " vs. " + rightType + ")")); } - fMaxAlignment = std::max(fMaxAlignment, vecField->GetAlignment()); - assert(itr->second < fSoAMemberOffsets.size()); fSoAMemberOffsets[itr->second] = dataMember->GetOffset(); nMembers++; @@ -863,7 +872,7 @@ std::vector ROOT::Experimental::RSoAField::SplitValue( return std::vector(); } -size_t ROOT::Experimental::RSoAField::GetValueSize() const +std::size_t ROOT::Experimental::RSoAField::GetValueSize() const { return fSoAClass->GetClassSize(); } @@ -878,6 +887,13 @@ std::uint32_t ROOT::Experimental::RSoAField::GetTypeChecksum() const return fSoAClass->GetCheckSum(); } +std::size_t ROOT::Experimental::RSoAField::GetAlignment() const +{ + const auto align = fSoAClass->GetClassAlignment(); + EnsureValidAlignment(align); + return align; +} + const std::type_info *ROOT::Experimental::RSoAField::GetPolymorphicTypeInfo() const { // TODO(jblomer): factor out @@ -1198,6 +1214,18 @@ std::vector ROOT::RProxiedCollectionField::SplitValue( return result; } +std::size_t ROOT::RProxiedCollectionField::GetValueSize() const +{ + return fProxy->Sizeof(); +} + +std::size_t ROOT::RProxiedCollectionField::GetAlignment() const +{ + const auto align = fProxy->GetCollectionClass()->GetClassAlignment(); + EnsureValidAlignment(align); + return align; +} + void ROOT::RProxiedCollectionField::AcceptVisitor(ROOT::Detail::RFieldVisitor &visitor) const { visitor.VisitProxiedCollectionField(*this); @@ -1405,7 +1433,9 @@ ROOT::RExtraTypeInfoDescriptor ROOT::RStreamerField::GetExtraTypeInfo() const std::size_t ROOT::RStreamerField::GetAlignment() const { - return std::min(alignof(std::max_align_t), GetValueSize()); // TODO(jblomer): fix me + const auto align = fClass->GetClassAlignment(); + EnsureValidAlignment(align); + return align; } std::size_t ROOT::RStreamerField::GetValueSize() const diff --git a/tree/ntuple/src/RFieldSequenceContainer.cxx b/tree/ntuple/src/RFieldSequenceContainer.cxx index 9c96ee2990d03..0fe8a22fd2396 100644 --- a/tree/ntuple/src/RFieldSequenceContainer.cxx +++ b/tree/ntuple/src/RFieldSequenceContainer.cxx @@ -3,6 +3,7 @@ /// \author Jonas Hahnfeld /// \date 2024-11-19 +#include #include #include #include @@ -30,6 +31,20 @@ std::vector SplitVector(std::shared_ptr valuePtr return result; } +std::size_t GetSizeOfVector() +{ + static_assert(sizeof(std::vector) == + sizeof(std::vector>)); + return sizeof(std::vector); +} + +std::size_t GetAlignOfVector() +{ + static_assert(alignof(std::vector) == + alignof(std::vector>)); + return alignof(std::vector); +} + } // anonymous namespace ROOT::RArrayField::RArrayField(std::string_view fieldName, std::unique_ptr itemField, @@ -648,6 +663,16 @@ std::vector ROOT::RVectorField::SplitValue(const RValu return SplitVector(value.GetPtr(), *fSubfields[0]); } +std::size_t ROOT::RVectorField::GetValueSize() const +{ + return GetSizeOfVector(); +} + +std::size_t ROOT::RVectorField::GetAlignment() const +{ + return GetAlignOfVector(); +} + void ROOT::RVectorField::AcceptVisitor(ROOT::Detail::RFieldVisitor &visitor) const { visitor.VisitVectorField(*this); @@ -974,6 +999,16 @@ std::vector ROOT::RArrayAsVectorField::SplitValue(cons return SplitVector(value.GetPtr(), *fSubfields[0]); } +std::size_t ROOT::RArrayAsVectorField::GetValueSize() const +{ + return GetSizeOfVector(); +} + +std::size_t ROOT::RArrayAsVectorField::GetAlignment() const +{ + return GetAlignOfVector(); +} + void ROOT::RArrayAsVectorField::AcceptVisitor(ROOT::Detail::RFieldVisitor &visitor) const { visitor.VisitArrayAsVectorField(*this); diff --git a/tree/ntuple/test/CMakeLists.txt b/tree/ntuple/test/CMakeLists.txt index 34e8b373149be..7bd5824ddfd7d 100644 --- a/tree/ntuple/test/CMakeLists.txt +++ b/tree/ntuple/test/CMakeLists.txt @@ -11,7 +11,7 @@ ROOT_STANDARD_LIBRARY_PACKAGE(CustomStruct HEADERS ${CMAKE_CURRENT_SOURCE_DIR}/CustomStruct.hxx SOURCES CustomStruct.cxx LINKDEF CustomStructLinkDef.h - DEPENDENCIES RIO) + DEPENDENCIES RIO ROOTVecOps) configure_file(CustomStruct.hxx . COPYONLY) if(MSVC AND NOT CMAKE_GENERATOR MATCHES Ninja) add_custom_command(TARGET CustomStruct POST_BUILD diff --git a/tree/ntuple/test/CustomStruct.hxx b/tree/ntuple/test/CustomStruct.hxx index c3d6e633f356b..03f54d46113ee 100644 --- a/tree/ntuple/test/CustomStruct.hxx +++ b/tree/ntuple/test/CustomStruct.hxx @@ -1,6 +1,8 @@ #ifndef ROOT_RNTuple_Test_CustomStruct #define ROOT_RNTuple_Test_CustomStruct +#include + #include // for Double32_t #include #include @@ -478,4 +480,20 @@ struct MemberWithCustomStreamer { ClassDefNV(MemberWithCustomStreamer, 2); }; +struct AlignmentDeterminedByTransientMember { + short int fA; + float fTransient; /// fVec; + std::array fArr; +}; + #endif diff --git a/tree/ntuple/test/CustomStructLinkDef.h b/tree/ntuple/test/CustomStructLinkDef.h index 51aadb41c34c2..4786ce21e2860 100644 --- a/tree/ntuple/test/CustomStructLinkDef.h +++ b/tree/ntuple/test/CustomStructLinkDef.h @@ -181,4 +181,9 @@ #pragma link C++ class MemberWithCustomStreamer+; +#pragma link C++ class AlignmentDeterminedByTransientMember+; +#pragma link C++ class AlignedAs+; +#pragma link C++ class OverAligned+; +#pragma link C++ class AlignmentEnvelope+; + #endif diff --git a/tree/ntuple/test/rfield_class.cxx b/tree/ntuple/test/rfield_class.cxx index 326a6140f394e..598a97b484825 100644 --- a/tree/ntuple/test/rfield_class.cxx +++ b/tree/ntuple/test/rfield_class.cxx @@ -502,3 +502,22 @@ TEST(RNTuple, MemberWithCustomStreamer) // After setting member streamer: field creation should throw EXPECT_THROW(RFieldBase::Create("f", "MemberWithCustomStreamer").Unwrap(), ROOT::RException); } + +TEST(RNTuple, AlignmentCornerCases) +{ + // Alignment determined by transient member + EXPECT_EQ(alignof(AlignmentDeterminedByTransientMember), + RFieldBase::Create("", "AlignmentDeterminedByTransientMember").Unwrap()->GetAlignment()); + + // Respect custom alignment + EXPECT_EQ(alignof(AlignedAs), RFieldBase::Create("", "AlignedAs").Unwrap()->GetAlignment()); + EXPECT_EQ(sizeof(AlignedAs), RFieldBase::Create("", "AlignedAs").Unwrap()->GetValueSize()); + EXPECT_EQ(8u, RFieldBase::Create("", "AlignedAs").Unwrap()->GetValueSize()); + + // Handling of over-aligned types + auto f = RFieldBase::Create("", "AlignmentEnvelope").Unwrap(); + EXPECT_GT(alignof(AlignmentEnvelope), sizeof(std::max_align_t)); + EXPECT_EQ(alignof(AlignmentEnvelope), f->GetAlignment()); + EXPECT_EQ(alignof(ROOT::RVec), + RFieldBase::Create("", "ROOT::RVec").Unwrap()->GetAlignment()); +} From ba94832888d0d60bad05d00ac93fe3ddb552501b Mon Sep 17 00:00:00 2001 From: Jakob Blomer Date: Fri, 15 May 2026 17:25:54 +0200 Subject: [PATCH 3/3] [ntuple] fix memory allocation for over-aligned fields --- tree/ntuple/inc/ROOT/RField.hxx | 4 +-- .../ROOT/RField/RFieldProxiedCollection.hxx | 8 ++--- tree/ntuple/inc/ROOT/RField/RFieldRecord.hxx | 5 +-- tree/ntuple/inc/ROOT/RField/RFieldSTLMisc.hxx | 18 +++++++--- .../ROOT/RField/RFieldSequenceContainer.hxx | 22 +++++++----- tree/ntuple/inc/ROOT/RField/RFieldSoA.hxx | 2 +- tree/ntuple/inc/ROOT/RFieldBase.hxx | 17 ++++++--- tree/ntuple/src/RField.cxx | 4 +-- tree/ntuple/src/RFieldBase.cxx | 13 +++++-- tree/ntuple/src/RFieldMeta.cxx | 35 +++++++++++++++---- tree/ntuple/src/RFieldSequenceContainer.cxx | 11 ++++-- tree/ntuple/test/ntuple_compat.cxx | 4 +-- tree/ntuple/test/rfield_class.cxx | 15 ++++++++ 13 files changed, 112 insertions(+), 46 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RField.hxx b/tree/ntuple/inc/ROOT/RField.hxx index 5cb2e808ad198..75929d03c4cb6 100644 --- a/tree/ntuple/inc/ROOT/RField.hxx +++ b/tree/ntuple/inc/ROOT/RField.hxx @@ -158,7 +158,7 @@ private: TClass *fClass; public: - explicit RClassDeleter(TClass *cl) : fClass(cl) {} + explicit RClassDeleter(TClass *cl); void operator()(void *objPtr, bool dtorOnly) final; }; @@ -239,7 +239,7 @@ private: TClass *fClass; public: - explicit RStreamerFieldDeleter(TClass *cl) : fClass(cl) {} + explicit RStreamerFieldDeleter(TClass *cl); void operator()(void *objPtr, bool dtorOnly) final; }; diff --git a/tree/ntuple/inc/ROOT/RField/RFieldProxiedCollection.hxx b/tree/ntuple/inc/ROOT/RField/RFieldProxiedCollection.hxx index 4b2637471a474..5a44fd6f8308c 100644 --- a/tree/ntuple/inc/ROOT/RField/RFieldProxiedCollection.hxx +++ b/tree/ntuple/inc/ROOT/RField/RFieldProxiedCollection.hxx @@ -128,13 +128,9 @@ protected: RCollectionIterableOnce::RIteratorFuncs fIFuncsWrite; public: - explicit RProxiedCollectionDeleter(std::shared_ptr proxy) : fProxy(proxy) {} + explicit RProxiedCollectionDeleter(std::shared_ptr proxy); RProxiedCollectionDeleter(std::shared_ptr proxy, std::unique_ptr itemDeleter, - size_t itemSize) - : fProxy(proxy), fItemDeleter(std::move(itemDeleter)), fItemSize(itemSize) - { - fIFuncsWrite = RCollectionIterableOnce::GetIteratorFuncs(fProxy.get(), false /* readFromDisk */); - } + size_t itemSize); void operator()(void *objPtr, bool dtorOnly) final; }; diff --git a/tree/ntuple/inc/ROOT/RField/RFieldRecord.hxx b/tree/ntuple/inc/ROOT/RField/RFieldRecord.hxx index 873da2f3e5225..99f5928faf27e 100644 --- a/tree/ntuple/inc/ROOT/RField/RFieldRecord.hxx +++ b/tree/ntuple/inc/ROOT/RField/RFieldRecord.hxx @@ -59,8 +59,9 @@ class RRecordField : public RFieldBase { std::vector fOffsets; public: - RRecordDeleter(std::vector> itemDeleters, const std::vector &offsets) - : fItemDeleters(std::move(itemDeleters)), fOffsets(offsets) + RRecordDeleter(std::vector> itemDeleters, const std::vector &offsets, + std::size_t alignment) + : RDeleter(alignment), fItemDeleters(std::move(itemDeleters)), fOffsets(offsets) { } void operator()(void *objPtr, bool dtorOnly) final; diff --git a/tree/ntuple/inc/ROOT/RField/RFieldSTLMisc.hxx b/tree/ntuple/inc/ROOT/RField/RFieldSTLMisc.hxx index 6a59b3a4d5e7a..1b23e9382ae8e 100644 --- a/tree/ntuple/inc/ROOT/RField/RFieldSTLMisc.hxx +++ b/tree/ntuple/inc/ROOT/RField/RFieldSTLMisc.hxx @@ -233,8 +233,10 @@ class ROptionalField : public RNullableField { std::size_t fEngagementPtrOffset = 0; public: - ROptionalDeleter(std::unique_ptr itemDeleter, std::size_t engagementPtrOffset) - : fItemDeleter(std::move(itemDeleter)), fEngagementPtrOffset(engagementPtrOffset) {} + ROptionalDeleter(std::unique_ptr itemDeleter, std::size_t engagementPtrOffset, std::size_t alignment) + : RDeleter(alignment), fItemDeleter(std::move(itemDeleter)), fEngagementPtrOffset(engagementPtrOffset) + { + } void operator()(void *objPtr, bool dtorOnly) final; }; @@ -284,7 +286,10 @@ class RUniquePtrField : public RNullableField { std::unique_ptr fItemDeleter; public: - explicit RUniquePtrDeleter(std::unique_ptr itemDeleter) : fItemDeleter(std::move(itemDeleter)) {} + explicit RUniquePtrDeleter(std::unique_ptr itemDeleter) + : RDeleter(alignof(std::unique_ptr)), fItemDeleter(std::move(itemDeleter)) + { + } void operator()(void *objPtr, bool dtorOnly) final; }; @@ -388,9 +393,12 @@ private: std::vector> fItemDeleters; public: - RVariantDeleter(std::size_t tagOffset, std::size_t variantOffset, + RVariantDeleter(std::size_t tagOffset, std::size_t variantOffset, std::size_t alignment, std::vector> itemDeleters) - : fTagOffset(tagOffset), fVariantOffset(variantOffset), fItemDeleters(std::move(itemDeleters)) + : RDeleter(alignment), + fTagOffset(tagOffset), + fVariantOffset(variantOffset), + fItemDeleters(std::move(itemDeleters)) { } void operator()(void *objPtr, bool dtorOnly) final; diff --git a/tree/ntuple/inc/ROOT/RField/RFieldSequenceContainer.hxx b/tree/ntuple/inc/ROOT/RField/RFieldSequenceContainer.hxx index 4bf35f3c6f657..0c9bb09e8e14a 100644 --- a/tree/ntuple/inc/ROOT/RField/RFieldSequenceContainer.hxx +++ b/tree/ntuple/inc/ROOT/RField/RFieldSequenceContainer.hxx @@ -51,8 +51,9 @@ private: std::unique_ptr fItemDeleter; public: - RArrayDeleter(std::size_t itemSize, std::size_t arrayLength, std::unique_ptr itemDeleter) - : fItemSize(itemSize), fArrayLength(arrayLength), fItemDeleter(std::move(itemDeleter)) + RArrayDeleter(std::size_t itemSize, std::size_t arrayLength, std::size_t alignment, + std::unique_ptr itemDeleter) + : RDeleter(alignment), fItemSize(itemSize), fArrayLength(arrayLength), fItemDeleter(std::move(itemDeleter)) { } void operator()(void *objPtr, bool dtorOnly) final; @@ -126,9 +127,15 @@ class RRVecField : public RFieldBase { std::unique_ptr fItemDeleter; public: - explicit RRVecDeleter(std::size_t itemAlignment) : fItemAlignment(itemAlignment) {} + explicit RRVecDeleter(std::size_t itemAlignment) + : RDeleter(ROOT::Internal::EvalRVecAlignment(itemAlignment)), fItemAlignment(itemAlignment) + { + } RRVecDeleter(std::size_t itemAlignment, std::size_t itemSize, std::unique_ptr itemDeleter) - : fItemAlignment(itemAlignment), fItemSize(itemSize), fItemDeleter(std::move(itemDeleter)) + : RDeleter(ROOT::Internal::EvalRVecAlignment(itemAlignment)), + fItemAlignment(itemAlignment), + fItemSize(itemSize), + fItemDeleter(std::move(itemDeleter)) { } void operator()(void *objPtr, bool dtorOnly) final; @@ -211,11 +218,8 @@ class RVectorField : public RFieldBase { std::unique_ptr fItemDeleter; public: - RVectorDeleter() = default; - RVectorDeleter(std::size_t itemSize, std::unique_ptr itemDeleter) - : fItemSize(itemSize), fItemDeleter(std::move(itemDeleter)) - { - } + RVectorDeleter(); + RVectorDeleter(std::size_t itemSize, std::unique_ptr itemDeleter); void operator()(void *objPtr, bool dtorOnly) final; }; diff --git a/tree/ntuple/inc/ROOT/RField/RFieldSoA.hxx b/tree/ntuple/inc/ROOT/RField/RFieldSoA.hxx index 97a625eeb3c83..c44c582cb17dc 100644 --- a/tree/ntuple/inc/ROOT/RField/RFieldSoA.hxx +++ b/tree/ntuple/inc/ROOT/RField/RFieldSoA.hxx @@ -58,7 +58,7 @@ class RSoAField : public RFieldBase { TClass *fSoAClass; public: - explicit RSoADeleter(TClass *cl) : fSoAClass(cl) {} + explicit RSoADeleter(TClass *cl); void operator()(void *objPtr, bool dtorOnly) final; }; diff --git a/tree/ntuple/inc/ROOT/RFieldBase.hxx b/tree/ntuple/inc/ROOT/RFieldBase.hxx index b62f62c78f13d..40cf79851ce19 100644 --- a/tree/ntuple/inc/ROOT/RFieldBase.hxx +++ b/tree/ntuple/inc/ROOT/RFieldBase.hxx @@ -102,18 +102,25 @@ class RFieldBase { using ReadCallback_t = std::function; +public: + /// Maximum supported alignment for field types, i.e. maximum returned by GetAlignment() + static constexpr std::size_t kMaxAlignment = 4096; + protected: /// A functor to release the memory acquired by CreateValue() (memory and constructor). /// This implementation works for types with a trivial destructor. More complex fields implement a derived deleter. /// The deleter is operational without the field object and thus can be used to destruct/release a value after /// the field has been destructed. class RDeleter { + std::size_t fAlignment; + public: + explicit RDeleter(std::size_t alignment) : fAlignment(alignment) { EnsureValidAlignment(alignment); } virtual ~RDeleter() = default; virtual void operator()(void *objPtr, bool dtorOnly) { if (!dtorOnly) - operator delete(objPtr); + operator delete(objPtr, fAlignment); } }; @@ -121,6 +128,7 @@ protected: template class RTypedDeleter : public RDeleter { public: + RTypedDeleter() : RDeleter(alignof(T)) {} void operator()(void *objPtr, bool dtorOnly) final { std::destroy_at(static_cast(objPtr)); @@ -422,10 +430,12 @@ protected: /// Constructs value in a given location of size at least GetValueSize(). Called by the base class' CreateValue(). virtual void ConstructValue(void *where) const = 0; - virtual std::unique_ptr GetDeleter() const { return std::make_unique(); } + virtual std::unique_ptr GetDeleter() const { return std::make_unique(GetAlignment()); } /// Allow derived classes to call ConstructValue(void *) and GetDeleter() on other (sub)fields. static void CallConstructValueOn(const RFieldBase &other, void *where) { other.ConstructValue(where); } static std::unique_ptr GetDeleterOf(const RFieldBase &other) { return other.GetDeleter(); } + /// Throws if alignment is not a power of two between 1 and kMaxAlignment. + static void EnsureValidAlignment(std::size_t alignment); /// Allow parents to mark their childs as artificial fields (used in class and record fields) static void CallSetArtificialOn(RFieldBase &other) { other.SetArtificial(); } @@ -556,9 +566,6 @@ protected: const ROOT::RNTupleDescriptor *desc, ROOT::DescriptorId_t fieldId); public: - /// Maximum supported alignment for field types, i.e. maximum returned by GetAlignment() - static constexpr std::size_t kMaxAlignment = 4096; - template class RSchemaIteratorTemplate; using RSchemaIterator = RSchemaIteratorTemplate; diff --git a/tree/ntuple/src/RField.cxx b/tree/ntuple/src/RField.cxx index dc41bd91023de..b457dd6347faf 100644 --- a/tree/ntuple/src/RField.cxx +++ b/tree/ntuple/src/RField.cxx @@ -731,7 +731,7 @@ std::unique_ptr ROOT::RRecordField::GetDeleter() con for (const auto &f : fSubfields) { itemDeleters.emplace_back(GetDeleterOf(*f)); } - return std::make_unique(std::move(itemDeleters), fOffsets); + return std::make_unique(std::move(itemDeleters), fOffsets, GetAlignment()); } std::vector ROOT::RRecordField::SplitValue(const RValue &value) const @@ -1169,7 +1169,7 @@ std::unique_ptr ROOT::ROptionalField::GetDeleter() c { return std::make_unique( (fSubfields[0]->GetTraits() & kTraitTriviallyDestructible) ? nullptr : GetDeleterOf(*fSubfields[0]), - fSubfields[0]->GetValueSize()); + fSubfields[0]->GetValueSize(), GetAlignment()); } std::vector ROOT::ROptionalField::SplitValue(const RValue &value) const diff --git a/tree/ntuple/src/RFieldBase.cxx b/tree/ntuple/src/RFieldBase.cxx index 0fd55c0183a67..bb9c13ef2542c 100644 --- a/tree/ntuple/src/RFieldBase.cxx +++ b/tree/ntuple/src/RFieldBase.cxx @@ -3,6 +3,7 @@ /// \author Jonas Hahnfeld /// \date 2024-11-19 +#include #include #include #include @@ -168,7 +169,7 @@ void ROOT::RFieldBase::RBulkValues::ReleaseValues() } } - operator delete(fValues); + operator delete(fValues, fField->GetAlignment()); } void ROOT::RFieldBase::RBulkValues::Reset(RNTupleLocalIndex firstIndex, std::size_t size) @@ -178,7 +179,7 @@ void ROOT::RFieldBase::RBulkValues::Reset(RNTupleLocalIndex firstIndex, std::siz throw RException(R__FAIL("invalid attempt to bulk read beyond the adopted buffer")); } ReleaseValues(); - fValues = operator new(size * fValueSize); + fValues = operator new(size * fValueSize, std::align_val_t(fField->GetAlignment())); if (!(fField->GetTraits() & RFieldBase::kTraitTriviallyConstructible)) { for (std::size_t i = 0; i < size; ++i) { @@ -633,7 +634,7 @@ std::size_t ROOT::RFieldBase::ReadBulkImpl(const RBulkSpec &bulkSpec) void *ROOT::RFieldBase::CreateObjectRawPtr() const { - void *where = operator new(GetValueSize()); + void *where = operator new(GetValueSize(), std::align_val_t(GetAlignment())); R__ASSERT(where != nullptr); ConstructValue(where); return where; @@ -645,6 +646,12 @@ ROOT::RFieldBase::RValue ROOT::RFieldBase::CreateValue() return RValue(this, std::shared_ptr(obj, RSharedPtrDeleter(GetDeleter()))); } +void ROOT::RFieldBase::EnsureValidAlignment(std::size_t alignment) +{ + if (alignment == 0 || alignment > ROOT::RFieldBase::kMaxAlignment || !ROOT::Internal::IsPowerOfTwo(alignment)) + throw ROOT::RException(R__FAIL(std::string("invalid alignment: ") + std::to_string(alignment))); +} + std::vector ROOT::RFieldBase::SplitValue(const RValue & /*value*/) const { return std::vector(); diff --git a/tree/ntuple/src/RFieldMeta.cxx b/tree/ntuple/src/RFieldMeta.cxx index 0289bf6a8a741..382bc61828a5f 100644 --- a/tree/ntuple/src/RFieldMeta.cxx +++ b/tree/ntuple/src/RFieldMeta.cxx @@ -118,12 +118,6 @@ TEnum *EnsureValidEnum(std::string_view enumName) return e; } -void EnsureValidAlignment(std::size_t align) -{ - if (align == 0 || align > ROOT::RFieldBase::kMaxAlignment || !ROOT::Internal::IsPowerOfTwo(align)) - throw ROOT::RException(R__FAIL(std::string("invalid alignment: ") + std::to_string(align))); -} - /// Create a comma-separated list of type names from the given fields. Uses either the real type names or the /// type aliases (if there are any, otherwise the actual type name). Used to construct template argument lists /// for templated types such as std::pair<...>, std::tuple<...>, std::variant<...>. @@ -609,6 +603,8 @@ void ROOT::RClassField::ConstructValue(void *where) const fClass->New(where); } +ROOT::RClassField::RClassDeleter::RClassDeleter(TClass *cl) : RDeleter(cl->GetClassAlignment()), fClass(cl) {} + void ROOT::RClassField::RClassDeleter::operator()(void *objPtr, bool dtorOnly) { fClass->Destructor(objPtr, true /* dtorOnly */); @@ -860,6 +856,10 @@ void ROOT::Experimental::RSoAField::ConstructValue(void *where) const fSoAClass->New(where); } +ROOT::Experimental::RSoAField::RSoADeleter::RSoADeleter(TClass *cl) : RDeleter(cl->GetClassAlignment()), fSoAClass(cl) +{ +} + void ROOT::Experimental::RSoAField::RSoADeleter::operator()(void *objPtr, bool dtorOnly) { fSoAClass->Destructor(objPtr, true /* dtorOnly */); @@ -1190,6 +1190,22 @@ std::unique_ptr ROOT::RProxiedCollectionField::GetDe return std::make_unique(fProxy); } +ROOT::RProxiedCollectionField::RProxiedCollectionDeleter::RProxiedCollectionDeleter( + std::shared_ptr proxy) + : RDeleter(proxy->GetCollectionClass()->GetClassAlignment()), fProxy(proxy) +{ +} + +ROOT::RProxiedCollectionField::RProxiedCollectionDeleter::RProxiedCollectionDeleter( + std::shared_ptr proxy, std::unique_ptr itemDeleter, size_t itemSize) + : RDeleter(proxy->GetCollectionClass()->GetClassAlignment()), + fProxy(proxy), + fItemDeleter(std::move(itemDeleter)), + fItemSize(itemSize) +{ + fIFuncsWrite = RCollectionIterableOnce::GetIteratorFuncs(fProxy.get(), false /* readFromDisk */); +} + void ROOT::RProxiedCollectionField::RProxiedCollectionDeleter::operator()(void *objPtr, bool dtorOnly) { if (fItemDeleter) { @@ -1415,6 +1431,11 @@ void ROOT::RStreamerField::ConstructValue(void *where) const fClass->New(where); } +ROOT::RStreamerField::RStreamerFieldDeleter::RStreamerFieldDeleter(TClass *cl) + : RDeleter(cl->GetClassAlignment()), fClass(cl) +{ +} + void ROOT::RStreamerField::RStreamerFieldDeleter::operator()(void *objPtr, bool dtorOnly) { fClass->Destructor(objPtr, true /* dtorOnly */); @@ -1816,7 +1837,7 @@ std::unique_ptr ROOT::RVariantField::GetDeleter() co for (const auto &f : fSubfields) { itemDeleters.emplace_back(GetDeleterOf(*f)); } - return std::make_unique(fTagOffset, fVariantOffset, std::move(itemDeleters)); + return std::make_unique(fTagOffset, fVariantOffset, GetAlignment(), std::move(itemDeleters)); } size_t ROOT::RVariantField::GetAlignment() const diff --git a/tree/ntuple/src/RFieldSequenceContainer.cxx b/tree/ntuple/src/RFieldSequenceContainer.cxx index 0fe8a22fd2396..6fc78dadc5394 100644 --- a/tree/ntuple/src/RFieldSequenceContainer.cxx +++ b/tree/ntuple/src/RFieldSequenceContainer.cxx @@ -151,8 +151,8 @@ void ROOT::RArrayField::RArrayDeleter::operator()(void *objPtr, bool dtorOnly) std::unique_ptr ROOT::RArrayField::GetDeleter() const { if (!(fSubfields[0]->GetTraits() & kTraitTriviallyDestructible)) - return std::make_unique(fItemSize, fArrayLength, GetDeleterOf(*fSubfields[0])); - return std::make_unique(); + return std::make_unique(fItemSize, fArrayLength, GetAlignment(), GetDeleterOf(*fSubfields[0])); + return std::make_unique(GetAlignment()); } std::vector ROOT::RArrayField::SplitValue(const RValue &value) const @@ -636,6 +636,13 @@ void ROOT::RVectorField::ReconcileOnDiskField(const RNTupleDescriptor &desc) EnsureMatchingOnDiskCollection(desc).ThrowOnError(); } +ROOT::RVectorField::RVectorDeleter::RVectorDeleter() : RDeleter(GetAlignOfVector()) {} + +ROOT::RVectorField::RVectorDeleter::RVectorDeleter(std::size_t itemSize, std::unique_ptr itemDeleter) + : RDeleter(GetAlignOfVector()), fItemSize(itemSize), fItemDeleter(std::move(itemDeleter)) +{ +} + void ROOT::RVectorField::RVectorDeleter::operator()(void *objPtr, bool dtorOnly) { auto vecPtr = static_cast *>(objPtr); diff --git a/tree/ntuple/test/ntuple_compat.cxx b/tree/ntuple/test/ntuple_compat.cxx index 6b2cd0a6a774e..8b4b7c47d17c8 100644 --- a/tree/ntuple/test/ntuple_compat.cxx +++ b/tree/ntuple/test/ntuple_compat.cxx @@ -452,8 +452,8 @@ class RFutureField : public RFieldBase { public: RFutureField(std::string_view name) : RFieldBase(name, "Future", ROOT::Internal::kTestFutureFieldStructure, false) {} - std::size_t GetValueSize() const final { return 0; } - std::size_t GetAlignment() const final { return 0; } + std::size_t GetValueSize() const final { return 1; } + std::size_t GetAlignment() const final { return 1; } }; TEST(RNTupleCompat, FutureFieldStructuralRole) diff --git a/tree/ntuple/test/rfield_class.cxx b/tree/ntuple/test/rfield_class.cxx index 598a97b484825..4a0516d4fa229 100644 --- a/tree/ntuple/test/rfield_class.cxx +++ b/tree/ntuple/test/rfield_class.cxx @@ -520,4 +520,19 @@ TEST(RNTuple, AlignmentCornerCases) EXPECT_EQ(alignof(AlignmentEnvelope), f->GetAlignment()); EXPECT_EQ(alignof(ROOT::RVec), RFieldBase::Create("", "ROOT::RVec").Unwrap()->GetAlignment()); + + std::unique_ptr ptr(f->CreateObject().release()); + EXPECT_EQ(0, reinterpret_cast(ptr.get()) % alignof(AlignmentEnvelope)); + + FileRaii fileGuard("test_ntuple_alignment_corner_cases.root"); + { + auto model = ROOT::RNTupleModel::Create(); + model->MakeField("f"); + auto writer = ROOT::RNTupleWriter::Recreate(std::move(model), "ntpl", fileGuard.GetPath()); + writer->Fill(); + } + auto reader = ROOT::RNTupleReader::Open("ntpl", fileGuard.GetPath()); + auto bulk = reader->GetModel().CreateBulk("f"); + auto bulkPtr = bulk.ReadBulk(ROOT::RNTupleLocalRange(0, 0, 1)); + EXPECT_EQ(0, reinterpret_cast(bulkPtr) % alignof(AlignmentEnvelope)); }