RNTuple: fields with mixed STL types sometimes fail to be filled

[ariostas]

Check duplicate issues.

• Checked for duplicates

Description

When having fields with mixed STL types sometimes you run into issues when trying to fill them. I've attached an example below.

Reproducer

This is a minimal script that shows that issue.

#include <ROOT/RNTupleModel.hxx>
#include <ROOT/RNTupleWriter.hxx>
 
#include <TSystem.h>
 
#include <vector>
#include <variant>
#include <optional>
 
using RNTupleModel = ROOT::Experimental::RNTupleModel;
using RNTupleWriter = ROOT::Experimental::RNTupleWriter;
 
void ntpl_issue()
{
   auto model = RNTupleModel::Create();
 
   auto fldVvar = model->MakeField<std::vector<std::variant<std::optional<int>,float>>>("vvar");
 
   auto ntuple = RNTupleWriter::Recreate(std::move(model), "F", "ntpl_issue.root");
 
   for (int i = 0; i < 10; i++) {
      fldVvar->clear();
 
      for (int j = 0; j < 5; ++j) {
         std::variant<std::optional<int>,float> var(1.0);
         fldVvar->emplace_back(var);
      }
 
      ntuple->Fill();
   }
}

And this is the error that it produces.

Fatal: (typedValue->size() % fItemSize) == 0 violated at line 2432 of `.../root_src/tree/ntuple/v7/src/RField.cxx'
aborting

Another way to get it to fail is by using std::vector<std::variant<std::atomic<int>,float>>.

ROOT version

6.33/01 (commit eef2244)

Installation method

Built from source

Operating system

macOS

Additional context

I found this issue while trying to generate std::variant types in an invalid state by doing something like this.

struct S {
   operator int() { throw 42; }
};

std::variant<int,float> var;
try {
  var = S();
} catch (int) {}
fldVvar->emplace_back(var);

The spec indicates that invalid states are supported, as shown in the line below. I wanted to ask what's the reason for them being supported given that they are not really supposed to be "legal", and it seems like one can't successfully generate an RNTuple with an invalid state. It either ends up initializing the first variant or it crashes.

root/tree/ntuple/v7/doc/specifications.md

Line 780 in 9530711

A value of 0 indicates that the variant is in the invalid state, i.e., it does not hold any of the valid alternatives.

[jblomer]

Root cause may actually be a bug in the std::variant field. But needs more investigation.

[jblomer]

I cannot reproduce it on Linux, it could be a macOS specific issue.

[jblomer]

The PR contains a test that demonstrates that user code can produce empty variants, which the I/O system needs to handle.

[jblomer]

Several fixes are up in the PR, however there is something strange about std::variant<std::optional<int>>. On macOS, this type has a size of 16 (and not 12 as one would expect, with 8 bytes for the int + bool struct of the optional plus 4 bytes for the index member). Moreover, the additional 4 bytes come from padding at the beginning of the variant type. Clang's dump-record-layouts gives the following, where I cannot explain why the __impl member starts at an offset of 4.

Could it be related to empty base class optimization, or in this case the lack thereof due to multiple inheritance? But in this case, why is the padding missing for std::variant<X>, with struct X {int i; bool b;};?

*** Dumping AST Record Layout
         0 | class std::variant<class std::optional<int> >
         0 |   struct std::__sfinae_ctor_base<true, true> (base) (empty)
         0 |   struct std::__sfinae_assign_base<true, true> (base) (empty)
         4 |   class std::__variant_detail::__impl<class std::optional<int> > __impl
         4 |     class std::__variant_detail::__copy_assignment<struct std::__variant_detail::__traits<class std::optional<int> >, std::__variant_detail::_Trait::_TriviallyAvailable> (base)
         4 |       class std::__variant_detail::__move_assignment<struct std::__variant_detail::__traits<class std::optional<int> >, std::__variant_detail::_Trait::_TriviallyAvailable> (base)
         4 |         class std::__variant_detail::__assignment<struct std::__variant_detail::__traits<class std::optional<int> > > (base)
         4 |           class std::__variant_detail::__copy_constructor<struct std::__variant_detail::__traits<class std::optional<int> >, std::__variant_detail::_Trait::_TriviallyAvailable> (base)
         4 |             class std::__variant_detail::__move_constructor<struct std::__variant_detail::__traits<class std::optional<int> >, std::__variant_detail::_Trait::_TriviallyAvailable> (base)
         4 |               class std::__variant_detail::__ctor<struct std::__variant_detail::__traits<class std::optional<int> > > (base)
         4 |                 class std::__variant_detail::__dtor<struct std::__variant_detail::__traits<class std::optional<int> >, std::__variant_detail::_Trait::_TriviallyAvailable> (base)
         4 |                   class std::__variant_detail::__base<std::__variant_detail::_Trait::_TriviallyAvailable, class std::optional<int> > (base)
         4 |                     union std::__variant_detail::__union<std::__variant_detail::_Trait::_TriviallyAvailable, 0, class std::optional<int> > __data
         4 |                       char __dummy
         4 |                       struct std::__variant_detail::__alt<0, class std::optional<int> > __head
         4 |                         class std::optional<int> __value
         4 |                           struct std::__optional_move_assign_base<int, true> (base)
         4 |                             struct std::__optional_copy_assign_base<int, true> (base)
         4 |                               struct std::__optional_move_base<int, true> (base)
         4 |                                 struct std::__optional_copy_base<int, true> (base)
         4 |                                   struct std::__optional_storage_base<int, false> (base)
         4 |                                     struct std::__optional_destruct_base<int, true> (base)
         4 |                                       union std::__optional_destruct_base<int, true>::(anonymous at /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/optional:258:5) 
         4 |                                         char __null_state_
         4 |                                         std::__optional_destruct_base<int, true>::value_type __val_
         8 |                                       _Bool __engaged_
         4 |                           struct std::__sfinae_ctor_base<true, true> (base) (empty)
         4 |                           struct std::__sfinae_assign_base<true, true> (base) (empty)
         4 |                       union std::__variant_detail::__union<std::__variant_detail::_Trait::_TriviallyAvailable, 1> __tail (empty)
        12 |                     std::__variant_detail::__base<std::__variant_detail::_Trait::_TriviallyAvailable, class std::optional<int> >::__index_t __index
           | [sizeof=16, dsize=16, align=4,
           |  nvsize=16, nvalign=4]

[hahnjo]

Could it be related to empty base class optimization, or in this case the lack thereof due to multiple inheritance? But in this case, why is the padding missing for std::variant<X>, with struct X {int i; bool b;};?

So it turns out this is due to EBCO, or rather its non-happening: The reason is that both std::variant and std::optional inherit from __sfinae_ctor_base and __sfinae_assign_base and EBCO is not allowed to optimize two empty types at the same offset. That's why the inner type has to have at least one byte of padding, which increases to 4 bytes offset due to alignment. A simplified example is

class Empty {};
class Inner : private Empty {
    int i;
};
class Outer1 {
    Inner i;
};
class Outer2 : private Empty {
    Inner i;
};

static_assert(sizeof(Outer1) == 4);
static_assert(sizeof(Outer2) == 8);

(https://godbolt.org/z/6dGdTK6ha) where Outer2 mimics the case of std::variant<std::optional<...>>. Naturally, struct X on the other hand does not inherit from the same empty base classes and doesn't need the padding.

The problem for RNTuple code is that this can happen for other STL (value) containers as well; we should probably check std::pair, std::tuple and maybe also maps (not familiar with the implementation). std::vectors should be fine because we only need to locate the pointer. (Edit: after further investigation, it appears that this problem is specific to the combination of std::variant and std::optional that are the only ones using the __sfinae_* base classes)

(FWIW I will also report this to the libc++ developers; they can't do much about it without breaking the ABI, but at least I want to make them aware of this inefficiency in the implementation.) (edit: does a std::variant<std::optional<...>> actually make much sense?)

[jblomer]

Nice investigation!

(FWIW I will also report this to the libc++ developers; they can't do much about it without breaking the ABI, but at least I want to make them aware of this inefficiency in the implementation.) (edit: does a std::variant<std::optional<...>> actually make much sense?)

As a slightly larger type std::variant<std::optional<int>, std::optional<float>>, I think it does make sense.