[ntuple] fixes and more testing for std::variant

tree/ntuple/v7/doc/specifications.md

@@ -778,6 +778,8 @@ Variants are stored in $n+1$ fields:
 
 The dispatch tag ranges from 1 to $n$.
 A value of 0 indicates that the variant is in the invalid state, i.e., it does not hold any of the valid alternatives.
+Variants must not have more than 125 subfields.
+This follows common compiler implementation limits.
 
 #### std::pair<T1, T2>
 

tree/ntuple/v7/inc/ROOT/RField.hxx

@@ -1349,14 +1349,22 @@ public:
 /// The generic field for std::variant types
 class RVariantField : public RFieldBase {
 private:
+   // Most compilers support at least 255 variants (256 - 1 value for the empty variant).
+   // Some compilers switch to a two-byte tag field already with 254 variants.
+   // MSVC only supports 163 variants in older versions, 250 in newer ones. It switches to a 2 byte
+   // tag as of 128 variants (at least in debug mode), so for simplicity we set the limit to 125 variants.
+   static constexpr std::size_t kMaxVariants = 125;
+
    class RVariantDeleter : public RDeleter {
    private:
       std::size_t fTagOffset;
+      std::size_t fVariantOffset;
       std::vector<std::unique_ptr<RDeleter>> fItemDeleters;
 
    public:
-      RVariantDeleter(std::size_t tagOffset, std::vector<std::unique_ptr<RDeleter>> &itemDeleters)
-         : fTagOffset(tagOffset), fItemDeleters(std::move(itemDeleters))
+      RVariantDeleter(std::size_t tagOffset, std::size_t variantOffset,
+                      std::vector<std::unique_ptr<RDeleter>> &itemDeleters)
+         : fTagOffset(tagOffset), fVariantOffset(variantOffset), fItemDeleters(std::move(itemDeleters))
       {
       }
       void operator()(void *objPtr, bool dtorOnly) final;
@@ -1366,12 +1374,17 @@ private:
    size_t fMaxAlignment = 1;
    /// In the std::variant memory layout, at which byte number is the index stored
    size_t fTagOffset = 0;
+   /// In the std::variant memory layout, the actual union of types may start at an offset > 0
+   size_t fVariantOffset = 0;
    std::vector<ClusterSize_t::ValueType> fNWritten;
 
    static std::string GetTypeList(const std::vector<RFieldBase *> &itemFields);
    /// Extracts the index from an std::variant and transforms it into the 1-based index used for the switch column
-   static std::uint32_t GetTag(const void *variantPtr, std::size_t tagOffset);
-   static void SetTag(void *variantPtr, std::size_t tagOffset, std::uint32_t tag);
+   /// The implementation supports two memory layouts that are in use: a trailing unsigned byte, zero-indexed,
+   /// having the exception caused empty state encoded by the max tag value,
+   /// or a trailing unsigned int instead of a char.
+   static std::uint8_t GetTag(const void *variantPtr, std::size_t tagOffset);
+   static void SetTag(void *variantPtr, std::size_t tagOffset, std::uint8_t tag);
 
 protected:
    std::unique_ptr<RFieldBase> CloneImpl(std::string_view newName) const final;
@@ -1396,7 +1409,7 @@ public:
    ~RVariantField() override = default;
 
    size_t GetValueSize() const final;
-   size_t GetAlignment() const final { return fMaxAlignment; }
+   size_t GetAlignment() const final;
 };
 
 /// The generic field for a std::set<Type> and std::unordered_set<Type>

tree/ntuple/v7/src/RField.cxx

@@ -374,6 +374,23 @@ ERNTupleUnsplitSetting GetRNTupleUnsplitSetting(TClass *cl)
    }
 }
 
+// Depending on the compiler, the variant tag is stored either in a trailing char or in a trailing unsigned int
+constexpr std::size_t GetVariantTagSize()
+{
+   // Should be all zeros except for the tag, which is 1
+   std::variant<char> t;
+   constexpr auto sizeOfT = sizeof(t);
+
+   static_assert(sizeOfT == 2 || sizeOfT == 8, "unsupported std::variant layout");
+   return sizeOfT == 2 ? 1 : 4;
+}
+
+template <std::size_t VariantSizeT>
+struct RVariantTag {
+   using ValueType_t = typename std::conditional_t<VariantSizeT == 1, std::uint8_t,
+                                                   typename std::conditional_t<VariantSizeT == 4, std::uint32_t, void>>;
+};
+
 } // anonymous namespace
 
 void ROOT::Experimental::Internal::CallCommitClusterOnField(RFieldBase &field)
