From ac6de07e5ca18656139b6dca3d53d82dda464e04 Mon Sep 17 00:00:00 2001 From: Jonas Hahnfeld Date: Mon, 28 Oct 2024 11:28:42 +0100 Subject: [PATCH 1/5] [ntuple] Remove dangerous RField constructors For typed RField> and RField>, it is dangerous to accept user-created item fields because there is no check that the types match. --- tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx b/tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx index a55a2347ff21b..e3080cf751a30 100644 --- a/tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx +++ b/tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx @@ -147,13 +147,11 @@ private: public: static std::string TypeName() { return "std::pair<" + RField::TypeName() + "," + RField::TypeName() + ">"; } - explicit RField(std::string_view name, std::array, 2> &&itemFields) - : RPairField(name, std::move(itemFields), BuildItemOffsets()) + explicit RField(std::string_view name) : RPairField(name, BuildItemFields(), BuildItemOffsets()) { fMaxAlignment = std::max(alignof(T1), alignof(T2)); fSize = sizeof(ContainerT); } - explicit RField(std::string_view name) : RField(name, BuildItemFields()) {} RField(RField &&other) = default; RField &operator=(RField &&other) = default; ~RField() final = default; @@ -227,13 +225,12 @@ private: public: static std::string TypeName() { return "std::tuple<" + BuildItemTypes() + ">"; } - explicit RField(std::string_view name, std::vector> &&itemFields) - : RTupleField(name, std::move(itemFields), BuildItemOffsets()) + explicit RField(std::string_view name) + : RTupleField(name, BuildItemFields(), BuildItemOffsets()) { fMaxAlignment = std::max({alignof(ItemTs)...}); fSize = sizeof(ContainerT); } - explicit RField(std::string_view name) : RField(name, BuildItemFields()) {} RField(RField &&other) = default; RField &operator=(RField &&other) = default; ~RField() final = default; From 8a4278ab30127d78b5815e17819210ffaaeb8519 Mon Sep 17 00:00:00 2001 From: Jonas Hahnfeld Date: Mon, 28 Oct 2024 12:42:36 +0100 Subject: [PATCH 2/5] [ntuple] Avoid UB in RRecordField construction In C++, the order of evaluation of function arguments is unspecified. This could lead to problems in the constructors of RPairField and RTupleField where itemFields was both std::move'd and passed to the respective GetTypeList. (In practice, the base class constructor took itemFields as xvalue so it was only passing a pointer.) This cannot be solved when callign a base class constructor, so split attaching the item fields into a separate method. --- .../v7/inc/ROOT/RField/RFieldRecord.hxx | 10 ++--- tree/ntuple/v7/src/RField.cxx | 37 +++++++++++-------- tree/ntuple/v7/test/ntuple_descriptor.cxx | 3 +- 3 files changed, 28 insertions(+), 22 deletions(-) diff --git a/tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx b/tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx index e3080cf751a30..549503033364c 100644 --- a/tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx +++ b/tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx @@ -71,17 +71,15 @@ protected: void ReadGlobalImpl(NTupleSize_t globalIndex, void *to) final; void ReadInClusterImpl(RClusterIndex clusterIndex, void *to) final; - RRecordField(std::string_view fieldName, std::vector> &&itemFields, - const std::vector &offsets, std::string_view typeName = ""); + RRecordField(std::string_view fieldName, std::string_view typeName); + + void AttachItemFields(std::vector> &&itemFields); template - RRecordField(std::string_view fieldName, std::array, N> &&itemFields, - const std::array &offsets, std::string_view typeName = "") - : ROOT::Experimental::RFieldBase(fieldName, typeName, ENTupleStructure::kRecord, false /* isSimple */) + void AttachItemFields(std::array, N> &&itemFields) { fTraits |= kTraitTrivialType; for (unsigned i = 0; i < N; ++i) { - fOffsets.push_back(offsets[i]); fMaxAlignment = std::max(fMaxAlignment, itemFields[i]->GetAlignment()); fSize += GetItemPadding(fSize, itemFields[i]->GetAlignment()) + itemFields[i]->GetValueSize(); fTraits &= itemFields[i]->GetTraits(); diff --git a/tree/ntuple/v7/src/RField.cxx b/tree/ntuple/v7/src/RField.cxx index 5843d11a1dc45..2b0c0fd23cbc5 100644 --- a/tree/ntuple/v7/src/RField.cxx +++ b/tree/ntuple/v7/src/RField.cxx @@ -2507,11 +2507,13 @@ ROOT::Experimental::RRecordField::RRecordField(std::string_view name, const RRec fTraits = source.fTraits; } -ROOT::Experimental::RRecordField::RRecordField(std::string_view fieldName, - std::vector> &&itemFields, - const std::vector &offsets, std::string_view typeName) - : ROOT::Experimental::RFieldBase(fieldName, typeName, ENTupleStructure::kRecord, false /* isSimple */), - fOffsets(offsets) +ROOT::Experimental::RRecordField::RRecordField(std::string_view fieldName, std::string_view typeName) + : ROOT::Experimental::RFieldBase(fieldName, typeName, ENTupleStructure::kRecord, false /* isSimple */) +{ +} + +void ROOT::Experimental::RRecordField::RRecordField::AttachItemFields( + std::vector> &&itemFields) { fTraits |= kTraitTrivialType; for (auto &item : itemFields) { @@ -3870,16 +3872,19 @@ ROOT::Experimental::RPairField::RPairField::GetTypeList(const std::array, 2> &&itemFields, const std::array &offsets) - : ROOT::Experimental::RRecordField(fieldName, std::move(itemFields), offsets, - "std::pair<" + GetTypeList(itemFields) + ">") + : ROOT::Experimental::RRecordField(fieldName, "std::pair<" + GetTypeList(itemFields) + ">") { + AttachItemFields(std::move(itemFields)); + fOffsets.push_back(offsets[0]); + fOffsets.push_back(offsets[1]); } ROOT::Experimental::RPairField::RPairField(std::string_view fieldName, std::array, 2> &itemFields) - : ROOT::Experimental::RRecordField(fieldName, std::move(itemFields), {}, - "std::pair<" + GetTypeList(itemFields) + ">") + : ROOT::Experimental::RRecordField(fieldName, "std::pair<" + GetTypeList(itemFields) + ">") { + AttachItemFields(std::move(itemFields)); + // ISO C++ does not guarantee any specific layout for `std::pair`; query TClass for the member offsets auto *c = TClass::GetClass(GetTypeName().c_str()); if (!c) @@ -3889,12 +3894,12 @@ ROOT::Experimental::RPairField::RPairField(std::string_view fieldName, auto firstElem = c->GetRealData("first"); if (!firstElem) throw RException(R__FAIL("first: no such member")); - fOffsets[0] = firstElem->GetThisOffset(); + fOffsets.push_back(firstElem->GetThisOffset()); auto secondElem = c->GetRealData("second"); if (!secondElem) throw RException(R__FAIL("second: no such member")); - fOffsets[1] = secondElem->GetThisOffset(); + fOffsets.push_back(secondElem->GetThisOffset()); } //------------------------------------------------------------------------------ @@ -3915,16 +3920,18 @@ ROOT::Experimental::RTupleField::RTupleField::GetTypeList(const std::vector> &&itemFields, const std::vector &offsets) - : ROOT::Experimental::RRecordField(fieldName, std::move(itemFields), offsets, - "std::tuple<" + GetTypeList(itemFields) + ">") + : ROOT::Experimental::RRecordField(fieldName, "std::tuple<" + GetTypeList(itemFields) + ">") { + AttachItemFields(std::move(itemFields)); + fOffsets = offsets; } ROOT::Experimental::RTupleField::RTupleField(std::string_view fieldName, std::vector> &itemFields) - : ROOT::Experimental::RRecordField(fieldName, std::move(itemFields), {}, - "std::tuple<" + GetTypeList(itemFields) + ">") + : ROOT::Experimental::RRecordField(fieldName, "std::tuple<" + GetTypeList(itemFields) + ">") { + AttachItemFields(std::move(itemFields)); + auto *c = TClass::GetClass(GetTypeName().c_str()); if (!c) throw RException(R__FAIL("cannot get type information for " + GetTypeName())); diff --git a/tree/ntuple/v7/test/ntuple_descriptor.cxx b/tree/ntuple/v7/test/ntuple_descriptor.cxx index 15fad274ce9f1..15348abf8da3d 100644 --- a/tree/ntuple/v7/test/ntuple_descriptor.cxx +++ b/tree/ntuple/v7/test/ntuple_descriptor.cxx @@ -594,7 +594,8 @@ TEST(RNTupleDescriptor, BuildStreamerInfos) streamerInfoMap = fnBuildStreamerInfosOf(*RFieldBase::Create("f", "std::unordered_map").Unwrap()); EXPECT_TRUE(streamerInfoMap.empty()); - streamerInfoMap = fnBuildStreamerInfosOf(ROOT::Experimental::RRecordField("f", {})); + std::vector> itemFields; + streamerInfoMap = fnBuildStreamerInfosOf(ROOT::Experimental::RRecordField("f", std::move(itemFields))); EXPECT_TRUE(streamerInfoMap.empty()); streamerInfoMap = fnBuildStreamerInfosOf(*RFieldBase::Create("f", "CustomStruct").Unwrap()); From 9a9d23333f5e60e12408eaa0e83f0a71c00f1c57 Mon Sep 17 00:00:00 2001 From: Jonas Hahnfeld Date: Mon, 28 Oct 2024 11:22:49 +0100 Subject: [PATCH 3/5] [ntuple] Take container of unique_ptr's by value Similar to taking a single std::unique_ptr by value to signal ownership handover, we should take containers thereof by value. This avoids having two overloads and requires the user to explicitly move the container of item fields instead of modifying it when passed by reference. --- .../v7/inc/ROOT/RField/RFieldRecord.hxx | 17 +++++++------ tree/ntuple/v7/src/RField.cxx | 24 +++++++------------ tree/ntuple/v7/src/RNTupleDescriptor.cxx | 2 +- tree/ntuple/v7/test/ntuple_show.cxx | 2 +- tree/ntuple/v7/test/ntuple_types.cxx | 2 +- tree/ntupleutil/v7/src/RNTupleImporter.cxx | 2 +- 6 files changed, 21 insertions(+), 28 deletions(-) diff --git a/tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx b/tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx index 549503033364c..cc3218a556590 100644 --- a/tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx +++ b/tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx @@ -46,7 +46,7 @@ private: std::vector fOffsets; public: - RRecordDeleter(std::vector> &itemDeleters, const std::vector &offsets) + RRecordDeleter(std::vector> itemDeleters, const std::vector &offsets) : fItemDeleters(std::move(itemDeleters)), fOffsets(offsets) { } @@ -73,10 +73,10 @@ protected: RRecordField(std::string_view fieldName, std::string_view typeName); - void AttachItemFields(std::vector> &&itemFields); + void AttachItemFields(std::vector> itemFields); template - void AttachItemFields(std::array, N> &&itemFields) + void AttachItemFields(std::array, N> itemFields) { fTraits |= kTraitTrivialType; for (unsigned i = 0; i < N; ++i) { @@ -90,8 +90,7 @@ protected: public: /// Construct a RRecordField based on a vector of child fields. The ownership of the child fields is transferred /// to the RRecordField instance. - RRecordField(std::string_view fieldName, std::vector> &&itemFields); - RRecordField(std::string_view fieldName, std::vector> &itemFields); + RRecordField(std::string_view fieldName, std::vector> itemFields); RRecordField(RRecordField &&other) = default; RRecordField &operator=(RRecordField &&other) = default; ~RRecordField() override = default; @@ -114,11 +113,11 @@ private: static std::string GetTypeList(const std::array, 2> &itemFields); protected: - RPairField(std::string_view fieldName, std::array, 2> &&itemFields, + RPairField(std::string_view fieldName, std::array, 2> itemFields, const std::array &offsets); public: - RPairField(std::string_view fieldName, std::array, 2> &itemFields); + RPairField(std::string_view fieldName, std::array, 2> itemFields); RPairField(RPairField &&other) = default; RPairField &operator=(RPairField &&other) = default; ~RPairField() override = default; @@ -165,11 +164,11 @@ private: static std::string GetTypeList(const std::vector> &itemFields); protected: - RTupleField(std::string_view fieldName, std::vector> &&itemFields, + RTupleField(std::string_view fieldName, std::vector> itemFields, const std::vector &offsets); public: - RTupleField(std::string_view fieldName, std::vector> &itemFields); + RTupleField(std::string_view fieldName, std::vector> itemFields); RTupleField(RTupleField &&other) = default; RTupleField &operator=(RTupleField &&other) = default; ~RTupleField() override = default; diff --git a/tree/ntuple/v7/src/RField.cxx b/tree/ntuple/v7/src/RField.cxx index 2b0c0fd23cbc5..d9e2f33b00aca 100644 --- a/tree/ntuple/v7/src/RField.cxx +++ b/tree/ntuple/v7/src/RField.cxx @@ -729,7 +729,7 @@ ROOT::Experimental::RFieldBase::Create(const std::string &fieldName, const std:: } std::array, 2> items{Create("_0", innerTypes[0]).Unwrap(), Create("_1", innerTypes[1]).Unwrap()}; - result = std::make_unique(fieldName, items); + result = std::make_unique(fieldName, std::move(items)); } else if (canonicalType.substr(0, 11) == "std::tuple<") { auto innerTypes = TokenizeTypeList(canonicalType.substr(11, canonicalType.length() - 12)); std::vector> items; @@ -737,7 +737,7 @@ ROOT::Experimental::RFieldBase::Create(const std::string &fieldName, const std:: for (unsigned int i = 0; i < innerTypes.size(); ++i) { items.emplace_back(Create("_" + std::to_string(i), innerTypes[i]).Unwrap()); } - result = std::make_unique(fieldName, items); + result = std::make_unique(fieldName, std::move(items)); } else if (canonicalType.substr(0, 12) == "std::bitset<") { auto size = std::stoull(canonicalType.substr(12, canonicalType.length() - 13)); result = std::make_unique(fieldName, size); @@ -2513,7 +2513,7 @@ ROOT::Experimental::RRecordField::RRecordField(std::string_view fieldName, std:: } void ROOT::Experimental::RRecordField::RRecordField::AttachItemFields( - std::vector> &&itemFields) + std::vector> itemFields) { fTraits |= kTraitTrivialType; for (auto &item : itemFields) { @@ -2525,7 +2525,7 @@ void ROOT::Experimental::RRecordField::RRecordField::AttachItemFields( } ROOT::Experimental::RRecordField::RRecordField(std::string_view fieldName, - std::vector> &&itemFields) + std::vector> itemFields) : ROOT::Experimental::RFieldBase(fieldName, "", ENTupleStructure::kRecord, false /* isSimple */) { fTraits |= kTraitTrivialType; @@ -2543,12 +2543,6 @@ ROOT::Experimental::RRecordField::RRecordField(std::string_view fieldName, fSize += GetItemPadding(fSize, fMaxAlignment); } -ROOT::Experimental::RRecordField::RRecordField(std::string_view fieldName, - std::vector> &itemFields) - : ROOT::Experimental::RRecordField(fieldName, std::move(itemFields)) -{ -} - std::size_t ROOT::Experimental::RRecordField::GetItemPadding(std::size_t baseOffset, std::size_t itemAlignment) const { if (itemAlignment > 1) { @@ -2610,7 +2604,7 @@ std::unique_ptr ROOT::Experimental::RR for (const auto &f : fSubFields) { itemDeleters.emplace_back(GetDeleterOf(*f)); } - return std::make_unique(itemDeleters, fOffsets); + return std::make_unique(std::move(itemDeleters), fOffsets); } std::vector @@ -3870,7 +3864,7 @@ ROOT::Experimental::RPairField::RPairField::GetTypeList(const std::array, 2> &&itemFields, + std::array, 2> itemFields, const std::array &offsets) : ROOT::Experimental::RRecordField(fieldName, "std::pair<" + GetTypeList(itemFields) + ">") { @@ -3880,7 +3874,7 @@ ROOT::Experimental::RPairField::RPairField(std::string_view fieldName, } ROOT::Experimental::RPairField::RPairField(std::string_view fieldName, - std::array, 2> &itemFields) + std::array, 2> itemFields) : ROOT::Experimental::RRecordField(fieldName, "std::pair<" + GetTypeList(itemFields) + ">") { AttachItemFields(std::move(itemFields)); @@ -3918,7 +3912,7 @@ ROOT::Experimental::RTupleField::RTupleField::GetTypeList(const std::vector> &&itemFields, + std::vector> itemFields, const std::vector &offsets) : ROOT::Experimental::RRecordField(fieldName, "std::tuple<" + GetTypeList(itemFields) + ">") { @@ -3927,7 +3921,7 @@ ROOT::Experimental::RTupleField::RTupleField(std::string_view fieldName, } ROOT::Experimental::RTupleField::RTupleField(std::string_view fieldName, - std::vector> &itemFields) + std::vector> itemFields) : ROOT::Experimental::RRecordField(fieldName, "std::tuple<" + GetTypeList(itemFields) + ">") { AttachItemFields(std::move(itemFields)); diff --git a/tree/ntuple/v7/src/RNTupleDescriptor.cxx b/tree/ntuple/v7/src/RNTupleDescriptor.cxx index 78c9f5326ba63..b4ea684f6366d 100644 --- a/tree/ntuple/v7/src/RNTupleDescriptor.cxx +++ b/tree/ntuple/v7/src/RNTupleDescriptor.cxx @@ -97,7 +97,7 @@ ROOT::Experimental::RFieldDescriptor::CreateField(const RNTupleDescriptor &ntplD return field; memberFields.emplace_back(std::move(field)); } - auto recordField = std::make_unique(GetFieldName(), memberFields); + auto recordField = std::make_unique(GetFieldName(), std::move(memberFields)); recordField->SetOnDiskId(fFieldId); return recordField; } diff --git a/tree/ntuple/v7/test/ntuple_show.cxx b/tree/ntuple/v7/test/ntuple_show.cxx index edea1fa4ce7ab..f12a44f8c2981 100644 --- a/tree/ntuple/v7/test/ntuple_show.cxx +++ b/tree/ntuple/v7/test/ntuple_show.cxx @@ -349,7 +349,7 @@ TEST(RNTupleShow, Collections) std::vector> leafFields; leafFields.emplace_back(std::make_unique>("myShort")); leafFields.emplace_back(std::make_unique>("myFloat")); - auto recordField = std::make_unique("_0", leafFields); + auto recordField = std::make_unique("_0", std::move(leafFields)); EXPECT_EQ(offsetof(MyStruct, myShort), recordField->GetOffsets()[0]); EXPECT_EQ(offsetof(MyStruct, myFloat), recordField->GetOffsets()[1]); diff --git a/tree/ntuple/v7/test/ntuple_types.cxx b/tree/ntuple/v7/test/ntuple_types.cxx index 14b0e8d741c6d..08a97c01828f4 100644 --- a/tree/ntuple/v7/test/ntuple_types.cxx +++ b/tree/ntuple/v7/test/ntuple_types.cxx @@ -138,7 +138,7 @@ TEST(RNTuple, CreateField) std::vector> itemFields; itemFields.push_back(std::make_unique>("u32")); itemFields.push_back(std::make_unique>("u8")); - ROOT::Experimental::RRecordField record("test", itemFields); + ROOT::Experimental::RRecordField record("test", std::move(itemFields)); EXPECT_EQ(alignof(std::uint32_t), record.GetAlignment()); // Check that trailing padding is added after `u8` to comply with the alignment requirements of uint32_t EXPECT_EQ(sizeof(std::uint32_t) + alignof(std::uint32_t), record.GetValueSize()); diff --git a/tree/ntupleutil/v7/src/RNTupleImporter.cxx b/tree/ntupleutil/v7/src/RNTupleImporter.cxx index 9712c81009734..2d93b38dcfe69 100644 --- a/tree/ntupleutil/v7/src/RNTupleImporter.cxx +++ b/tree/ntupleutil/v7/src/RNTupleImporter.cxx @@ -317,7 +317,7 @@ ROOT::Experimental::RResult ROOT::Experimental::RNTupleImporter::PrepareSc auto &c = p.second; c.fFieldName = "_collection" + std::to_string(iLeafCountCollection); - auto recordField = std::make_unique("_0", c.fLeafFields); + auto recordField = std::make_unique("_0", std::move(c.fLeafFields)); c.fRecordField = recordField.get(); auto collectionField = RVectorField::CreateUntyped(c.fFieldName, std::move(recordField)); fModel->AddField(std::move(collectionField)); From cd2865ad96d50f1d4317ce83444f9d83977ec5f7 Mon Sep 17 00:00:00 2001 From: Jonas Hahnfeld Date: Mon, 28 Oct 2024 13:00:15 +0100 Subject: [PATCH 4/5] [ntuple] Simplify some template functions --- tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx b/tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx index cc3218a556590..a3c19502cb3a0 100644 --- a/tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx +++ b/tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx @@ -128,10 +128,9 @@ class RField> final : public RPairField { using ContainerT = typename std::pair; private: - template static std::array, 2> BuildItemFields() { - return {std::make_unique>("_0"), std::make_unique>("_1")}; + return {std::make_unique>("_0"), std::make_unique>("_1")}; } static std::array BuildItemOffsets() @@ -144,7 +143,7 @@ private: public: static std::string TypeName() { return "std::pair<" + RField::TypeName() + "," + RField::TypeName() + ">"; } - explicit RField(std::string_view name) : RPairField(name, BuildItemFields(), BuildItemOffsets()) + explicit RField(std::string_view name) : RPairField(name, BuildItemFields(), BuildItemOffsets()) { fMaxAlignment = std::max(alignof(T1), alignof(T2)); fSize = sizeof(ContainerT); @@ -195,11 +194,10 @@ private: if constexpr (sizeof...(TailTs) > 0) _BuildItemFields(itemFields, index + 1); } - template static std::vector> BuildItemFields() { std::vector> result; - _BuildItemFields(result); + _BuildItemFields(result); return result; } @@ -212,18 +210,16 @@ private: if constexpr (sizeof...(TailTs) > 0) _BuildItemOffsets(offsets, tuple); } - template static std::vector BuildItemOffsets() { std::vector result; - _BuildItemOffsets<0, Ts...>(result, ContainerT()); + _BuildItemOffsets<0, ItemTs...>(result, ContainerT()); return result; } public: static std::string TypeName() { return "std::tuple<" + BuildItemTypes() + ">"; } - explicit RField(std::string_view name) - : RTupleField(name, BuildItemFields(), BuildItemOffsets()) + explicit RField(std::string_view name) : RTupleField(name, BuildItemFields(), BuildItemOffsets()) { fMaxAlignment = std::max({alignof(ItemTs)...}); fSize = sizeof(ContainerT); From bcc29e8591956cc65cc629b36a6b37db99a5d44b Mon Sep 17 00:00:00 2001 From: Jonas Hahnfeld Date: Mon, 28 Oct 2024 14:16:39 +0100 Subject: [PATCH 5/5] [ntuple] Fix fSize calculation in RRecordField::AttachItemFields It was missing the trailing padding, which was covered up by the typed RField specializations overriding the fSize member. Convert them into assertions, which can only compare inequality because the alignment of RMapField and RSetField are pessimized since commit 49ef6f91d7. --- tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx | 11 +++++++---- tree/ntuple/v7/src/RField.cxx | 3 +++ tree/ntuple/v7/test/ntuple_types.cxx | 8 ++++++++ 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx b/tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx index a3c19502cb3a0..02a9484d0f9f3 100644 --- a/tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx +++ b/tree/ntuple/v7/inc/ROOT/RField/RFieldRecord.hxx @@ -85,6 +85,9 @@ protected: fTraits &= itemFields[i]->GetTraits(); Attach(std::move(itemFields[i])); } + // Trailing padding: although this is implementation-dependent, most add enough padding to comply with the + // requirements of the type with strictest alignment + fSize += GetItemPadding(fSize, fMaxAlignment); } public: @@ -145,8 +148,8 @@ public: static std::string TypeName() { return "std::pair<" + RField::TypeName() + "," + RField::TypeName() + ">"; } explicit RField(std::string_view name) : RPairField(name, BuildItemFields(), BuildItemOffsets()) { - fMaxAlignment = std::max(alignof(T1), alignof(T2)); - fSize = sizeof(ContainerT); + R__ASSERT(fMaxAlignment >= std::max(alignof(T1), alignof(T2))); + R__ASSERT(fSize >= sizeof(ContainerT)); } RField(RField &&other) = default; RField &operator=(RField &&other) = default; @@ -221,8 +224,8 @@ public: static std::string TypeName() { return "std::tuple<" + BuildItemTypes() + ">"; } explicit RField(std::string_view name) : RTupleField(name, BuildItemFields(), BuildItemOffsets()) { - fMaxAlignment = std::max({alignof(ItemTs)...}); - fSize = sizeof(ContainerT); + R__ASSERT(fMaxAlignment >= std::max({alignof(ItemTs)...})); + R__ASSERT(fSize >= sizeof(ContainerT)); } RField(RField &&other) = default; RField &operator=(RField &&other) = default; diff --git a/tree/ntuple/v7/src/RField.cxx b/tree/ntuple/v7/src/RField.cxx index d9e2f33b00aca..b4dbd0622cd5e 100644 --- a/tree/ntuple/v7/src/RField.cxx +++ b/tree/ntuple/v7/src/RField.cxx @@ -2522,6 +2522,9 @@ void ROOT::Experimental::RRecordField::RRecordField::AttachItemFields( fTraits &= item->GetTraits(); Attach(std::move(item)); } + // Trailing padding: although this is implementation-dependent, most add enough padding to comply with the + // requirements of the type with strictest alignment + fSize += GetItemPadding(fSize, fMaxAlignment); } ROOT::Experimental::RRecordField::RRecordField(std::string_view fieldName, diff --git a/tree/ntuple/v7/test/ntuple_types.cxx b/tree/ntuple/v7/test/ntuple_types.cxx index 08a97c01828f4..79d48ba359c35 100644 --- a/tree/ntuple/v7/test/ntuple_types.cxx +++ b/tree/ntuple/v7/test/ntuple_types.cxx @@ -307,6 +307,14 @@ TEST(RNTuple, StdTuple) "tupleTupleField"); EXPECT_STREQ("std::tuple,std::vector>>", tupleTupleField.GetTypeName().c_str()); + using TupleMapSetType = std::tuple, std::set>; + auto tupleMapSetField = RField("tupleMapSetField"); + EXPECT_LE(sizeof(TupleMapSetType), tupleMapSetField.GetValueSize()); + EXPECT_LE(alignof(TupleMapSetType), tupleMapSetField.GetAlignment()); + using TupleUnorderedMapSetType = std::tuple, std::unordered_set>; + auto tupleUnorderedMapSetField = RField("tupleUnorderedMapSetField"); + EXPECT_LE(sizeof(TupleUnorderedMapSetType), tupleUnorderedMapSetField.GetValueSize()); + EXPECT_LE(alignof(TupleUnorderedMapSetType), tupleUnorderedMapSetField.GetAlignment()); FileRaii fileGuard("test_ntuple_rfield_stdtuple.root"); {