@@ -3208,15 +3225,28 @@ ROOT::Experimental::RVariantField::RVariantField(std::string_view fieldName,
    fTraits |= kTraitTriviallyDestructible & ~kTraitTriviallyConstructible;
 
    auto nFields = itemFields.size();
-   R__ASSERT(nFields > 0);
+   if (nFields == 0 || nFields > kMaxVariants) {
+      throw RException(R__FAIL("invalid number of variant fields (outside [1.." + std::to_string(kMaxVariants) + ")"));
+   }
    fNWritten.resize(nFields, 0);
    for (unsigned int i = 0; i < nFields; ++i) {
       fMaxItemSize = std::max(fMaxItemSize, itemFields[i]->GetValueSize());
       fMaxAlignment = std::max(fMaxAlignment, itemFields[i]->GetAlignment());
       fTraits &= itemFields[i]->GetTraits();
       Attach(std::unique_ptr<RFieldBase>(itemFields[i]));
    }
-   fTagOffset = (fMaxItemSize < fMaxAlignment) ? fMaxAlignment : fMaxItemSize;
+
+   // With certain template parameters, the union of members of an std::variant starts at an offset > 0.
+   // For instance, std::variant<std::optional<int>> on macOS.
+   auto cl = TClass::GetClass(GetTypeName().c_str());
+   assert(cl);
+   auto dm = reinterpret_cast<TDataMember *>(cl->GetListOfDataMembers()->First());
+   if (dm)
+      fVariantOffset = dm->GetOffset();
+
+   const auto tagSize = GetVariantTagSize();
+   const auto padding = tagSize - (fMaxItemSize % tagSize);
+   fTagOffset = fVariantOffset + fMaxItemSize + ((padding == tagSize) ? 0 : padding);
 }
 
 std::unique_ptr<ROOT::Experimental::RFieldBase>
@@ -3232,16 +3262,18 @@ ROOT::Experimental::RVariantField::CloneImpl(std::string_view newName) const
    return std::make_unique<RVariantField>(newName, itemFields);
 }
 
-std::uint32_t ROOT::Experimental::RVariantField::GetTag(const void *variantPtr, std::size_t tagOffset)
+std::uint8_t ROOT::Experimental::RVariantField::GetTag(const void *variantPtr, std::size_t tagOffset)
 {
-   auto index = *(reinterpret_cast<const char *>(variantPtr) + tagOffset);
-   return (index < 0) ? 0 : index + 1;
+   using TagType_t = RVariantTag<GetVariantTagSize()>::ValueType_t;
+   auto tag = *reinterpret_cast<const TagType_t *>(reinterpret_cast<const unsigned char *>(variantPtr) + tagOffset);
+   return (tag == TagType_t(-1)) ? 0 : tag + 1;
 }
 
-void ROOT::Experimental::RVariantField::SetTag(void *variantPtr, std::size_t tagOffset, std::uint32_t tag)
+void ROOT::Experimental::RVariantField::SetTag(void *variantPtr, std::size_t tagOffset, std::uint8_t tag)
 {
-   auto index = reinterpret_cast<char *>(variantPtr) + tagOffset;
-   *index = static_cast<char>(tag - 1);
+   using TagType_t = RVariantTag<GetVariantTagSize()>::ValueType_t;
+   auto tagPtr = reinterpret_cast<TagType_t *>(reinterpret_cast<unsigned char *>(variantPtr) + tagOffset);
+   *tagPtr = (tag == 0) ? TagType_t(-1) : static_cast<TagType_t>(tag - 1);
 }
 
 std::size_t ROOT::Experimental::RVariantField::AppendImpl(const void *from)
@@ -3250,7 +3282,7 @@ std::size_t ROOT::Experimental::RVariantField::AppendImpl(const void *from)
    std::size_t nbytes = 0;
    auto index = 0;
    if (tag > 0) {
-      nbytes += CallAppendOn(*fSubFields[tag - 1], from);
+      nbytes += CallAppendOn(*fSubFields[tag - 1], reinterpret_cast<const unsigned char *>(from) + fVariantOffset);
       index = fNWritten[tag - 1]++;
    }
    RColumnSwitch varSwitch(ClusterSize_t(index), tag);
@@ -3263,13 +3295,15 @@ void ROOT::Experimental::RVariantField::ReadGlobalImpl(NTupleSize_t globalIndex,
    RClusterIndex variantIndex;
    std::uint32_t tag;
    fPrincipalColumn->GetSwitchInfo(globalIndex, &variantIndex, &tag);
+   R__ASSERT(tag < 256);
 
    // If `tag` equals 0, the variant is in the invalid state, i.e, it does not hold any of the valid alternatives in
    // the type list.  This happens, e.g., if the field was late added; in this case, keep the invalid tag, which makes
    // any `std::holds_alternative<T>` check fail later.
    if (R__likely(tag > 0)) {
-      CallConstructValueOn(*fSubFields[tag - 1], to);
-      CallReadOn(*fSubFields[tag - 1], variantIndex, to);
+      void *varPtr = reinterpret_cast<unsigned char *>(to) + fVariantOffset;
+      CallConstructValueOn(*fSubFields[tag - 1], varPtr);
+      CallReadOn(*fSubFields[tag - 1], variantIndex, varPtr);
    }
    SetTag(to, fTagOffset, tag);
 }
@@ -3295,15 +3329,15 @@ void ROOT::Experimental::RVariantField::GenerateColumnsImpl(const RNTupleDescrip
 void ROOT::Experimental::RVariantField::ConstructValue(void *where) const
 {
    memset(where, 0, GetValueSize());
-   CallConstructValueOn(*fSubFields[0], where);
+   CallConstructValueOn(*fSubFields[0], reinterpret_cast<unsigned char *>(where) + fVariantOffset);
    SetTag(where, fTagOffset, 1);
 }
 
 void ROOT::Experimental::RVariantField::RVariantDeleter::operator()(void *objPtr, bool dtorOnly)
 {
    auto tag = GetTag(objPtr, fTagOffset);
    if (tag > 0) {
-      fItemDeleters[tag - 1]->operator()(objPtr, true /* dtorOnly */);
+      fItemDeleters[tag - 1]->operator()(reinterpret_cast<unsigned char *>(objPtr) + fVariantOffset, true /*dtorOnly*/);
    }
    RDeleter::operator()(objPtr, dtorOnly);
 }
@@ -3315,12 +3349,20 @@ std::unique_ptr<ROOT::Experimental::RFieldBase::RDeleter> ROOT::Experimental::RV
    for (const auto &f : fSubFields) {
       itemDeleters.emplace_back(GetDeleterOf(*f));
    }
-   return std::make_unique<RVariantDeleter>(fTagOffset, itemDeleters);
+   return std::make_unique<RVariantDeleter>(fTagOffset, fVariantOffset, itemDeleters);
+}
+
+size_t ROOT::Experimental::RVariantField::GetAlignment() const
+{
+   return std::max(fMaxAlignment, alignof(RVariantTag<GetVariantTagSize()>::ValueType_t));
 }
 
 size_t ROOT::Experimental::RVariantField::GetValueSize() const
 {
-   return fMaxItemSize + fMaxAlignment;  // TODO: fix for more than 255 items
+   const auto alignment = GetAlignment();
+   const auto actualSize = fTagOffset + GetVariantTagSize();
+   const auto padding = alignment - (actualSize % alignment);
+   return actualSize + ((padding == alignment) ? 0 : padding);
 }
 
 void ROOT::Experimental::RVariantField::CommitClusterImpl()

tree/ntuple/v7/test/CMakeLists.txt

@@ -54,6 +54,10 @@ ROOT_ADD_GTEST(rfield_check rfield_check.cxx LIBRARIES ROOTNTuple CustomStruct)
 ROOT_ADD_GTEST(rfield_class rfield_class.cxx LIBRARIES ROOTNTuple CustomStruct Physics)
 ROOT_ADD_GTEST(rfield_string rfield_string.cxx LIBRARIES ROOTNTuple CustomStruct)
 ROOT_ADD_GTEST(rfield_variant rfield_variant.cxx LIBRARIES ROOTNTuple CustomStruct)
+if(MSVC)
+  # Necessary to have TClass handle very large variants
+  set_property(TARGET rfield_variant APPEND_STRING PROPERTY LINK_FLAGS " -STACK:4000000")
+endif()
 ROOT_ADD_GTEST(rfield_vector rfield_vector.cxx LIBRARIES ROOTNTuple CustomStruct)
 
 ROOT_ADD_GTEST(ntuple_minifile ntuple_minifile.cxx LIBRARIES ROOTNTuple Tree CustomStruct)

tree/ntuple/v7/test/CustomStruct.hxx

@@ -7,6 +7,7 @@
 #include <TVirtualCollectionProxy.h>
 
 #include <chrono>
+#include <stdexcept>
 #include <cstddef>
 #include <cstdint>
 #include <functional>
@@ -296,4 +297,10 @@ public:
    ClassDefOverride(DerivedFromLeftAndTObject, 1)
 };
 
+struct ThrowForVariant {
+   ThrowForVariant() = default;
+   ThrowForVariant(const ThrowForVariant &) { throw std::runtime_error("copy ctor"); }
+   ThrowForVariant &operator=(const ThrowForVariant &) = default;
+};
+
 #endif

tree/ntuple/v7/test/CustomStructLinkDef.h

@@ -82,4 +82,6 @@
 #pragma link C++ class Left + ;
 #pragma link C++ class DerivedFromLeftAndTObject+;
 
+#pragma link C++ class ThrowForVariant + ;
+
 #endif

tree/ntuple/v7/test/ntuple_types.cxx

@@ -1170,6 +1170,7 @@ TEST(RNTuple, Optional)
 {
    using CharArray3_t = std::array<char, 3>;
    using CharArray4_t = std::array<char, 4>;
+   using Variant_t = std::variant<int, float>;
 
    EXPECT_EQ(sizeof(std::optional<char>), RField<std::optional<char>>("f").GetValueSize());
    EXPECT_EQ(alignof(std::optional<char>), RField<std::optional<char>>("f").GetAlignment());
@@ -1184,6 +1185,8 @@ TEST(RNTuple, Optional)
    EXPECT_EQ(alignof(std::optional<double>), RField<std::optional<double>>("f").GetAlignment());
    EXPECT_EQ(sizeof(std::optional<CustomStruct>), RField<std::optional<CustomStruct>>("f").GetValueSize());
    EXPECT_EQ(alignof(std::optional<CustomStruct>), RField<std::optional<CustomStruct>>("f").GetAlignment());
+   EXPECT_EQ(sizeof(std::optional<Variant_t>), RField<std::optional<Variant_t>>("f").GetValueSize());
+   EXPECT_EQ(alignof(std::optional<Variant_t>), RField<std::optional<Variant_t>>("f").GetAlignment());
 
    FileRaii fileGuard("test_ntuple_optional.root");
 
@@ -1195,6 +1198,7 @@ TEST(RNTuple, Optional)
       auto pOptInt16 = model->MakeField<std::optional<std::int16_t>>("oi");
       auto pOptFloat = model->MakeField<std::optional<float>>("of");
       auto pOptDouble = model->MakeField<std::optional<double>>("od");
+      auto pOptVariant = model->MakeField<std::optional<Variant_t>>("ov");
       model->AddField(RFieldBase::Create("ocs", "std::optional<CustomStruct>").Unwrap());
 
       auto writer = RNTupleWriter::Recreate(std::move(model), "ntuple", fileGuard.GetPath());
@@ -1209,6 +1213,7 @@ TEST(RNTuple, Optional)
       *pOptInt16 = 137;
       *pOptFloat = 1.0;
       *pOptDouble = 2.0;
+      *pOptVariant = float(3.0);
       *pOptCustomStruct = CustomStruct();
       writer->Fill();
       pOptChar->reset();
@@ -1217,6 +1222,7 @@ TEST(RNTuple, Optional)
       pOptInt16->reset();
       pOptFloat->reset();
       pOptDouble->reset();
+      pOptVariant->reset();
       pOptCustomStruct->reset();
       writer->Fill();
    }
@@ -1231,6 +1237,7 @@ TEST(RNTuple, Optional)
    auto pOptInt16 = defaultEntry.GetPtr<std::optional<std::int16_t>>("oi");
    auto pOptFloat = defaultEntry.GetPtr<std::optional<float>>("of");
    auto pOptDouble = defaultEntry.GetPtr<std::optional<double>>("od");
+   auto pOptVariant = defaultEntry.GetPtr<std::optional<Variant_t>>("ov");
    auto pOptCustomStruct = defaultEntry.GetPtr<std::optional<CustomStruct>>("ocs");
 
    reader->LoadEntry(0);
@@ -1241,6 +1248,7 @@ TEST(RNTuple, Optional)
    EXPECT_FALSE(*pOptInt16);
    EXPECT_FALSE(*pOptFloat);
    EXPECT_FALSE(*pOptDouble);
+   EXPECT_FALSE(*pOptVariant);
    EXPECT_FALSE(*pOptCustomStruct);
 
    reader->LoadEntry(1);
@@ -1253,6 +1261,7 @@ TEST(RNTuple, Optional)
    EXPECT_EQ(137, *pOptInt16);
    EXPECT_FLOAT_EQ(1.0, pOptFloat->value());
    EXPECT_DOUBLE_EQ(2.0, pOptDouble->value());
+   EXPECT_FLOAT_EQ(3.0, std::get<1>(pOptVariant->value()));
    EXPECT_EQ(CustomStruct(), *pOptCustomStruct);
 
    reader->LoadEntry(2);
@@ -1263,6 +1272,7 @@ TEST(RNTuple, Optional)
    EXPECT_FALSE(*pOptInt16);
    EXPECT_FALSE(*pOptFloat);
    EXPECT_FALSE(*pOptDouble);
+   EXPECT_FALSE(*pOptVariant);
    EXPECT_FALSE(*pOptCustomStruct);
 }